On Tue, Dec 13, 2016 at 10:48 AM, Plüm, Rüdiger, Vodafone Group
<ruediger.pl...@vodafone.com> wrote:
>>
>> To clarify: I can't reproduce any problems with r1773861 in the first
>> place, even with ErrorDocument. I agree that r1773862 (and r1773865)
>> work for me; I just don't know what makes them functionally different.
>> In my attempted test cases, I can't find any case where the rr->pool
>> used during the internal redirect differs from the original r->pool.
>
> I don't see any case where this is needed. Internal redirects always
> use the same pool as the original request. So it should be sufficient
> to memorize just in r->pool.
Agreed (as I said to Jacob, I was thinking of sub-requests like pool
for internal-requests).
I'll commit the change now that the "main" fix is in 2.4.24 (not
really needed there, this is not a bug or a fast path anyway).
>
> Another aspect of all these patches that I don't get is why we need
> to eat the contents of the original brigade? IMHO we don't need to do
> this. It is thrown away automatically at the end of the request and
> as we leave with an AP_FILTER_ERROR after the ap_die it should never
> be sent.
The pros with this is indeed the reduced complexity (though the loop
to walk the brigade already existed), the cons are that we rely on the
caller/handler to not ignore ap_pass_brigade()'s return value and
mainly that we could close/reset an incomplete backend/origin
connection (hence not reusable) while it may not be the culprit (e.g.,
when some "Header set ..." is).
> If we want to play safe we can remove ourselves from the
> filterchain after returning from ap_die. This could make the whole
> stuff much less complex.
Still we'd depend on the caller to do the right thing with errors
(AP_FILTER_ERROR).
I don't find the change too complex after all, and that's a quite
critical filter for doing the right/safe thing...
I'm even inclined to do the below changes, so that we are really safe
(i.e. ignore any data) after EOS...
@@ -1257,7 +1254,6 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_http_heade
/* We may come back here from ap_die() below,
* so clear anything from this response.
*/
- ctx->headers_error = 0;
apr_table_clear(r->headers_out);
apr_table_clear(r->err_headers_out);
@@ -1267,6 +1263,7 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_http_heade
* empty body (EOS only).
*/
if (!check_headers_recursion(r)) {
+ ctx->headers_error = 0;
apr_brigade_cleanup(b);
ap_die(HTTP_INTERNAL_SERVER_ERROR, r);
return AP_FILTER_ERROR;
_
Regards,
Yann.
