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

Re: Bug in mod_ratelimit?

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