On 01/28/2017 07:14 PM, Daniel Ruggeri wrote:
>
> On 1/25/2017 9:02 PM, Daniel Ruggeri wrote:
>> On 1/25/2017 6:53 PM, Daniel Ruggeri wrote:
>>> I'd say that not returning until ctx->bb has enough information to
>>> determine if the header is present or not would be sufficient. Isn't
>>> this already done in the potentially repeated calls to ap_get_brigade on
>>> line no 1056 inside the loop verifying that ctx->done is not set? If
>>> we're not intentionally holding onto the data until we know it's safe,
>>> then I think there's a structural problem in this module that would
>>> require us to start doing so.
>> Sorry - I neglected to notice the obvious check that the brigade is not
>> empty JUST before this line. Nevermind this silly comment...
>>
>> Indeed, if the brigade runs dry before the minimum number of bytes
>> needed has been read, the data should not be deleted.
>> I'm thinking apr_bucket_setaside(b, f->c->pool) in place of
>> apr_bucket_delete(b) would be good here... I will also experiment with
>> having the filter ask for less data than it needs to verify these
>> multiple calls through the filter work as expected.
>>
>
> Changes to set aside the bucket data are in r1780725. I'll await
> updating the 2.4 backport patch with this and the compiler warning fix
> when we're good on how to proceed regarding the other email I just sent.
>
> To verify behavior, I forced the filter to receive only a few bytes at a
> time until it "built up" enough data to evaluate the header type and
> pass all data set aside in bb_out when RemoteIPProxyProtocol is set to
> Optional. Thanks for the pointers.
>
I think there are still several edge cases open, especially in the optional
case. Let me try to list.
1. I guess you should step aside (for this particular call) if the filter is
called with the following modes:
AP_MODE_INIT
AP_MODE_EATCRLF ???
2. All the real fun starts with the optional case if you do not find the PROXY
header:
1. The original call to your filter requested less bytes than MIN_HDR_LEN. You
can only
return the amount of data requested and you need to keep the remaining stuff
in your ctx->store_bb. For each
following call you need to hand out your remaining data in ctx->store_bb
first.
2. 1. gets even more fun if the the reader requested mode AP_MODE_SPECULATIVE.
In this case you can proceed as in 1.,
but you need to hand out copies of the data as you need to be able to return
that data later again in case of a non
speculative read.
3. In AP_MODE_GETLINE you need to ensure that you don't return data past a LF
character. If there is no LF in your
ctx->store_bb data you need to add the missing data by doing a
ap_get_brigade on your own.
4. There might be intermittent calls of 1., 2. and 3.
Thinking of all the above it might be best if you read in mode
AP_MODE_SPECULATIVE on your own from upstream until
you have MIN_HDR_LEN data. If the PROXY header is present, read MIN_HDR_LEN in
AP_MODE_READBYTES to finally consume the
data and move on. If the PROXY header is not present, well then just forward
the original request and you are fine.
This way you leave all the hassle to the upstream filters.
Regards
Rüdiger
