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

Re: Bug in mod_ratelimit?

Hi Yann,

2018-07-19 21:41 GMT+02:00 Yann Ylavic <ylavic.dev@xxxxxxxxx>:
On Thu, Jul 19, 2018 at 8:23 PM, Luca Toscano <toscano.luca@xxxxxxxxx> 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
                 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) */

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 :(