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
