osdir.com


[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Bug in mod_ratelimit?


Hi Luca,

We’ve tested this in house and it does seem to resolve the issues from the previous patch. Looks good to us. :) 

Thanks,
Cory

> On Aug 27, 2018, at 8:50 AM, Cory McIntire <cory@xxxxxxxxxx> wrote:
> 
> Hello Luca,
> 
> Sorry for late reply, we’re digging into and testing this version of the patch now, will update this week
> with more info, preliminary results seem to be positive however. 
> 
> Thanks,
> Cory McIntire
> Release Manager - EasyApache
> cPanel, Inc.
> 
>> On Aug 23, 2018, at 11:25 AM, Luca Toscano <toscano.luca@xxxxxxxxx> wrote:
>> 
>> Hi Cory,
>> 
>> Il giorno gio 2 ago 2018 alle ore 14:10 Yann Ylavic
>> <ylavic.dev@xxxxxxxxx> ha scritto:
>>> 
>>> On Fri, Jul 27, 2018 at 6:56 PM, Cory McIntire <cory@xxxxxxxxxx> wrote:
>>>> 
>>>> {quote}
>>>> While it will probably resolve the issues we saw, I’d be hesitant to
>>>> move forward with that patch as it modifies how all output filters
>>>> work with HEAD requests, this is too large a change, especially when
>>>> the bug(s) being addressesed are in a single module.
>>>> 
>>>> I’d recommend making mod_ratelimit do the same “optimization” hack
>>>> that other modules for HEAD requests instead, and keep the surface
>>>> area for this bug fix isolated to mod_ratelimit only.
>>>> 
>>>> Something like what mod_brotli does:
>>>> 
>>>>        if (r->header_only && r->bytes_sent) {
>>>>            ap_remove_output_filter(f);
>>>>            return ap_pass_brigade(f->next, bb);
>>>>        }
>>>> {quote}
>>>> 
>>>> If there are any further adjustments to this patch we’d be happy to
>>>> take a look, just let us know.
>>> 
>>> Sorry, I missed that proposal.
>>> 
>>> So, AIUI, the above plus moving the ratelimit filter after the "CHUNK"
>>> filter, right?
>>> Otherwise I don't see where we address the "header
>>> ratelimited/retained before chunking" case (regardless of
>>> HEAD/GET/..).
>>> IOW, the patch (limited to mod_ratelimit) would be something like:
>>> 
>>> @@ -123,6 +123,13 @@ rate_limit_filter(ap_filter_t *f, apr_bucket_briga
>>>        APR_BRIGADE_PREPEND(bb, ctx->holdingbb);
>>>    }
>>> 
>>> +    if (f->r->header_only && f->r->bytes_sent) {
>>> +        ap_remove_output_filter(f);
>>> +        rv = ap_pass_brigade(f->next, bb);
>>> +        apr_brigade_cleanup(bb);
>>> +        return rv;
>>> +    }
>>> +
>>>    while (!APR_BRIGADE_EMPTY(bb)) {
>>>        apr_bucket *e;
>>> 
>>> @@ -327,7 +334,7 @@ static void register_hooks(apr_pool_t *p)
>>>    ap_register_output_filter(RATE_LIMIT_FILTER_NAME, rate_limit_filter,
>>> -                              NULL, AP_FTYPE_PROTOCOL + 3);
>>> +                              NULL, AP_FTYPE_CONNECTION - 1);
>>> 
>>> I think it does not work for the case where a HEAD response has a
>>> large header but "Content-Length: 0", such that rate_limit_filter()
>>> retains the last buckets but is never called to release them (i.e.
>>> EOS).
>>> Really we shouldn't have any (protocol) filter that eats EOS, this is
>>> the garantee that each request filter sees when it should terminate
>>> and bail out (that's also the purpose of
>>> ap_finalize_request_protocol() for instance).
>>> 
>>> r1837130 looks like the right fix to me.
>> 
>> sorry for the late ping but I was on holidays. I know that you and
>> your team expressed some doubts about the fix for mod_ratelimit, but
>> it seems that Yann's fix is the correct way to go for me too. Any
>> thoughts? It would be great to reach some consensus within the
>> community before proceeding :)
>> 
>> Thanks!
>> 
>> Luca
> 

Attachment: smime.p7s
Description: S/MIME cryptographic signature