httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Yann Ylavic <ylavic....@gmail.com>
Subject Re: svn commit: r1773865 - /httpd/httpd/trunk/modules/http/http_filters.c
Date Tue, 13 Dec 2016 13:49:07 GMT
On Tue, Dec 13, 2016 at 10:48 AM, Plüm, Rüdiger, Vodafone Group
<ruediger.pluem@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.

Mime
View raw message