Re: Bug in mod_ratelimit?

[Yann Ylavic]

On Tue, Aug 28, 2018 at 4:21 PM Cory McIntire  wrote:
>
> We’ve tested this in house and it does seem to resolve the issues from the 
> previous patch. Looks good to us. :)

Thanks Cory for the tests, it has been merge in 2.4.x and will be
available in next 2.4 version.

Regards,
Yann.

Re: Bug in mod_ratelimit?

[Cory McIntire]

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  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  wrote:
>> 
>> Hi Cory,
>> 
>> Il giorno gio 2 ago 2018 alle ore 14:10 Yann Ylavic
>>  ha scritto:
>>> 
>>> On Fri, Jul 27, 2018 at 6:56 PM, Cory McIntire  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
> 



smime.p7s
Description: S/MIME cryptographic signature

Re: Bug in mod_ratelimit?

[Cory McIntire]

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  wrote:
> 
> Hi Cory,
> 
> Il giorno gio 2 ago 2018 alle ore 14:10 Yann Ylavic
>  ha scritto:
>> 
>> On Fri, Jul 27, 2018 at 6:56 PM, Cory McIntire  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



smime.p7s
Description: S/MIME cryptographic signature

Re: Bug in mod_ratelimit?

[Luca Toscano]

Hi Cory,

Il giorno gio 2 ago 2018 alle ore 14:10 Yann Ylavic
 ha scritto:
>
> On Fri, Jul 27, 2018 at 6:56 PM, Cory McIntire  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

Re: Bug in mod_ratelimit?

[Yann Ylavic]

On Fri, Jul 27, 2018 at 6:56 PM, Cory McIntire  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.

Re: Bug in mod_ratelimit?

[William A Rowe Jr]

I'd concur that this suggested change is lighter weight and less fragile.


On Fri, Jul 27, 2018, 12:56 Cory McIntire  wrote:

> Hi Luca,
>
> Sorry for the delay in response.. we did look into it further..
>
> On of our devs had been looking into it and came up with the following:
>
> {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.
>
> Thanks,
> Cory McIntire
> Release Manager - EasyApache
> cPanel, Inc.
>
>
> > On Jul 27, 2018, at 10:46 AM, Luca Toscano 
> wrote:
> >
> > Hi Cory,
> >
> > 2018-07-20 13:47 GMT+02:00 Yann Ylavic :
> >> Hi Cory,
> >>
> >> On Thu, Jul 19, 2018 at 11:23 PM, Cory McIntire 
> wrote:
> >>>
> >>> We’re going to revert to the 2.4.33 version of mod_ratelimit for now.
> >>>
> >>> HEAD requests with large amount of headers were still problematic in
> our testing with both versions of the patch applied.
> >>
> >> Thanks for letting us know.
> >>
> >> I think the right fix is the attached patch (tested with GET/HEAD with
> >> large header and/or body, seems to work).
> >> If by any chance you can give it a try...
> >
> > In the meantime, other people are testing Yann's last patch in
> > https://bz.apache.org/bugzilla/show_bug.cgi?id=62568 (it is attached
> > in there). If you could chime in whenever you have time and let us
> > know your thoughts it would be really great.
> >
> > Thanks in advance!
> >
> > Luca
>
>
>
>

Re: Bug in mod_ratelimit?

[Cory McIntire]

Hi Luca,

Sorry for the delay in response.. we did look into it further.. 

On of our devs had been looking into it and came up with the following:

{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.

Thanks,
Cory McIntire
Release Manager - EasyApache 
cPanel, Inc.


> On Jul 27, 2018, at 10:46 AM, Luca Toscano  wrote:
> 
> Hi Cory,
> 
> 2018-07-20 13:47 GMT+02:00 Yann Ylavic :
>> Hi Cory,
>> 
>> On Thu, Jul 19, 2018 at 11:23 PM, Cory McIntire  wrote:
>>> 
>>> We’re going to revert to the 2.4.33 version of mod_ratelimit for now.
>>> 
>>> HEAD requests with large amount of headers were still problematic in our 
>>> testing with both versions of the patch applied.
>> 
>> Thanks for letting us know.
>> 
>> I think the right fix is the attached patch (tested with GET/HEAD with
>> large header and/or body, seems to work).
>> If by any chance you can give it a try...
> 
> In the meantime, other people are testing Yann's last patch in
> https://bz.apache.org/bugzilla/show_bug.cgi?id=62568 (it is attached
> in there). If you could chime in whenever you have time and let us
> know your thoughts it would be really great.
> 
> Thanks in advance!
> 
> Luca





smime.p7s
Description: S/MIME cryptographic signature

Re: Bug in mod_ratelimit?

[Luca Toscano]

Hi Cory,

2018-07-20 13:47 GMT+02:00 Yann Ylavic :
> Hi Cory,
>
> On Thu, Jul 19, 2018 at 11:23 PM, Cory McIntire  wrote:
>>
>> We’re going to revert to the 2.4.33 version of mod_ratelimit for now.
>>
>> HEAD requests with large amount of headers were still problematic in our 
>> testing with both versions of the patch applied.
>
> Thanks for letting us know.
>
> I think the right fix is the attached patch (tested with GET/HEAD with
> large header and/or body, seems to work).
> If by any chance you can give it a try...

In the meantime, other people are testing Yann's last patch in
https://bz.apache.org/bugzilla/show_bug.cgi?id=62568 (it is attached
in there). If you could chime in whenever you have time and let us
know your thoughts it would be really great.

Thanks in advance!

Luca

Re: Bug in mod_ratelimit?

[Yann Ylavic]

On Mon, Jul 23, 2018 at 7:45 AM, Plüm, Rüdiger, Vodafone Group
 wrote:
>
>
>> -Ursprüngliche Nachricht-
>> Von: Eric Covener 
>> Gesendet: Sonntag, 22. Juli 2018 21:58
>> An: Apache HTTP Server Development List 
>> Betreff: Re: Bug in mod_ratelimit?
>>
>> > > You probably didn't test with chunked encoding (neither did I!), see
>> > > how ap_http_header_filter() adds the "CHUNK" filter after the
>> headers
>> > > have been sent, so if anything retains the headers they will be
>> > > considered as the body by the (late) ap_http_chunk_filter()..
>>
>> would it be reasonable to send a flush down, and/or track whether the
>> headers are flushed somewhere out of the http filter ctx?
>
> Flush in case of mod_ratelimit or in general?

mod_ratelimit FLUSHes data already, and the first patch proposed here
allowed the headers to pass with a FLUSH.
Yet if headers are large enough to be rate limited, the first ones
will pass before the "CHUNK" filter is added, but not the next ones.

> In general wouldn't
> that cause "smaller" TCP packets to be sent if content is ready and
> the headers are "short" and hence cause some performance
> degradation?

In the general case I agree that we shouldn't always flush the
headers, moreover mod_ratelimit isn't supposed to (and doesn't) honor
FLUSHes. The rate is applied regardless of FLUSH buckets from previous
filters.

One way to possibly address this would be to handle a new EOH (End Of
Header) meta-bucket, à la EOS/EOR/..
ap_http_header_filter() would add the "CHUNK" filter first and then
pass the headers brigade ended by EOH.
The "CHUNK" filter would do nothing until EOH.

I think my patch "rate_limit_filter+header_only.patch" is needed
anyway w.r.t. header_only handling in ap_http_header_filter() and EOS.


Regards,
Yann.

Re: Bug in mod_ratelimit?

[Eric Covener]

> > You probably didn't test with chunked encoding (neither did I!), see
> > how ap_http_header_filter() adds the "CHUNK" filter after the headers
> > have been sent, so if anything retains the headers they will be
> > considered as the body by the (late) ap_http_chunk_filter()..

would it be reasonable to send a flush down, and/or track whether the
headers are flushed somewhere out of the http filter ctx?

Re: Bug in mod_ratelimit?

[Luca Toscano]

Hi Yann,

2018-07-19 22:55 GMT+02:00 Yann Ylavic :
>
> Hi Luca,
>
> On Thu, Jul 19, 2018 at 9:59 PM, Luca Toscano  wrote:
> >
> > I confirm that it works fine in my local dev environment, plus now gdb's
> > dump_brigade makes sense, but I am not sure that I have understood what was
> > wrong. In my tests, the headers were the first heap bucket reported by
> > dump_brigade in gdb (before your patch), so IIUC we were saving them to
> > holdingbb and then finally passing them via ap_pass_brigade. But I thought
> > that this was they way to go, what am I missing? I am pretty sure this is
> > basic filter knowledge but I have missed it up to now, if not present in the
> > docs already I'll make sure to add it.
>
> You probably didn't test with chunked encoding (neither did I!), see
> how ap_http_header_filter() adds the "CHUNK" filter after the headers
> have been sent, so if anything retains the headers they will be
> considered as the body by the (late) ap_http_chunk_filter()..


Thanks a lot for the explanation, it took me a while to understand
what you meant but I think I kinda got it. You are saying that
ap_http_header_filter specifically passes the brigade containing the
headers to the next filters in the chain, hoping that it would reach
the client before any attempt to insert the chunk filter is made. But
in this case, the rate_limit_filter interferes with
ap_http_header_filter's assumptions, and buffers the brigade
containing the headers (that never reaches the external client). At
this point, the chunk filter is added by ap_http_header_filter, and
during the next ap_pass_brigade call the buffered data is passed by
rate_limit_filter to the chain, eventually reaching the chunk filter,
that will interpret it as body (making the mess that Cory reported).

Is my understanding vaguely resembling what happens? If so I think it
would be an interesting use case to document in
https://httpd.apache.org/docs/trunk/developer/output-filters.html :)

> @team: the issue with HEAD request is that ap_http_header_filter()
> eats the final EOS (along with the body), I don't think this is
> correct because downstream filters may depend on it to bail out (like
> rate_limit_filter() does now).
> So how about the attached patch both with regard to level
> FTYPE_CONNECTION - 1 for ratelimit filter and ap_http_header_filter()
> to forward the final EOS?

It looks good to me, but I'd really like to get others opinion on how
to fix this problem. What do others think about it?

Thanks a lot for this fix!

Luca

Re: Bug in mod_ratelimit?

[Yann Ylavic]

Hi Cory,

On Thu, Jul 19, 2018 at 11:23 PM, Cory McIntire  wrote:
>
> We’re going to revert to the 2.4.33 version of mod_ratelimit for now.
>
> HEAD requests with large amount of headers were still problematic in our 
> testing with both versions of the patch applied.

Thanks for letting us know.

I think the right fix is the attached patch (tested with GET/HEAD with
large header and/or body, seems to work).
If by any chance you can give it a try...

@team: the issue with HEAD request is that ap_http_header_filter()
eats the final EOS (along with the body), I don't think this is
correct because downstream filters may depend on it to bail out (like
rate_limit_filter() does now).
So how about the attached patch both with regard to level
FTYPE_CONNECTION - 1 for ratelimit filter and ap_http_header_filter()
to forward the final EOS?

Regards,
Yann.
Index: modules/filters/mod_ratelimit.c
===
--- modules/filters/mod_ratelimit.c	(revision 1836337)
+++ modules/filters/mod_ratelimit.c	(working copy)
@@ -327,7 +327,7 @@ static void register_hooks(apr_pool_t *p)
 {
 /* run after mod_deflate etc etc, but not at connection level, ie, mod_ssl. */
 ap_register_output_filter(RATE_LIMIT_FILTER_NAME, rate_limit_filter,
-  NULL, AP_FTYPE_PROTOCOL + 3);
+  NULL, AP_FTYPE_CONNECTION - 1);
 }
 
 AP_DECLARE_MODULE(ratelimit) = {
Index: modules/http/http_filters.c
===
--- modules/http/http_filters.c	(revision 1836337)
+++ modules/http/http_filters.c	(working copy)
@@ -1308,8 +1308,19 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_http_heade
 else if (ctx->headers_sent) {
 /* Eat body if response must not have one. */
 if (r->header_only || r->status == HTTP_NO_CONTENT) {
+/* Still next filters may be waiting for EOS, so pass it (alone)
+ * when encountered and be done with this filter.
+ */
+e = APR_BRIGADE_LAST(b);
+if (e != APR_BRIGADE_SENTINEL(b) && APR_BUCKET_IS_EOS(e)) {
+APR_BUCKET_REMOVE(e);
+apr_brigade_cleanup(b);
+APR_BRIGADE_INSERT_HEAD(b, e);
+ap_remove_output_filter(f);
+rv = ap_pass_brigade(f->next, b);
+}
 apr_brigade_cleanup(b);
-return APR_SUCCESS;
+return rv;
 }
 }
 

Re: Bug in mod_ratelimit?

[Cory McIntire]

Hi all,

We’re going to revert to the 2.4.33 version of mod_ratelimit for now.

HEAD requests with large amount of headers were still problematic in our 
testing with both versions of the patch applied.

Thanks,
Cory McIntire
Release Manager - EasyApache 
cPanel, Inc.


> On Jul 19, 2018, at 3:36 PM, Yann Ylavic  wrote:
> 
> On Thu, Jul 19, 2018 at 10:30 PM, Yann Ylavic  wrote:
>> Hi,
>> 
>> On Thu, Jul 19, 2018 at 10:16 PM, Cory McIntire  wrote:
>>> 
>>> Upon some initial testing of the patch we have found some conditions to 
>>> which this will still break, consider the following:
>>> 
>>> Put something like this into your php file,
>>> 
>>>for ($i = 1; $i <= 2000; $i++) {
>>>header("x$i: $i");
>>>}
>> 
>> Yes I was thinking about this, currently mod_ratelimit is not able to
>> ratelimit headers when chunked encoding is to be used for the body.
>> 
>> This is because the http (header) filter assumes nothing retains the
>> headers in between itself and the chunked filter (which itself assumes
>> everything it receives is the body).
>> 
>> I'm looking at the best way to address this, possibly mod_ratelimit's
>> filter should be moved after the "CHUNK" filter (i.e.
>> AP_FTYPE_TRANSCODE)? The requirement seems to be after deflate but
>> before network filter...
> 
> The patch would be something like this (instead of the previous one):
> 
> Index: modules/filters/mod_ratelimit.c
> ===
> --- modules/filters/mod_ratelimit.c(revision 1835556)
> +++ modules/filters/mod_ratelimit.c(working copy)
> @@ -327,7 +327,7 @@ static void register_hooks(apr_pool_t *p)
> {
> /* run after mod_deflate etc etc, but not at connection level,
> ie, mod_ssl. */
> ap_register_output_filter(RATE_LIMIT_FILTER_NAME, rate_limit_filter,
> -  NULL, AP_FTYPE_PROTOCOL + 3);
> +  NULL, AP_FTYPE_TRANSCODE + 1);
> }
> 
> --
> 
> This seems to work for me too...



smime.p7s
Description: S/MIME cryptographic signature

Re: Bug in mod_ratelimit?

[Yann Ylavic]

Hi Luca,

On Thu, Jul 19, 2018 at 9:59 PM, Luca Toscano  wrote:
>
> I confirm that it works fine in my local dev environment, plus now gdb's
> dump_brigade makes sense, but I am not sure that I have understood what was
> wrong. In my tests, the headers were the first heap bucket reported by
> dump_brigade in gdb (before your patch), so IIUC we were saving them to
> holdingbb and then finally passing them via ap_pass_brigade. But I thought
> that this was they way to go, what am I missing? I am pretty sure this is
> basic filter knowledge but I have missed it up to now, if not present in the
> docs already I'll make sure to add it.

You probably didn't test with chunked encoding (neither did I!), see
how ap_http_header_filter() adds the "CHUNK" filter after the headers
have been sent, so if anything retains the headers they will be
considered as the body by the (late) ap_http_chunk_filter()...

Regards,
Yann.

Re: Bug in mod_ratelimit?

[Yann Ylavic]

On Thu, Jul 19, 2018 at 10:30 PM, Yann Ylavic  wrote:
> Hi,
>
> On Thu, Jul 19, 2018 at 10:16 PM, Cory McIntire  wrote:
>>
>> Upon some initial testing of the patch we have found some conditions to 
>> which this will still break, consider the following:
>>
>> Put something like this into your php file,
>>
>> for ($i = 1; $i <= 2000; $i++) {
>> header("x$i: $i");
>> }
>
> Yes I was thinking about this, currently mod_ratelimit is not able to
> ratelimit headers when chunked encoding is to be used for the body.
>
> This is because the http (header) filter assumes nothing retains the
> headers in between itself and the chunked filter (which itself assumes
> everything it receives is the body).
>
> I'm looking at the best way to address this, possibly mod_ratelimit's
> filter should be moved after the "CHUNK" filter (i.e.
> AP_FTYPE_TRANSCODE)? The requirement seems to be after deflate but
> before network filter...

The patch would be something like this (instead of the previous one):

Index: modules/filters/mod_ratelimit.c
===
--- modules/filters/mod_ratelimit.c(revision 1835556)
+++ modules/filters/mod_ratelimit.c(working copy)
@@ -327,7 +327,7 @@ static void register_hooks(apr_pool_t *p)
 {
 /* run after mod_deflate etc etc, but not at connection level,
ie, mod_ssl. */
 ap_register_output_filter(RATE_LIMIT_FILTER_NAME, rate_limit_filter,
-  NULL, AP_FTYPE_PROTOCOL + 3);
+  NULL, AP_FTYPE_TRANSCODE + 1);
 }

--

This seems to work for me too...

Re: Bug in mod_ratelimit?

[Yann Ylavic]

Hi,

On Thu, Jul 19, 2018 at 10:16 PM, Cory McIntire  wrote:
>
> Upon some initial testing of the patch we have found some conditions to which 
> this will still break, consider the following:
>
> Put something like this into your php file,
>
> for ($i = 1; $i <= 2000; $i++) {
> header("x$i: $i");
> }

Yes I was thinking about this, currently mod_ratelimit is not able to
ratelimit headers when chunked encoding is to be used for the body.

This is because the http (header) filter assumes nothing retains the
headers in between itself and the chunked filter (which itself assumes
everything it receives is the body).

I'm looking at the best way to address this, possibly mod_ratelimit's
filter should be moved after the "CHUNK" filter (i.e.
AP_FTYPE_TRANSCODE)? The requirement seems to be after deflate but
before network filter...

Regards,
Yann.

Re: Bug in mod_ratelimit?

[Cory McIntire]

Hello,

Upon some initial testing of the patch we have found some conditions to which 
this will still break, consider the following:

Put something like this into your php file,

for ($i = 1; $i <= 2000; $i++) {
header("x$i: $i");
}

Set your rate limit pretty low and it will cause the headers to be larger than 
the chunk size, 
and you will see an error with those responses such as this:

curl -H'Host: cptestaddon.com' http://10.215.218.12/
curl: (56) Illegal or missing hexadecimal sequence in chunked-encoding

which of course means the page doesn’t load.

Real world how often is it set that low is unknown but thought we’d share our 
findings.

Cory

> On Jul 19, 2018, at 2:53 PM, Cory McIntire  wrote:
> 
> Hello Yann,
> 
> We can confirm this patch works on our end. We’ll apply this and send out an 
> update. 
> 
>> On Jul 19, 2018, at 2:41 PM, Yann Ylavic  wrote:
>> 
>> On Thu, Jul 19, 2018 at 8:23 PM, Luca Toscano  wrote:
>>> 
>>> Yann, any idea?
>> 
>> Looks like we missed the simplest case :/
>> 
>> Index: modules/filters/mod_ratelimit.c
>> ===
>> --- modules/filters/mod_ratelimit.c(revision 1835556)
>> +++ modules/filters/mod_ratelimit.c(working copy)
>> @@ -208,7 +208,7 @@ rate_limit_filter(ap_filter_t *f, apr_bucket_briga
>>ap_remove_output_filter(f);
>>}
>>else if (!APR_BUCKET_IS_FLUSH(e)) {
>> -if (APR_BRIGADE_EMPTY(bb)) {
>> +if (ctx->do_sleep && APR_BRIGADE_EMPTY(bb)) {
>>/* Wait for more (or next call) */
>>break;
>>}
>> _
> 
> Much appreciated,
> Cory
> 



smime.p7s
Description: S/MIME cryptographic signature

Re: Bug in mod_ratelimit?

[Luca Toscano]

Hi Yann,

2018-07-19 21:41 GMT+02:00 Yann Ylavic :

> On Thu, Jul 19, 2018 at 8:23 PM, Luca Toscano 
> wrote:
> >
> > Yann, any idea?
>
> Looks like we missed the simplest case :/
>
> Index: modules/filters/mod_ratelimit.c
> ===
> --- modules/filters/mod_ratelimit.c(revision 1835556)
> +++ modules/filters/mod_ratelimit.c(working copy)
> @@ -208,7 +208,7 @@ rate_limit_filter(ap_filter_t *f, apr_bucket_briga
>  ap_remove_output_filter(f);
>  }
>  else if (!APR_BUCKET_IS_FLUSH(e)) {
> -if (APR_BRIGADE_EMPTY(bb)) {
> +if (ctx->do_sleep && APR_BRIGADE_EMPTY(bb)) {
>  /* Wait for more (or next call) */
>  break;
>  }
> _
>

I confirm that it works fine in my local dev environment, plus now gdb's
dump_brigade makes sense, but I am not sure that I have understood what was
wrong. In my tests, the headers were the first heap bucket reported by
dump_brigade in gdb (before your patch), so IIUC we were saving them to
holdingbb and then finally passing them via ap_pass_brigade. But I thought
that this was they way to go, what am I missing? I am pretty sure this is
basic filter knowledge but I have missed it up to now, if not present in
the docs already I'll make sure to add it.

Other mea culpa: I tested this change with a proxied scenario in which a
file was transferred from a proxied backed to the client via httpd, and I
didn't notice any problem. I also thought that the tests suite would have
caught a case like this one, but I was terribly wrong, so if possible I'll
try to add a test as follow up to make sure that basic proxied responses
are not mangled like this.

Really sorry for this mess, I should have been more careful before
committing. Lesson learned for the next time :(

Luca

Re: Bug in mod_ratelimit?

[Eric Covener]

On Thu, Jul 19, 2018 at 3:41 PM Yann Ylavic  wrote:
>
> On Thu, Jul 19, 2018 at 8:23 PM, Luca Toscano  wrote:
> >
> > Yann, any idea?
>
> Looks like we missed the simplest case :/
>
> Index: modules/filters/mod_ratelimit.c
> ===
> --- modules/filters/mod_ratelimit.c(revision 1835556)
> +++ modules/filters/mod_ratelimit.c(working copy)
> @@ -208,7 +208,7 @@ rate_limit_filter(ap_filter_t *f, apr_bucket_briga
>  ap_remove_output_filter(f);
>  }
>  else if (!APR_BUCKET_IS_FLUSH(e)) {
> -if (APR_BRIGADE_EMPTY(bb)) {
> +if (ctx->do_sleep && APR_BRIGADE_EMPTY(bb)) {
>  /* Wait for more (or next call) */
>  break;
>  }
> _

that fixes my symptom with the headers being eaten into the first chunk

-- 
Eric Covener
cove...@gmail.com

Re: Bug in mod_ratelimit?

[Cory McIntire]

Hello Yann,

We can confirm this patch works on our end. We’ll apply this and send out an 
update. 

> On Jul 19, 2018, at 2:41 PM, Yann Ylavic  wrote:
> 
> On Thu, Jul 19, 2018 at 8:23 PM, Luca Toscano  wrote:
>> 
>> Yann, any idea?
> 
> Looks like we missed the simplest case :/
> 
> Index: modules/filters/mod_ratelimit.c
> ===
> --- modules/filters/mod_ratelimit.c(revision 1835556)
> +++ modules/filters/mod_ratelimit.c(working copy)
> @@ -208,7 +208,7 @@ rate_limit_filter(ap_filter_t *f, apr_bucket_briga
> ap_remove_output_filter(f);
> }
> else if (!APR_BUCKET_IS_FLUSH(e)) {
> -if (APR_BRIGADE_EMPTY(bb)) {
> +if (ctx->do_sleep && APR_BRIGADE_EMPTY(bb)) {
> /* Wait for more (or next call) */
> break;
> }
> _

Much appreciated,
Cory



smime.p7s
Description: S/MIME cryptographic signature

Re: Bug in mod_ratelimit?

[Yann Ylavic]

On Thu, Jul 19, 2018 at 8:23 PM, Luca Toscano  wrote:
>
> Yann, any idea?

Looks like we missed the simplest case :/

Index: modules/filters/mod_ratelimit.c
===
--- modules/filters/mod_ratelimit.c(revision 1835556)
+++ modules/filters/mod_ratelimit.c(working copy)
@@ -208,7 +208,7 @@ rate_limit_filter(ap_filter_t *f, apr_bucket_briga
 ap_remove_output_filter(f);
 }
 else if (!APR_BUCKET_IS_FLUSH(e)) {
-if (APR_BRIGADE_EMPTY(bb)) {
+if (ctx->do_sleep && APR_BRIGADE_EMPTY(bb)) {
 /* Wait for more (or next call) */
 break;
 }
_

Re: Bug in mod_ratelimit?

[Eric Covener]

On Thu, Jul 19, 2018 at 2:23 PM Luca Toscano  wrote:
>
> Hi again Cory,
>
> 2018-07-19 19:02 GMT+02:00 Cory McIntire :
>>
>> Hi Luca,
>>
>> Sorry for quick reply but we were able to replicate it just now:
>>
>> # setup a brand new install of wp on a domain (don't have to go through the 
>> 'db' setup process, just configure wp-config.php to get to install.php 
>> redirect)
>> # install mod_ratelimit, and setup a vhost.conf with the ratelimit config 
>> for the domain
>> # restart apache
>> # visit site, see you are getting the "redirect" content instead of actually 
>> being redirected:
>>
>> •  curl -H'Host: cptestaddon.com' http://10.215.218.12/
>> • HTTP/1.1 302 Moved Temporarily
>> • Date: Thu, 19 Jul 2018 16:47:07 GMT
>> • Server: Apache
>> • X-Powered-By: PHP/5.6.36
>> • Expires: Wed, 11 Jan 1984 05:00:00 GMT
>> • Cache-Control: no-cache, must-revalidate, max-age=0
>> • Pragma: no-cache
>> • Location: http://cptestaddon.com/wp-admin/install.php
>> • Transfer-Encoding: chunked
>> • Content-Type: text/html; charset=UTF-8
>> • 0
>>
>> It is any CGI app but WP was an easy target to replicate on.
>>
>
> I can see the same thing with a simple php script that says "this is a test" 
> on my testing environment:
>
> vagrant@stretch:~$ curl -k https://localhost/test.php
> HTTP/1.1 200 OK
> Date: Thu, 19 Jul 2018 18:15:09 GMT
> Server: Apache/2.4.34-dev (Unix) OpenSSL/1.1.0f
> Transfer-Encoding: chunked
> Content-Type: text/html; charset=UTF-8
>
> this is a test!
> 0
>
> (Note the zero at the end)
>

You think that's weird, I see a chunk length before the status line
when using a raw socket:

$ printf "GET /cgi-bin/test-cgi HTTP/1.1\r\nHost: foo\r\n\r\n" | nc 0 80
23a
HTTP/1.1 200 OK
Date: Thu, 19 Jul 2018 19:12:05 GMT
Server: Apache/2.4.34-dev (Unix) OpenSSL/1.1.0g
Transfer-Encoding: chunked
Content-Type: text/plain

CGI/1.0 test script report:

argc is 0. argv is .

SERVER_SOFTWARE = Apache/2.4.34-dev (Unix) OpenSSL/1.1.0g
SERVER_NAME = foo
GATEWAY_INTERFACE = CGI/1.1
SERVER_PROTOCOL = HTTP/1.1
SERVER_PORT = 80
REQUEST_METHOD = GET
HTTP_ACCEPT =
PATH_INFO =
PATH_TRANSLATED =
SCRIPT_NAME = /cgi-bin/test-cgi
QUERY_STRING =
REMOTE_HOST =
REMOTE_ADDR = 127.0.0.1
REMOTE_USER =
AUTH_TYPE =
CONTENT_TYPE =
CONTENT_LENGTH =

0


> So this is a bug introduced by the latest patch for sure, but I still have no 
> idea where it comes from. I apologize for this issue, I was convinced that 
> the new code was tested but apparently I missed the most basic use cases.
>
> Yann, any idea?
>
> Luca



-- 
Eric Covener
cove...@gmail.com

Re: Bug in mod_ratelimit?

[Luca Toscano]

Hi again Cory,

2018-07-19 19:02 GMT+02:00 Cory McIntire :

> Hi Luca,
>
> Sorry for quick reply but we were able to replicate it just now:
>
> # setup a brand new install of wp on a domain (don't have to go through
> the 'db' setup process, just configure wp-config.php to get to install.php
> redirect)
> # install mod_ratelimit, and setup a vhost.conf with the ratelimit config
> for the domain
> # restart apache
> # visit site, see you are getting the "redirect" content instead of
> actually being redirected:
>
> •  curl -H'Host: cptestaddon.com' http://10.215.218.12/
> • HTTP/1.1 302 Moved Temporarily
> • Date: Thu, 19 Jul 2018 16:47:07 GMT
> • Server: Apache
> • X-Powered-By: PHP/5.6.36
> • Expires: Wed, 11 Jan 1984 05:00:00 GMT
> • Cache-Control: no-cache, must-revalidate, max-age=0
> • Pragma: no-cache
> • Location: http://cptestaddon.com/wp-admin/install.php
> • Transfer-Encoding: chunked
> • Content-Type: text/html; charset=UTF-8
> • 0
>
> It is any CGI app but WP was an easy target to replicate on.
>
>
I can see the same thing with a simple php script that says "this is a
test" on my testing environment:

vagrant@stretch:~$ curl -k https://localhost/test.php
HTTP/1.1 200 OK
Date: Thu, 19 Jul 2018 18:15:09 GMT
Server: Apache/2.4.34-dev (Unix) OpenSSL/1.1.0f
Transfer-Encoding: chunked
Content-Type: text/html; charset=UTF-8

this is a test!
0

(Note the zero at the end)

So this is a bug introduced by the latest patch for sure, but I still have
no idea where it comes from. I apologize for this issue, I was convinced
that the new code was tested but apparently I missed the most basic use
cases.

Yann, any idea?

Luca

Re: Bug in mod_ratelimit?

[Cory McIntire]

Hi Luca,

Sorry for quick reply but we were able to replicate it just now:

# setup a brand new install of wp on a domain (don't have to go through the 
'db' setup process, just configure wp-config.php to get to install.php redirect)
# install mod_ratelimit, and setup a vhost.conf with the ratelimit config for 
the domain
# restart apache
# visit site, see you are getting the "redirect" content instead of actually 
being redirected:

•  curl -H'Host: cptestaddon.com' http://10.215.218.12/
• HTTP/1.1 302 Moved Temporarily
• Date: Thu, 19 Jul 2018 16:47:07 GMT
• Server: Apache
• X-Powered-By: PHP/5.6.36
• Expires: Wed, 11 Jan 1984 05:00:00 GMT
• Cache-Control: no-cache, must-revalidate, max-age=0
• Pragma: no-cache
• Location: http://cptestaddon.com/wp-admin/install.php
• Transfer-Encoding: chunked
• Content-Type: text/html; charset=UTF-8
• 0

It is any CGI app but WP was an easy target to replicate on. 

If you confirm I will create a bug report for it, basically mod_ratelimit 
causes CGI-style apps to emit plaintext. 

Thanks,
Cory McIntire
Release Manager - EasyApache 
cPanel, Inc.

> On Jul 19, 2018, at 10:32 AM, Luca Toscano  wrote:
> 
> Hi Cory,
> 
> 2018-07-19 16:10 GMT+02:00 Cory McIntire :
> Hello all,
> 
> We’re starting to see some issues where mod_ratelimit change here:
> 
>   *) mod_ratelimit: fix behavior when proxing content. PR 62362.
>  [Luca Toscano, Yann Ylavic]
> 
> Is causing some sites to load in plain text/source code…
> 
> We haven’t found the connection beyond unloading mod_ratelimit which resolves 
> the issue,
>  and its not happening everywhere, just curious if anyone else is seeing this?
> 
> I’ll report back once I have more info on further factors involved. 
> 
> Thanks a lot for reporting this. Can you add a bit more info about how to 
> reproduce (httpd config I mean)? Anything relevant in the error logs?
> 
> Luca 
> 



smime.p7s
Description: S/MIME cryptographic signature

Re: Bug in mod_ratelimit?

[Cory McIntire]

Hi Luca,

So far we’ve not seen much in the logs of our customer reports, however I was
able to get the following settings:



SetOutputFilter RATE_LIMIT
SetEnv rate-limit 512
SetEnv rate-initial-burst 625



When removed/commented out and/or removing mod_ratelimit the site would begin 
to work again.

When in a broken state we would see things like the following when visiting the 
page:

HTTP/1.1 200 OK
Date: Thu, 19 Jul 2018 13:33:05 GMT
Server: Apache
X-Frame-Options: SAMEORIGIN
X-Pingback: 
http:///xmlrpc.php

Link: <
http://X/wp-json/>; rel="https://api.w.org/";, ;
 rel=shortlink
Keep-Alive: timeout=5, max=100
Connection: Keep-Alive
Transfer-Encoding: chunked
Content-Type: text/html; charset=UTF-8
….
….
 

We are working internally to attempt to get more of these answers.. having 
trouble so far reproducing in house.

Sorry I don’t have more info but I will reply again if we can get it 
reproducible. 

Thanks for your quick response! 
Cory McIntire
Release Manager - EasyApache 
cPanel, Inc.

 
> On Jul 19, 2018, at 10:32 AM, Luca Toscano  wrote:
> 
> Hi Cory,
> 
> 2018-07-19 16:10 GMT+02:00 Cory McIntire :
> Hello all,
> 
> We’re starting to see some issues where mod_ratelimit change here:
> 
>   *) mod_ratelimit: fix behavior when proxing content. PR 62362.
>  [Luca Toscano, Yann Ylavic]
> 
> Is causing some sites to load in plain text/source code…
> 
> We haven’t found the connection beyond unloading mod_ratelimit which resolves 
> the issue,
>  and its not happening everywhere, just curious if anyone else is seeing this?
> 
> I’ll report back once I have more info on further factors involved. 
> 
> Thanks a lot for reporting this. Can you add a bit more info about how to 
> reproduce (httpd config I mean)? Anything relevant in the error logs?
> 
> Luca 
> 



smime.p7s
Description: S/MIME cryptographic signature

Re: Bug in mod_ratelimit?

[Luca Toscano]

Hi Cory,

2018-07-19 16:10 GMT+02:00 Cory McIntire :

> Hello all,
>
> We’re starting to see some issues where mod_ratelimit change here:
>
>   *) mod_ratelimit: fix behavior when proxing content. PR 62362.
>  [Luca Toscano, Yann Ylavic]
>
> Is causing some sites to load in plain text/source code…
>
> We haven’t found the connection beyond unloading mod_ratelimit which
> resolves the issue,
>  and its not happening everywhere, just curious if anyone else is seeing
> this?
>
> I’ll report back once I have more info on further factors involved.
>

Thanks a lot for reporting this. Can you add a bit more info about how to
reproduce (httpd config I mean)? Anything relevant in the error logs?

Luca