apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "David Reid" <dr...@jetnet.co.uk>
Subject Re: [PATCH] fix design bug in apr_file_gets()
Date Wed, 30 Apr 2003 16:38:10 GMT
+1 from me!


----- Original Message -----
From: "Jeff Trawick" <trawick@attglobal.net>
To: <dev@apr.apache.org>
Sent: Sunday, April 27, 2003 7:13 PM
Subject: [PATCH] fix design bug in apr_file_gets()


> change apr_file_gets() to return APR_SUCCESS if returning any data
>
> current apr_file_gets() will return APR_EOF if it hit end of file before
> newline
>
> current callers of apr_file_gets() already need to check for existence
> of newline since buffer overflow will cause apr_file_gets() to return
> APR_SUCCESS + line with no '\n'; that logic will normally discover
> missing newline at end of file
>
> this change will resolve bugs in such callers as mod_cgi and mod_cgid
> which did not have logic to find data with no final '\n' at end of file;
> conceivably it will make a number of other callers accept data when
> there is no '\n'
>
> if somebody really really wants until apr 1.0 to fix this, we can have
> apr_file_gets_foo() until then for callers which would rather have apr
> do the heavy lifting instead of adding missing logic to deal with the
> fact that on normal files (pipes) where there are no I/O errors good
> data can come even when rv != APR_SUCCESS
>


----------------------------------------------------------------------------
----


> Index: file_io/os2/readwrite.c
> ===================================================================
> RCS file: /home/cvs/apr/file_io/os2/readwrite.c,v
> retrieving revision 1.57
> diff -u -r1.57 readwrite.c
> --- file_io/os2/readwrite.c 4 Mar 2003 22:19:38 -0000 1.57
> +++ file_io/os2/readwrite.c 27 Apr 2003 17:53:01 -0000
> @@ -330,6 +330,7 @@
>
>  APR_DECLARE(apr_status_t) apr_file_gets(char *str, int len, apr_file_t
*thefile)
>  {
> +    const char *str_start = str;
>      apr_size_t readlen;
>      apr_status_t rv = APR_SUCCESS;
>      int i;
> @@ -349,6 +350,12 @@
>          }
>      }
>      str[i] = 0;
> +    if (str > str_start) {
> +        /* we stored chars; don't report EOF or any other errors;
> +         * the app will find out about that on the next call
> +         */
> +        return APR_SUCCESS;
> +    }
>      return rv;
>  }
>
> Index: file_io/unix/readwrite.c
> ===================================================================
> RCS file: /home/cvs/apr/file_io/unix/readwrite.c,v
> retrieving revision 1.84
> diff -u -r1.84 readwrite.c
> --- file_io/unix/readwrite.c 7 Jan 2003 00:52:53 -0000 1.84
> +++ file_io/unix/readwrite.c 27 Apr 2003 17:53:01 -0000
> @@ -330,6 +330,7 @@
>  {
>      apr_status_t rv = APR_SUCCESS; /* get rid of gcc warning */
>      apr_size_t nbytes;
> +    const char *str_start = str;
>      char *final = str + len - 1;
>
>      if (len <= 1)

> @@ -353,7 +354,13 @@
>      /* We must store a terminating '\0' if we've stored any chars. We can
>       * get away with storing it if we hit an error first.
>       */
> -    *str = '\0';
> +    *str = '\0';
> +    if (str > str_start) {
> +        /* we stored chars; don't report EOF or any other errors;
> +         * the app will find out about that on the next call
> +         */
> +        return APR_SUCCESS;
> +    }
>      return rv;
>  }
>
> Index: file_io/win32/readwrite.c
> ===================================================================
> RCS file: /home/cvs/apr/file_io/win32/readwrite.c,v
> retrieving revision 1.78
> diff -u -r1.78 readwrite.c
> --- file_io/win32/readwrite.c 4 Mar 2003 22:19:38 -0000 1.78
> +++ file_io/win32/readwrite.c 27 Apr 2003 17:53:01 -0000
> @@ -439,6 +439,7 @@
>
>  APR_DECLARE(apr_status_t) apr_file_gets(char *str, int len, apr_file_t
*thefile)
>  {
> +    const char *str_start = str;
>      apr_size_t readlen;
>      apr_status_t rv = APR_SUCCESS;
>      int i;
> @@ -458,6 +459,12 @@
>          }
>      }
>      str[i] = 0;
> +    if (str > str_start) {
> +        /* we stored chars; don't report EOF or any other errors;
> +         * the app will find out about that on the next call
> +         */
> +        return APR_SUCCESS;
> +    }
>      return rv;
>  }
>
> Index: include/apr_file_io.h
> ===================================================================
> RCS file: /home/cvs/apr/include/apr_file_io.h,v
> retrieving revision 1.138
> diff -u -r1.138 apr_file_io.h
> --- include/apr_file_io.h 3 Apr 2003 23:20:05 -0000 1.138
> +++ include/apr_file_io.h 27 Apr 2003 17:53:02 -0000
> @@ -451,8 +451,6 @@
>   * @param str The buffer to store the string in.
>   * @param len The length of the string
>   * @param thefile The file descriptor to read from
> - * @remark APR_EOF will be returned if some characters are read but the
end
> - * of file is reached before a newline is read.
>   * @remark The buffer will be '\0'-terminated if any characters are
stored,
>   * even if something other than APR_SUCCESS is returned.
>   */
> Index: test/testfile.c
> ===================================================================
> RCS file: /home/cvs/apr/test/testfile.c,v
> retrieving revision 1.67
> diff -u -r1.67 testfile.c
> --- test/testfile.c 2 Jan 2003 14:05:42 -0000 1.67
> +++ test/testfile.c 27 Apr 2003 17:53:02 -0000
> @@ -369,12 +369,15 @@
>      CuAssertIntEquals(tc, APR_SUCCESS, rv);
>
>      rv = apr_file_gets(str, 256, f);
> -    /* Only one line in the test file, so we should get the EOF on the
first
> -     * call to gets.
> +    /* Only one line in the test file, so we will encounter  EOF on the
first
> +     * call to gets, but we should get APR_SUCCESS on this call and
> +     * APR_EOF on the next.
>       */
> -    CuAssertIntEquals(tc, APR_EOF, rv);
> +    CuAssertIntEquals(tc, APR_SUCCESS, rv);
>      CuAssertStrEquals(tc, TESTSTR, str);
> -
> +    rv = apr_file_gets(str, 256, f);
> +    CuAssertIntEquals(tc, APR_EOF, rv);
> +    CuAssertStrEquals(tc, "", str);
>      apr_file_close(f);
>  }
>
>


Mime
View raw message