+1 from me!
----- Original Message ----- From: "Jeff Trawick" <[EMAIL PROTECTED]> To: <[email protected]> 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); > } > >
