On Sun, 30 Nov 2008 18:22:39 -0500
"Eric Covener" <[EMAIL PROTECTED]> wrote:
> On Sun, Nov 30, 2008 at 5:20 PM, Nick Kew <[EMAIL PROTECTED]> wrote:
> > In practice, the proposed fix looks good, and I want to
> > vote +1. I'm just a little concerned over whether we're
> > in danger of an infinite loop if we feed an AP_FILTER_ERROR
> > into ap_http_header_filter. The filter will just return
> > AP_FILTER_ERROR, and might get re-invoked with the error
> > still there.
>
> This is my first real pass through the filters, so please correct me
> if something looks fishy.
Something certainly looks fishy. But it's not your analysis:
it's the long-standing confusion over what a filter error looks like.
> If the filter is ever re-invoked, with the same brigade containing the
> error bucket, doesn't that mean an earlier filter (or handler) is
> looping over the same (uncleared) brigade?
Sure, it means noone has cleared the brigade. But it is customary
to clear a brigade as we consume it, and it's possible something
might be assuming that's happening.
> > Looking at ap_http_header_filter, if we encounter an error,
> > we first note it and continue. If we subsequently encounter
> > EOC, we'll return ap_pass_brigade and ignore the error
> > altogether. Otherwise we'll call ap_die (which is a no-op
> > if passed AP_FILTER_ERROR) and then return AP_FILTER_ERROR
> > up the stack, leaving the filter in place.
>
> Note that the ap_die() call in this context doesn't pass the filter
> chain rv (AP_FILTER_ERROR) but the stashed away HTTP error code that
> was pulled from the error bucket.
Yes. But in principle, that error could be AP_FILTER_ERROR, and will
be if recursion ever happens. How about, for example, a filter error
in a subrequest returning AP_FILTER_ERROR. What does the parent
request see?
> This is the call that actually gets
> us the error response someone has queued up earlier (e.g.
> LimitRequestBody during the HTTP_IN filter)
>
> It's the later call to ap_die, back in ap_process_request, that should
> see AP_FILTER_ERROR and no-op. This is the return code that the
> general fix for PR#31759 is catching as non-HTTP and changing to 500.
>
"Should" is great; I'm worrying about edge-cases.
To rephrase: I'd +1 the patch if either:
(1) We do as I suggest, and remove the error bucket in
ap_http_header_filter,
or
(2) Someone can convince me there is really no possibility,
even in the event of a bug elsewhere, of this recursion.
--
Nick Kew
Application Development with Apache - the Apache Modules Book
http://www.apachetutor.org/
