Possible error in apr_rgetline_core
Lines 250-263 of server/protocol.c:
250/* Would this overrun our buffer? If so, we'll die. */
251if (n bytes_handled + len) {
252*read = bytes_handled;
253if (*s) {
254/* ensure this string is terminated */
255if (bytes_handled n) {
256(*s)[bytes_handled-1] = '\0';
257}
258else {
259(*s)[n-1] = '\0';
260}
261}
262return APR_ENOSPC;
263}
The first time through this loop, bytes_handled will be 0. If the
buffer was provided, rather than being allocated by ap_rgetline_core,
and the first read exceeded the maximum length (n), then line 256 will
set the byte *before* the buffer to 0.
I believe that the change introduced at revision 1.152 is incorrect;
the test at line 255 ensures that the buffer will not be overrun, so
the change from bytes_handled to bytes_handled-1 in line 256 was
unnecessary.
Rici
Re: Possible error in apr_rgetline_core
On Tue, Sep 21, 2004 at 10:00:48AM -0500, Rici Lake wrote: The first time through this loop, bytes_handled will be 0. If the buffer was provided, rather than being allocated by ap_rgetline_core, and the first read exceeded the maximum length (n), then line 256 will set the byte *before* the buffer to 0. you know, I had even traced this through and checked that bytes_handled must be 0 if *s is non-NULL, conveniently forgetting about the user-supplied *s case :( I believe that the change introduced at revision 1.152 is incorrect; the test at line 255 ensures that the buffer will not be overrun, so the change from bytes_handled to bytes_handled-1 in line 256 was unnecessary. I don't agree with this, though... trace through the old code with n = 8192, for two reads of len=8191, and len=5: pass 1: bytes_handled = 0, *s = NULL apr_bucket_read() = len = 8191 if (n bytes_handled + len) = false current_alloc = 8191 *s = apr_palloc(, 8191) bytes_handled += 8191 pass 2: bytes_handled = 8191, *s = 8191-byte region apr_bucket_read() = len = 5 if (n bytes_handled + len) : 8192 8191 + 5 = true *read = 8191 if (*s) = true if (bytes_handled n) : 8191 8192 = true (*s)[8191] = '\0' = off-by-one
Re: Possible error in apr_rgetline_core
On 21-Sep-04, at 11:03 AM, Joe Orton wrote:
pass 2: bytes_handled = 8191, *s = 8191-byte region
Quite right: I hadn't looked closely enough at the case where there was
no user-supplied buffer :(
Ok, bytes_handled can never be n, right?
So the following ought to work:
255if (bytes_handled 0) {
256(*s)[bytes_handled-1] = '\0';
257}
258else {
259(*s)[0] = '\0';
260}
Re: Possible error in apr_rgetline_core
On Tue, Sep 21, 2004 at 11:33:32AM -0500, Rici Lake wrote:
On 21-Sep-04, at 11:03 AM, Joe Orton wrote:
pass 2: bytes_handled = 8191, *s = 8191-byte region
Quite right: I hadn't looked closely enough at the case where there was
no user-supplied buffer :(
Between us we'll have it covered...
Ok, bytes_handled can never be n, right?
So the following ought to work:
Yes, good, I was about to add a preceding if (bytes_handled == 0) branch
but your patch is better, I'll commit it this evening. Thanks for this.
255if (bytes_handled 0) {
256(*s)[bytes_handled-1] = '\0';
257}
258else {
259(*s)[0] = '\0';
260}
