httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ruediger Pluem <rpl...@apache.org>
Subject Re: AW: svn commit: r1776575 - in /httpd/httpd/trunk: docs/log-message-tags/next-number docs/manual/mod/mod_remoteip.xml modules/metadata/mod_remoteip.c
Date Mon, 30 Jan 2017 10:45:11 GMT


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

Mime
View raw message