Re: Handling AP_FILTER_ERROR
On 12/01/2008 12:42 AM, Nick Kew wrote:
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?
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
Re: Handling AP_FILTER_ERROR
On 1 Dec 2008, at 08:52, Ruediger Pluem wrote:
2. IMHO ap_invoke_handler is only called in three locations:
It's a public API, so could be called by any module.
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;
}
}
That looks good, too.
But do you see any objection to the (IMHO simpler) fix of
removing error buckets as we detect them?
--
Nick Kew
Re: Handling AP_FILTER_ERROR
On 12/01/2008 10:02 AM, Nick Kew wrote:
On 1 Dec 2008, at 08:52, Ruediger Pluem wrote:
2. IMHO ap_invoke_handler is only called in three locations:
It's a public API, so could be called by any module.
Correct.
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;
}
}
That looks good, too.
But do you see any objection to the (IMHO simpler) fix of
removing error buckets as we detect them?
Not directly. Could you please propose a concrete patch?
It makes discussion easier :-).
Regards
Rüdiger
Re: Handling AP_FILTER_ERROR
On 1 Dec 2008, at 09:31, Ruediger Pluem wrote:
But do you see any objection to the (IMHO simpler) fix of
removing error buckets as we detect them?
Not directly. Could you please propose a concrete patch?
It makes discussion easier :-).
I think the one-liner should work, since we don't re-use the error
bucket.
So against trunk, that's:
--- modules/http/http_filters.c (revision 722000)
+++ modules/http/http_filters.c (working copy)
@@ -1133,6 +1133,7 @@
{
if (AP_BUCKET_IS_ERROR(e) !eb) {
eb = e-data;
+apr_bucket_delete(e);
continue;
}
/*
--
Nick Kew
Re: Handling AP_FILTER_ERROR
On 12/01/2008 11:01 AM, Nick Kew wrote:
On 1 Dec 2008, at 09:31, Ruediger Pluem wrote:
But do you see any objection to the (IMHO simpler) fix of
removing error buckets as we detect them?
Not directly. Could you please propose a concrete patch?
It makes discussion easier :-).
I think the one-liner should work, since we don't re-use the error bucket.
So against trunk, that's:
--- modules/http/http_filters.c (revision 722000)
+++ modules/http/http_filters.c (working copy)
@@ -1133,6 +1133,7 @@
{
if (AP_BUCKET_IS_ERROR(e) !eb) {
eb = e-data;
+apr_bucket_delete(e);
continue;
}
/*
I guess this could segfault (the status is in eb-status).
Why not cleaning up the whole brigade? IMHO it is of no use
anymore when we do an ap_die and afterwards return AP_FILTER_ERROR;
Index: modules/http/http_filters.c
===
--- modules/http/http_filters.c (Revision 721833)
+++ modules/http/http_filters.c (Arbeitskopie)
@@ -1145,7 +1145,11 @@
}
}
if (eb) {
-ap_die(eb-status, r);
+int status;
+
+status = eb-status;
+apr_brigade_cleanup(b);
+ap_die(status, r);
return AP_FILTER_ERROR;
}
Regards
Rüdiger
Re: Handling AP_FILTER_ERROR
On 1 Dec 2008, at 10:19, Ruediger Pluem wrote:
if (eb) {
-ap_die(eb-status, r);
+int status;
+
+status = eb-status;
+apr_brigade_cleanup(b);
+ap_die(status, r);
return AP_FILTER_ERROR;
}
Good call. +1 to that.
--
Nick Kew
Re: Handling AP_FILTER_ERROR
On Mon, Dec 1, 2008 at 6:44 AM, Nick Kew [EMAIL PROTECTED] wrote:
On 1 Dec 2008, at 10:19, Ruediger Pluem wrote:
if (eb) {
-ap_die(eb-status, r);
+int status;
+
+status = eb-status;
+apr_brigade_cleanup(b);
+ap_die(status, r);
return AP_FILTER_ERROR;
}
Good call. +1 to that.
I'll revisit this afternoon, add to the backport, and reset the voting.
Thanks all.
--
Eric Covener
[EMAIL PROTECTED]
Re: Handling AP_FILTER_ERROR
On Mon, Dec 1, 2008 at 7:33 AM, Eric Covener [EMAIL PROTECTED] wrote:
On Mon, Dec 1, 2008 at 6:44 AM, Nick Kew [EMAIL PROTECTED] wrote:
On 1 Dec 2008, at 10:19, Ruediger Pluem wrote:
if (eb) {
-ap_die(eb-status, r);
+int status;
+
+status = eb-status;
+apr_brigade_cleanup(b);
+ap_die(status, r);
return AP_FILTER_ERROR;
}
Good call. +1 to that.
I'll revisit this afternoon, add to the backport, and reset the voting.
Committed in r722081 with no change in perl framework.
--
Eric Covener
[EMAIL PROTECTED]
Re: Handling AP_FILTER_ERROR
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. 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? 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. 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. -- Eric Covener [EMAIL PROTECTED]
Re: Handling AP_FILTER_ERROR
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/
