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

Re: Bug in mod_ratelimit?

Hi Yann,

2018-07-19 22:55 GMT+02:00 Yann Ylavic <ylavic.dev@xxxxxxxxx>:
> Hi Luca,
> On Thu, Jul 19, 2018 at 9:59 PM, Luca Toscano <toscano.luca@xxxxxxxxx> 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!