httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Nick Kew <n...@webthing.com>
Subject Re: Handling AP_FILTER_ERROR
Date Sun, 30 Nov 2008 23:42:50 GMT
On Sun, 30 Nov 2008 18:22:39 -0500
"Eric Covener" <covener@gmail.com> wrote:

> On Sun, Nov 30, 2008 at 5:20 PM, Nick Kew <nick@webthing.com> 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/

Mime
View raw message