OSDir


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

Re: Bug in mod_ratelimit?


Hi Cory,

On Thu, Jul 19, 2018 at 11:23 PM, Cory McIntire <cory@xxxxxxxxxx> 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;
         }
     }