httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ruediger Pluem <rpl...@apache.org>
Subject Re: Handling AP_FILTER_ERROR
Date Mon, 01 Dec 2008 08:52:34 GMT


On 12/01/2008 12:42 AM, Nick Kew wrote:
> 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?

That depends on what the caller of ap_run_sub_req does with the result.
I guess no general statement is possible here.

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

Sorry, but now you confused ME :-).

So lets try to sort this out from the start.

1. The run_handler hook might return AP_FILTER_ERROR back to ap_invoke_handler.
   So AP_FILTER_ERROR might be the final result *after* the handler phase.
   With the current code ap_invoke_handler would transform AP_FILTER_ERROR into
   HTTP_INTERNAL_SERVER_ERROR and return this. After the patch
   ap_invoke_handler would return AP_FILTER_ERROR instead.

2. IMHO ap_invoke_handler is only called in three locations:

   ap_run_sub_req
   ap_process_request
   ap_internal_redirect

2.1 ap_run_sub_req
    ap_run_sub_req will return HTTP_INTERNAL_SERVER_ERROR (now) or
    AP_FILTER_ERROR (after the patch) to the caller. It cannot be
    said what the caller does with this return code as subrequests
    are done from many location and also by third party modules.

2.2 ap_process_request
    ap_process_request sets r->status to HTTP_OK (to avoid error page recursion)
    and calls ap_die with the return code (HTTP_INTERNAL_SERVER_ERROR / AP_FILTER_ERROR).
    With the current code this produces an error page, after the patch this is a noop.
    Afterwards the status is never used again.

2.3 ap_internal_redirect
    Seems like the same case as ap_process_request to me.

I fail to see where a recursion could happen here after the patch and where
the error buckets come into the game. IMHO the worst case is that a
filter returns AP_FILTER_ERROR before any HTTP status code was sent, but does
nothing else (like sending an error bucket down the chain), the handler feeding
the chain does nothing but returning this code to ap_invoke_handler / ap_internal_redirect
and subsequently to ap_process_request after the patch which would cause us to not sent any
response at all (which would be also wrong).
So ap_die maybe should do the following:

    if (type == AP_FILTER_ERROR) {
        if [ap_http_header_filter is still in chain] {
           log this
           type = HTTP_INTERNAL_SERVER_ERROR;
        }
        else {
           return;
        }
    }


Regards

RĂ¼diger

Mime
View raw message