Bug 41143

Created attachment 19239 [details]
patch against the 2.2.x branch

Fixed in 2.2.4

Are you sure? which commit fixed it? I fail to see how this was solved. The code in
question is still present on the 2.2.x branch and trunk.

(In reply to comment #0)
> The misplaced ap_getline may discard a valid header after a too long header,
ap_getline already discards 
> extra data.

I am no sure if this is true in the case that the header is larger than 16KB,
because in this case ap_rgetline_core will be left after the second
ap_get_brigade (in the case we only have the ap_core_input_filter in the input
filter chain, not checked the SSL case) which only read 16KB from the socket.

> The patch (side effect) also reduces the stack usage considerably (by 8KB).

(In reply to comment #4)
> (In reply to comment #0)
> > The misplaced ap_getline may discard a valid header after a too long header,
> > ap_getline already discards extra data.
> 
> I am no sure if this is true in the case that the header is larger than 16KB,
> because in this case ap_rgetline_core will be left after the second
> ap_get_brigade (in the case we only have the ap_core_input_filter in the input
> filter chain, not checked the SSL case) which only read 16KB from the socket.
> 

Bear with me:

ap_proxy_read_headers() calls ap_getline() with a buffer of 8192 bytes. ap_getline()
creates a brigade and calls ap_rgetline() to get a line of protocol input (and to copy
the line to the buffer). ap_rgetline_core() calls ap_get_brigade(AP_MODE_GETLINE)
which returns a line (brigade) of 8200 bytes (for the sake of example). If all data
is in the first bucket, the line size could overflow the buffer, then APR_ENOSPC
is returned. ap_getline sees the APR_ENOSPC and returns the buffer size.

We have two problems here. First, if the data of the first bucket overflows the buffer,
the data is not copied but ap_getline returns the size of the buffer (meaning that the
data was copied). Second, ap_proxy_read_headers checks if the returned len is >= than
the buffer size-1, if so, it calls ap_getline again, but the line was already read by
ap_get_brigade and discarded.

Makes sense now?

(In reply to comment #5)

> Bear with me:
> 
> ap_proxy_read_headers() calls ap_getline() with a buffer of 8192 bytes.
ap_getline()
> creates a brigade and calls ap_rgetline() to get a line of protocol input (and
to copy
> the line to the buffer). ap_rgetline_core() calls ap_get_brigade(AP_MODE_GETLINE)
> which returns a line (brigade) of 8200 bytes (for the sake of example). If all
data

But ap_get_brigade(AP_MODE_GETLINE) returns 8192 bytes at max and it is not
guaranteed that this data contains a LF. It is only guaranteed that
ap_get_brigade(AP_MODE_GETLINE) does not read *past* a LF. See
ap_core_input_filter and apr_brigade_split_line for why.


> 
> Makes sense now?

I agree that current code does it wrong, but I fear that there are other cases
where your patch will do it wrong.

(In reply to comment #6)
> (In reply to comment #5)
> 
> 
> But ap_get_brigade(AP_MODE_GETLINE) returns 8192 bytes at max and it is not
> guaranteed that this data contains a LF.

No! ap_get_brigade(AP_MODE_GETLINE) may return much more than 8192 bytes.
Suppose a brigade has two buckets of APR_BUCKET_BUFF_SIZE (8000) bytes each
and the line break is on the second bucket, at position 4000. In this case, 
apr_brigade_split_line() will read the first bucket, and will proceed to read the
next one since it's size is not >= HUGE_STRING_LEN. It will find a LF on the second
one and return a brigade of 12000 bytes.

> I agree that current code does it wrong, but I fear that there are other cases
> where your patch will do it wrong.

Would you care to elaborate? Also, i'm not sure too if it's the best fix, but it can't
get any worse (famous last words).

I forgot to mention, but but if my memory serves well I spotted this issue while playing with
large cookies. It's not a critical real-world issue, yet :-)

Thanks for you comments.

(In reply to comment #7)
> (In reply to comment #6)

> > But ap_get_brigade(AP_MODE_GETLINE) returns 8192 bytes at max and it is not
> > guaranteed that this data contains a LF.
> 
> No! ap_get_brigade(AP_MODE_GETLINE) may return much more than 8192 bytes.

Ok. You are correct with this. Sorry, I misread the loop. 

> Suppose a brigade has two buckets of APR_BUCKET_BUFF_SIZE (8000) bytes each
> and the line break is on the second bucket, at position 4000. In this case, 
> apr_brigade_split_line() will read the first bucket, and will proceed to read the
> next one since it's size is not >= HUGE_STRING_LEN. It will find a LF on the
second
> one and return a brigade of 12000 bytes.

So apr_brigade_split_line() will not return more than maxbytes-1 + the size of
the following bucket. So the brigade returned by ap_get_brigade(AP_MODE_GETLINE)
may not contain the LF if the header is long enough (I would suppose that 16KB =
2 * 8000 (APR_BUCKET_BUFF_SIZE) + 384 should be sufficient to get there).
I admit that this is a constructed edge case that might not happen in real life :-).

> 
> > I agree that current code does it wrong, but I fear that there are other cases
> > where your patch will do it wrong.
> 
> Would you care to elaborate? Also, i'm not sure too if it's the best fix, but
it can't
> get any worse (famous last words).

It is not getting worse, only different :-).