You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by yl...@apache.org on 2016/12/12 20:31:44 UTC
svn commit: r1773865 - /httpd/httpd/trunk/modules/http/http_filters.c
Author: ylavic
Date: Mon Dec 12 20:31:44 2016
New Revision: 1773865
URL: http://svn.apache.org/viewvc?rev=1773865&view=rev
Log:
Follow up to r1773761: improved recursion detection.
Modified:
httpd/httpd/trunk/modules/http/http_filters.c
Modified: httpd/httpd/trunk/modules/http/http_filters.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http/http_filters.c?rev=1773865&r1=1773864&r2=1773865&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/http/http_filters.c (original)
+++ httpd/httpd/trunk/modules/http/http_filters.c Mon Dec 12 20:31:44 2016
@@ -687,6 +687,20 @@ static APR_INLINE int check_headers(requ
apr_table_do(check_header, &ctx, r->err_headers_out, NULL);
}
+static int check_headers_recursion(request_rec *r)
+{
+ request_rec *rr;
+ for (rr = r; rr; rr = rr->prev) {
+ void *dying = NULL;
+ apr_pool_userdata_get(&dying, "check_headers_recursion", rr->pool);
+ if (dying) {
+ return 1;
+ }
+ }
+ apr_pool_userdata_setn("true", "check_headers_recursion", NULL, r->pool);
+ return 0;
+}
+
typedef struct header_struct {
apr_pool_t *pool;
apr_bucket_brigade *bb;
@@ -1192,7 +1206,6 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_
header_filter_ctx *ctx = f->ctx;
const char *ctype;
ap_bucket_error *eb = NULL;
- void *dying = NULL;
int eos = 0;
AP_DEBUG_ASSERT(!r->main);
@@ -1208,7 +1221,6 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_
}
}
else if (!ctx->headers_error && !check_headers(r)) {
- apr_pool_userdata_get(&dying, "http_header_filter_dying", r->pool);
ctx->headers_error = 1;
}
for (e = APR_BRIGADE_FIRST(b);
@@ -1249,14 +1261,13 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_
apr_table_clear(r->headers_out);
apr_table_clear(r->err_headers_out);
- /* Don't try ErrorDocument if we are (internal-)redirect-ed or dying
- * already, otherwise we can end up in infinite recursion, better fall
- * through with 500, minimal headers and an empty body (EOS only).
+ /* Don't recall ap_die() if we come back here (from its own internal
+ * redirect or error response), otherwise we can end up in infinite
+ * recursion; better fall through with 500, minimal headers and an
+ * empty body (EOS only).
*/
- if (ap_is_initial_req(r) && !dying) {
+ if (!check_headers_recursion(r)) {
apr_brigade_cleanup(b);
- apr_pool_userdata_setn("true", "http_header_filter_dying",
- apr_pool_cleanup_null, r->pool);
ap_die(HTTP_INTERNAL_SERVER_ERROR, r);
return AP_FILTER_ERROR;
}
Re: svn commit: r1773865 - /httpd/httpd/trunk/modules/http/http_filters.c
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Mon, Dec 12, 2016 at 3:32 PM, Yann Ylavic <yl...@gmail.com> wrote:
> On Mon, Dec 12, 2016 at 10:16 PM, William A Rowe Jr <wr...@rowe-clan.net>
> wrote:
> > On Mon, Dec 12, 2016 at 3:07 PM, Jacob Champion <ch...@gmail.com>
> > wrote:+
> >>
> >>
> >> What's the case where this catches recursion that the previous logic in
> >> r1773861 did not handle? I'm trying to write a test that fails on
> r1773861
> >> and succeeds on r1773865, but I haven't figured it out yet.
> >
> >
> > I'm confused by a different aspect.
> >
> > In trashing the body-in-flight, whose headers caused us to 500-reject
> > the response, have we also trashed any and all correct error documents
> > or built-in short 500 response explanation?
>
> No, I tried (quite hard, in a second time) to honor ErrorDocument by
> calling ap_die() when check_headers() fails.
>
> That's only if/when that ErrorDocument is caught by check_headers that
> we end up generating a minimal 500 response (with Server, Date,
> Connection: close and empty body), to avoid infinite recursion.
>
I set up a test case of a constant text line and an html error doc, and can
confirm that everything is working as expected/hoped for.
Thank you for all the troubleshooting and coding on this effort!
Re: svn commit: r1773865 - /httpd/httpd/trunk/modules/http/http_filters.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Mon, Dec 12, 2016 at 10:16 PM, William A Rowe Jr <wr...@rowe-clan.net> wrote:
> On Mon, Dec 12, 2016 at 3:07 PM, Jacob Champion <ch...@gmail.com>
> wrote:+
>>
>>
>> What's the case where this catches recursion that the previous logic in
>> r1773861 did not handle? I'm trying to write a test that fails on r1773861
>> and succeeds on r1773865, but I haven't figured it out yet.
>
>
> I'm confused by a different aspect.
>
> In trashing the body-in-flight, whose headers caused us to 500-reject
> the response, have we also trashed any and all correct error documents
> or built-in short 500 response explanation?
No, I tried (quite hard, in a second time) to honor ErrorDocument by
calling ap_die() when check_headers() fails.
That's only if/when that ErrorDocument is caught by check_headers that
we end up generating a minimal 500 response (with Server, Date,
Connection: close and empty body), to avoid infinite recursion.
Re: svn commit: r1773865 - /httpd/httpd/trunk/modules/http/http_filters.c
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Mon, Dec 12, 2016 at 3:07 PM, Jacob Champion <ch...@gmail.com>
wrote:+
>
>
> What's the case where this catches recursion that the previous logic in
> r1773861 did not handle? I'm trying to write a test that fails on r1773861
> and succeeds on r1773865, but I haven't figured it out yet.
I'm confused by a different aspect.
In trashing the body-in-flight, whose headers caused us to 500-reject
the response, have we also trashed any and all correct error documents
or built-in short 500 response explanation? Unlike a 400-reject (which is
always caused by a badly implemented client, and should never arrive
from a conventional browser user-agent), the 500-reject speaks to some
module or backend that isn't under the requestor's control. An error doc
may be quite helpful in this situation.
I've verified that the bad headers in the current patch are redacted looking
at wireshark capture. Only one (first) bad header is logged, which is fine
IMO, no need for excessive error.log noise.
Re: svn commit: r1773865 - /httpd/httpd/trunk/modules/http/http_filters.c
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Tue, Dec 13, 2016 at 6:42 PM, Yann Ylavic <yl...@gmail.com> wrote:
> On Tue, Dec 13, 2016 at 8:37 PM, Ruediger Pluem <rp...@apache.org> wrote:
> >
> > This change is also unrelated to the bad header issue and I think
> > if there is interest to address this it should be done in a separate
> > patch set.
>
> It is actually, we could eventually avoid reading/consuming the body
> of the original response (closing prematuraly any backend
> connection...), but in any case we need to eat the ap_die()'s one if
> we encounter a bad header on its generated response.
> That's because we want to generate a minimal 500 response in this
> case, with no body (even the ap_die(500)'s one not be related,
> ErrorDocument can be any redirection, including proxying).
>
> Thus the eat-body code is needed anyway, let's also use it for the
> original response then
Indeed but we have to reiterate one point Fielding has raised in another
forum that seems to be ignored in this conversation.
In light of bad input, the http servers and user agents must present valid
data to their upstream servers and downstream consumers.
The spec does not define how this is accomplished, the entire response
can be redacted, or the invalid headers can be redacted. No consumer
can be presented with invalid request or response data.
How we choose to accomplish this is entirely our choice, that was
Roy's point.
I took the following statement literally and accepted Yann's proposal;
"This is still not great. We get the original body, a 500 status code
and status line."
Any 500 status with the original body would be completely nonsense,
and I didn't even test to confirm that this earlier assertion was true,
I just assumed the statement was accurate.
But it could still be the original status with all garbage response
headers redacted.
It could be a proper 500 response and let httpd's mechanisms to
kill the request, letting the handler still dump useless data at the
filter stack does work (confirmed). Provided it presents the correct
error body, and redacts bad headers at every phase, we deliver
a comprehensible response in any case.
Best yet, we inform the caller that the send brigade call failed, and
let smart callers short-circuit streams that they are able to. As a
practical matter, we can't do that in the proxy, since the entire
response body must be eaten to ensure correct socket closure.
But the direct response to the caller could and should be issued
directly.
Right now, we have Yann's proposal. Several of us would like to
see patches attached to these concerns with a cleaner solution.
I'm not clear that the filter stack was conceived as the origin
of error responses, but this case, and many other conceivable
examples proves that we should have considered that case, and
made the programming pattern a little clearer.
Re: AW: svn commit: r1773865 - /httpd/httpd/trunk/modules/http/http_filters.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Mon, Dec 19, 2016 at 9:00 PM, Ruediger Pluem <rp...@apache.org> wrote:
>
>
> This doesn't seem to be in trunk yet. Care to commit?
Done in r1775195, thanks Rüdiger for your great suggestions.
Regards,
Yann.
Re: AW: svn commit: r1773865 -
/httpd/httpd/trunk/modules/http/http_filters.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 12/15/2016 11:13 AM, Pl�m, R�diger, Vodafone Group wrote:
>
>
>> -----Urspr�ngliche Nachricht-----
>> Von: Yann Ylavic [mailto:ylavic.dev@gmail.com]
>> Gesendet: Mittwoch, 14. Dezember 2016 19:58
>> An: httpd-dev <de...@httpd.apache.org>
>> Betreff: Re: svn commit: r1773865 -
>> /httpd/httpd/trunk/modules/http/http_filters.c
>>
>> On Wed, Dec 14, 2016 at 9:45 AM, Pl�m, R�diger, Vodafone Group
>> <ru...@vodafone.com> wrote:
>>>
>>>> -----Urspr�ngliche Nachricht-----
>>>> Von: Yann Ylavic [mailto:ylavic.dev@gmail.com]
>>>> Gesendet: Mittwoch, 14. Dezember 2016 01:42
>>
>>>>
>>>> For the EOC case, how about:
>>>
>>> Looks good apart that I wouldn't do the break in the EOS case to catch
>> EOC buckets after the EOS.
>>
>> Applied in r1774286 (and proposed for backport, 2.4.24?).
>
> Thanks. Just voted. So only one more vote missing.
>
>>
>> No hurry about the new (attached) patch however, only an improvement
>> IMHO.
>
> New one looks good. Happy to see this in trunk.
This doesn't seem to be in trunk yet. Care to commit?
Regards
R�diger
AW: svn commit: r1773865 -
/httpd/httpd/trunk/modules/http/http_filters.c
Posted by Plüm,
Rüdiger,
Vodafone Group <ru...@vodafone.com>.
> -----Ursprüngliche Nachricht-----
> Von: Yann Ylavic [mailto:ylavic.dev@gmail.com]
> Gesendet: Mittwoch, 14. Dezember 2016 19:58
> An: httpd-dev <de...@httpd.apache.org>
> Betreff: Re: svn commit: r1773865 -
> /httpd/httpd/trunk/modules/http/http_filters.c
>
> On Wed, Dec 14, 2016 at 9:45 AM, Plüm, Rüdiger, Vodafone Group
> <ru...@vodafone.com> wrote:
> >
> >> -----Ursprüngliche Nachricht-----
> >> Von: Yann Ylavic [mailto:ylavic.dev@gmail.com]
> >> Gesendet: Mittwoch, 14. Dezember 2016 01:42
>
> >>
> >> For the EOC case, how about:
> >
> > Looks good apart that I wouldn't do the break in the EOS case to catch
> EOC buckets after the EOS.
>
> Applied in r1774286 (and proposed for backport, 2.4.24?).
Thanks. Just voted. So only one more vote missing.
>
> No hurry about the new (attached) patch however, only an improvement
> IMHO.
New one looks good. Happy to see this in trunk.
Regards
Rüdiger
Re: svn commit: r1773865 - /httpd/httpd/trunk/modules/http/http_filters.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Wed, Dec 14, 2016 at 9:45 AM, Plüm, Rüdiger, Vodafone Group
<ru...@vodafone.com> wrote:
>
>> -----Ursprüngliche Nachricht-----
>> Von: Yann Ylavic [mailto:ylavic.dev@gmail.com]
>> Gesendet: Mittwoch, 14. Dezember 2016 01:42
>>
>> It is actually, we could eventually avoid reading/consuming the body
>> of the original response (closing prematuraly any backend
>> connection...), but in any case we need to eat the ap_die()'s one if
>> we encounter a bad header on its generated response.
>> That's because we want to generate a minimal 500 response in this
>> case, with no body (even the ap_die(500)'s one not be related,
>> ErrorDocument can be any redirection, including proxying).
>
> We could do that easier in this case:
Actually I played with your proposal and it's indeed cleaner.
I also revised my position about whether we should or not close some
origin/backend connection early when turning the response into 500,
and think now we should make it know about the "issue" anyway (by
closing prematurely).
Hence the attached patch, no content slurping, use of AP_FILTER_ERROR
and EOC semantics to respectively warn the caller and cleanly
terminate the connection afterwards.
>>
>> Hmm, we return AP_FILTER_ERROR right?
>
> But only after we slurped all content. We return APR_SUCESS until we see EOS.
> For the reasons above this is bad.
Same issue w/o slurping the content if the response is made of
multiple brigades (i.e. EOS not in first one), because the http_header
filter is then removed and http_outerror would receive any following
METADATA directly (after the EOC+EOS we add ourself).
So I had still to handle this in the proposed patch (see
filter_error=1 handling), such that we always return AP_FILTER_ERROR
once the 500 is sent out (I could also have added the EOS after the
EOC only if it was already in the original brigade, but I think it's
better to stop the caller [ap_die() here] from sending more
[meta-]data, the connection is over anyway).
Overall I think it's still a simplification, more semantically correct too...
>>
>> For the EOC case, how about:
>
> Looks good apart that I wouldn't do the break in the EOS case to catch EOC buckets after the EOS.
Applied in r1774286 (and proposed for backport, 2.4.24?).
No hurry about the new (attached) patch however, only an improvement IMHO.
Regards,
Yann.
Re: AW: svn commit: r1773865 - /httpd/httpd/trunk/modules/http/http_filters.c
Posted by Luca Toscano <to...@gmail.com>.
2016-12-14 18:53 GMT+01:00 Jacob Champion <ch...@gmail.com>:
> On 12/14/2016 09:33 AM, Jacob Champion wrote:
>
>> The current public filter documentation at [1] (which is old, but still
>> the only overall documentation I'm aware of) says
>>
>
> Forgot the link, sorry.
>
> [1] https://httpd.apache.org/docs/trunk/developer/output-filters.html
+1, it would be really great to have more bucket-related documentation for
newcomers like me, it would help a lot to bootstrap committers in the right
way :)
Thanks!
Luca
Re: AW: svn commit: r1773865 -
/httpd/httpd/trunk/modules/http/http_filters.c
Posted by Jacob Champion <ch...@gmail.com>.
On 12/14/2016 09:33 AM, Jacob Champion wrote:
> The current public filter documentation at [1] (which is old, but still
> the only overall documentation I'm aware of) says
Forgot the link, sorry.
[1] https://httpd.apache.org/docs/trunk/developer/output-filters.html
--Jacob
Re: AW: svn commit: r1773865 -
/httpd/httpd/trunk/modules/http/http_filters.c
Posted by Jacob Champion <ch...@gmail.com>.
On 12/14/2016 12:45 AM, Pl�m, R�diger, Vodafone Group wrote:
>> -----Urspr�ngliche Nachricht-----
>> Von: Yann Ylavic [mailto:ylavic.dev@gmail.com]
>
>> @@ -1227,13 +1224,6 @@ AP_CORE_DECLARE_NONSTD(apr_status_t)
>> ap_http_heade
>> e != APR_BRIGADE_SENTINEL(b);
>> e = APR_BUCKET_NEXT(e))
>> {
>> - if (ctx->headers_error) {
>> - if (APR_BUCKET_IS_EOS(e)) {
>> - eos = 1;
>> - break;
>> - }
>> - continue;
>> - }
>> if (AP_BUCKET_IS_ERROR(e) && !eb) {
>> eb = e->data;
>> continue;
>> @@ -1246,6 +1236,11 @@ AP_CORE_DECLARE_NONSTD(apr_status_t)
>> ap_http_heade
>> ap_remove_output_filter(f);
>> return ap_pass_brigade(f->next, b);
>> }
>> +
>> + if (ctx->headers_error && APR_BUCKET_IS_EOS(e)) {
>> + eos = 1;
>> + break;
>> + }
>> }
>> if (ctx->headers_error) {
>> if (!eos) {
>> ?
>>
>
> Looks good apart that I wouldn't do the break in the EOS case to
> catch EOC buckets after the EOS. I guess in practice that will not
> add much or additional rounds in the loop at all, but it Keeps the
> current expectation that EOC can be anywhere.
The current public filter documentation at [1] (which is old, but still
the only overall documentation I'm aware of) says
> An EOS bucket indicates that the end of the response has been
> reachedand no further buckets need be processed.
and
> [Rule] 3. Output filters should ignore any buckets following an EOS
> bucket.
Is there an up-to-date set of rules somewhere else I've missed? If not,
can anyone update that document with the rules for 2.4?
--Jacob
AW: svn commit: r1773865 -
/httpd/httpd/trunk/modules/http/http_filters.c
Posted by Plüm,
Rüdiger,
Vodafone Group <ru...@vodafone.com>.
> -----Ursprüngliche Nachricht-----
> Von: Yann Ylavic [mailto:ylavic.dev@gmail.com]
> Gesendet: Mittwoch, 14. Dezember 2016 01:42
> An: httpd-dev <de...@httpd.apache.org>
> Betreff: Re: svn commit: r1773865 -
> /httpd/httpd/trunk/modules/http/http_filters.c
>
> On Tue, Dec 13, 2016 at 8:37 PM, Ruediger Pluem <rp...@apache.org>
> wrote:
> >
> > On 12/13/2016 02:49 PM, Yann Ylavic wrote:
> >>
> >> 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
> >
> > IMHO this would violate a principal and easy to follow design pattern.
> > I don't think that we can and want to protect module developers from
> > all errors they could make, especially if they are easy to spot
> > and understand. And if why catch this here?
>
> I agree that defensive programming in not good (though the whole
> check_headers() patchset is nothing else after all...).
>
> >
> > If someone in the the filter chain continues sending content even
> > after passing things down the chain caused an error weird things
> > can happen before us or if the source is after us after us.
>
> Agreed, still.
>
> >
> > This change is also unrelated to the bad header issue and I think
> > if there is interest to address this it should be done in a separate
> > patch set.
>
> It is actually, we could eventually avoid reading/consuming the body
> of the original response (closing prematuraly any backend
> connection...), but in any case we need to eat the ap_die()'s one if
> we encounter a bad header on its generated response.
> That's because we want to generate a minimal 500 response in this
> case, with no body (even the ap_die(500)'s one not be related,
> ErrorDocument can be any redirection, including proxying).
We could do that easier in this case:
Index: http_filters.c
===================================================================
--- http_filters.c (revision 1774129)
+++ http_filters.c (working copy)
@@ -1248,11 +1248,6 @@
}
}
if (ctx->headers_error) {
- if (!eos) {
- /* Eat body until EOS */
- apr_brigade_cleanup(b);
- return APR_SUCCESS;
- }
/* We may come back here from ap_die() below,
* so clear anything from this response.
@@ -1271,14 +1266,16 @@
ap_die(HTTP_INTERNAL_SERVER_ERROR, r);
return AP_FILTER_ERROR;
}
- AP_DEBUG_ASSERT(APR_BUCKET_IS_EOS(e));
- APR_BUCKET_REMOVE(e);
apr_brigade_cleanup(b);
+ e = apr_bucket_eoc_create(f->c->bucket_alloc);
APR_BRIGADE_INSERT_TAIL(b, e);
+ e = apr_bucket_eos_create(f->c->bucket_alloc);
+ APR_BRIGADE_INSERT_TAIL(b, e);
r->status = HTTP_INTERNAL_SERVER_ERROR;
r->content_type = r->content_encoding = NULL;
r->content_languages = NULL;
ap_set_content_length(r, 0);
+
}
else if (eb) {
int status;
The ap_http_outerror_filter will then kill any further possible data
that gets down the chain.
>
> Thus the eat-body code is needed anyway, let's also use it for the
> original response then.
>
> >
> > Furthermore it causes a handler to continue generating content
> > in a properly resource intensive way that we drop directly. And the
> > handler has no chance to know this because we return APR_SUCCESS.
>
> Hmm, we return AP_FILTER_ERROR right?
But only after we slurped all content. We return APR_SUCESS until we see EOS.
For the reasons above this is bad.
>
> >>
> >> I'm even inclined to do the below changes, so that we are really safe
> >> (i.e. ignore any data) after EOS...
> >
> > After this patch we do the wrong thing with an EOC bucket.
> > The current contract is that an EOC bucket *anywhere* in the brigade
> causes the
> > header filter to go out of the way. After the patch this is broken if
> > a bad header is present. But as EOC tells us to get out of the way and
> do nothing,
> > we don't need to take care about bad headers. This breaks edge case
> with mod_proxy_http
>
> OK, since we don't send the headers in the EOC case, I agree we should
> bypass check_headers(). But we may not be aware of an EOC unless it
> is part of the first brigade sent down the chain, and in this case
> check_headers() could trigger (that's fine I think, for late EOC
> cases, i.e. error while forwarding bodies, we need to send or block
> something already).
>
> >
> > Albeit this would be fixable I see more negative points then positive
> points here
> > and I am -0.5 on that content slurping.
>
> You mean, the one already backported to 2.4.x or the additional hunks
> I proposed above?
The code already in 2.4.x
> I think we should fix the EOC case in 2.4.x, but still eat ignored
> bodies in ap_http_header_filter().
>
> For the EOC case, how about:
>
> @@ -1227,13 +1224,6 @@ AP_CORE_DECLARE_NONSTD(apr_status_t)
> ap_http_heade
> e != APR_BRIGADE_SENTINEL(b);
> e = APR_BUCKET_NEXT(e))
> {
> - if (ctx->headers_error) {
> - if (APR_BUCKET_IS_EOS(e)) {
> - eos = 1;
> - break;
> - }
> - continue;
> - }
> if (AP_BUCKET_IS_ERROR(e) && !eb) {
> eb = e->data;
> continue;
> @@ -1246,6 +1236,11 @@ AP_CORE_DECLARE_NONSTD(apr_status_t)
> ap_http_heade
> ap_remove_output_filter(f);
> return ap_pass_brigade(f->next, b);
> }
> +
> + if (ctx->headers_error && APR_BUCKET_IS_EOS(e)) {
> + eos = 1;
> + break;
> + }
> }
> if (ctx->headers_error) {
> if (!eos) {
> ?
>
Looks good apart that I wouldn't do the break in the EOS case to catch EOC buckets after the EOS.
I guess in practice that will not add much or additional rounds in the loop at all, but it
Keeps the current expectation that EOC can be anywhere.
Regards
Rüdiger
Re: svn commit: r1773865 - /httpd/httpd/trunk/modules/http/http_filters.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Tue, Dec 13, 2016 at 8:37 PM, Ruediger Pluem <rp...@apache.org> wrote:
>
> On 12/13/2016 02:49 PM, Yann Ylavic wrote:
>>
>> 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
>
> IMHO this would violate a principal and easy to follow design pattern.
> I don't think that we can and want to protect module developers from
> all errors they could make, especially if they are easy to spot
> and understand. And if why catch this here?
I agree that defensive programming in not good (though the whole
check_headers() patchset is nothing else after all...).
>
> If someone in the the filter chain continues sending content even
> after passing things down the chain caused an error weird things
> can happen before us or if the source is after us after us.
Agreed, still.
>
> This change is also unrelated to the bad header issue and I think
> if there is interest to address this it should be done in a separate
> patch set.
It is actually, we could eventually avoid reading/consuming the body
of the original response (closing prematuraly any backend
connection...), but in any case we need to eat the ap_die()'s one if
we encounter a bad header on its generated response.
That's because we want to generate a minimal 500 response in this
case, with no body (even the ap_die(500)'s one not be related,
ErrorDocument can be any redirection, including proxying).
Thus the eat-body code is needed anyway, let's also use it for the
original response then.
>
> Furthermore it causes a handler to continue generating content
> in a properly resource intensive way that we drop directly. And the
> handler has no chance to know this because we return APR_SUCCESS.
Hmm, we return AP_FILTER_ERROR right?
>>
>> I'm even inclined to do the below changes, so that we are really safe
>> (i.e. ignore any data) after EOS...
>
> After this patch we do the wrong thing with an EOC bucket.
> The current contract is that an EOC bucket *anywhere* in the brigade causes the
> header filter to go out of the way. After the patch this is broken if
> a bad header is present. But as EOC tells us to get out of the way and do nothing,
> we don't need to take care about bad headers. This breaks edge case with mod_proxy_http
OK, since we don't send the headers in the EOC case, I agree we should
bypass check_headers(). But we may not be aware of an EOC unless it
is part of the first brigade sent down the chain, and in this case
check_headers() could trigger (that's fine I think, for late EOC
cases, i.e. error while forwarding bodies, we need to send or block
something already).
>
> Albeit this would be fixable I see more negative points then positive points here
> and I am -0.5 on that content slurping.
You mean, the one already backported to 2.4.x or the additional hunks
I proposed above?
I think we should fix the EOC case in 2.4.x, but still eat ignored
bodies in ap_http_header_filter().
For the EOC case, how about:
@@ -1227,13 +1224,6 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_http_heade
e != APR_BRIGADE_SENTINEL(b);
e = APR_BUCKET_NEXT(e))
{
- if (ctx->headers_error) {
- if (APR_BUCKET_IS_EOS(e)) {
- eos = 1;
- break;
- }
- continue;
- }
if (AP_BUCKET_IS_ERROR(e) && !eb) {
eb = e->data;
continue;
@@ -1246,6 +1236,11 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_http_heade
ap_remove_output_filter(f);
return ap_pass_brigade(f->next, b);
}
+
+ if (ctx->headers_error && APR_BUCKET_IS_EOS(e)) {
+ eos = 1;
+ break;
+ }
}
if (ctx->headers_error) {
if (!eos) {
?
Regards,
Yann.
Re: svn commit: r1773865 -
/httpd/httpd/trunk/modules/http/http_filters.c
Posted by Jacob Champion <ch...@gmail.com>.
On 12/13/2016 12:18 PM, Ruediger Pluem wrote:
> Have a look at line 1334 of mod_proxy_http.c:
Okay, that's starting to make some more sense. Thanks!
>> On 12/13/2016 11:37 AM, Ruediger Pluem wrote:
>>> After this patch we do the wrong thing with an EOC bucket.
>>> The current contract is that an EOC bucket *anywhere* in the brigade causes the
>>> header filter to go out of the way. After the patch this is broken if
>>> a bad header is present. But as EOC tells us to get out of the way and do nothing,
>>> we don't need to take care about bad headers.
As I understand it:
- the case you're discussing happens when we're proxying to an upstream
HTTP server, *and* this is at least the second request on this
particular client connection, *and* we fail to get a status line from
upstream, *and* somehow an invalid header gets set (perhaps with the
Header directive)
- the new behavior from the client side is that, instead of the
connection simply dropping out with no response, we send a 500 and
*then* drop the connection.
Whether this is broken behavior from the outside looking in, I'm not
sure -- an invalid header in this situation implies there's been a
misconfiguration; why not send a 500? -- but your point about the
internal API contract stands.
There's also the other question you seem to be raising -- do we need to
check headers and 500 an outgoing response if no headers are actually
going to be sent? IIUC, this same question also applies to the HTTP/0.9
code path.
--Jacob
Re: svn commit: r1773865 -
/httpd/httpd/trunk/modules/http/http_filters.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 12/13/2016 08:57 PM, Jacob Champion wrote:
> On 12/13/2016 11:37 AM, Ruediger Pluem wrote:
>> On 12/13/2016 02:49 PM, Yann Ylavic wrote:
>>> 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...
>>
>> After this patch we do the wrong thing with an EOC bucket.
>> The current contract is that an EOC bucket *anywhere* in the brigade causes the
>> header filter to go out of the way. After the patch this is broken if
>> a bad header is present. But as EOC tells us to get out of the way and do nothing,
>> we don't need to take care about bad headers. This breaks edge case with mod_proxy_http
>
> Can you elaborate on this broken case? Is it something we can add a test for?
Have a look at line 1334 of mod_proxy_http.c:
/*
* If we are a reverse proxy request shutdown the connection
* WITHOUT ANY response to trigger a retry by the client
* if allowed (as for idempotent requests).
* BUT currently we should not do this if the request is the
* first request on a keepalive connection as browsers like
* seamonkey only display an empty page in this case and do
* not do a retry. We should also not do this on a
* connection which times out; instead handle as
* we normally would handle timeouts
*/
Regards
R�diger
Re: svn commit: r1773865 -
/httpd/httpd/trunk/modules/http/http_filters.c
Posted by Jacob Champion <ch...@gmail.com>.
On 12/13/2016 11:37 AM, Ruediger Pluem wrote:
> On 12/13/2016 02:49 PM, Yann Ylavic wrote:
>> 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...
>
> After this patch we do the wrong thing with an EOC bucket.
> The current contract is that an EOC bucket *anywhere* in the brigade causes the
> header filter to go out of the way. After the patch this is broken if
> a bad header is present. But as EOC tells us to get out of the way and do nothing,
> we don't need to take care about bad headers. This breaks edge case with mod_proxy_http
Can you elaborate on this broken case? Is it something we can add a test
for?
--Jacob
Re: svn commit: r1773865 -
/httpd/httpd/trunk/modules/http/http_filters.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 12/13/2016 02:49 PM, Yann Ylavic wrote:
> On Tue, Dec 13, 2016 at 10:48 AM, Pl�m, R�diger, Vodafone Group
> <ru...@vodafone.com> wrote:
>> 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
IMHO this would violate a principal and easy to follow design pattern.
I don't think that we can and want to protect module developers from
all errors they could make, especially if they are easy to spot
and understand. And if why catch this here?
If someone in the the filter chain continues sending content even
after passing things down the chain caused an error weird things
can happen before us or if the source is after us after us.
This change is also unrelated to the bad header issue and I think
if there is interest to address this it should be done in a separate
patch set.
Furthermore it causes a handler to continue generating content
in a properly resource intensive way that we drop directly. And the
handler has no chance to know this because we return APR_SUCCESS.
> 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 see this differently. See above.
> 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...
After this patch we do the wrong thing with an EOC bucket.
The current contract is that an EOC bucket *anywhere* in the brigade causes the
header filter to go out of the way. After the patch this is broken if
a bad header is present. But as EOC tells us to get out of the way and do nothing,
we don't need to take care about bad headers. This breaks edge case with mod_proxy_http
Albeit this would be fixable I see more negative points then positive points here
and I am -0.5 on that content slurping.
Regards
R�diger
Re: svn commit: r1773865 - /httpd/httpd/trunk/modules/http/http_filters.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Tue, Dec 13, 2016 at 10:48 AM, Plüm, Rüdiger, Vodafone Group
<ru...@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.
AW: svn commit: r1773865 -
/httpd/httpd/trunk/modules/http/http_filters.c
Posted by Plüm,
Rüdiger,
Vodafone Group <ru...@vodafone.com>.
> -----Ursprüngliche Nachricht-----
> Von: Jacob Champion [mailto:champion.p@gmail.com]
> Gesendet: Dienstag, 13. Dezember 2016 00:01
> An: dev@httpd.apache.org
> Betreff: Re: svn commit: r1773865 -
> /httpd/httpd/trunk/modules/http/http_filters.c
>
> On 12/12/2016 01:23 PM, Yann Ylavic wrote:
> > On Mon, Dec 12, 2016 at 10:07 PM, Jacob Champion
> <ch...@gmail.com> wrote:
> >
> >>
> >> What's the case where this catches recursion that the previous logic
> in
> >> r1773861 did not handle? I'm trying to write a test that fails on
> r1773861
> >> and succeeds on r1773865, but I haven't figured it out yet.
> >
> > I think it's more r1773862 that fixes your test case.
>
> 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.
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. 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.
Regards
Rüdiger
Re: svn commit: r1773865 - /httpd/httpd/trunk/modules/http/http_filters.c
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Dec 12, 2016 6:07 PM, "Yann Ylavic" <yl...@gmail.com> wrote:
On Tue, Dec 13, 2016 at 12:17 AM, Jacob Champion <ch...@gmail.com>
wrote:
> On 12/12/2016 03:10 PM, William A Rowe Jr wrote:
>>
>> On Mon, Dec 12, 2016 at 5:00 PM, Jacob Champion <champion.p@gmail.com
>> <ma...@gmail.com>> wrote:
>>
>> On 12/12/2016 01:23 PM, Yann Ylavic wrote:
>>
>> On Mon, Dec 12, 2016 at 10:07 PM, Jacob Champion
>> <champion.p@gmail.com <ma...@gmail.com>> wrote:
>>
>>
>> What's the case where this catches recursion that the
>> previous logic in
>> r1773861 did not handle? I'm trying to write a test that
>> fails on r1773861
>> and succeeds on r1773865, but I haven't figured it out yet.
>>
>>
>> I think it's more r1773862 that fixes your test case.
>>
>>
>> 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.
>>
>> Can you send me a config snippet that reproduces the loop with
>> ErrorDocument? I'm not arguing against your followup patches; I just
>> want to make sure a correct test case gets into the suite. :D
>>
>>
>> Speaking of the test suite behavior, your mission had succeeded. My quad
>> core machine was pegged, X-Windows/Gnome unresponsive.
>>
>> Do we want to put such tests in the framework?
>
>
> I would say yes, definitely. Better for us to bring down a tester's
machine
> with a regression and fix the problem before it goes live, than to spare
the
> tester and end up shipping said regression.
Not worried about my machine. Hanging a build machine with no feedback
would be an issue
>> If any of our perl gurus
>> have a good suggestion to throttle the top limit of cpu/time consumed,
>> that would be a good enhancement to the framework.
>
>
> Agreed!
BSD::Resource's setrlimit(RLIMIT_CPU, soft, hard)?
If portable to Linux/Windows, then mission accomplished.
Re: svn commit: r1773865 - /httpd/httpd/trunk/modules/http/http_filters.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Tue, Dec 13, 2016 at 12:17 AM, Jacob Champion <ch...@gmail.com> wrote:
> On 12/12/2016 03:10 PM, William A Rowe Jr wrote:
>>
>> On Mon, Dec 12, 2016 at 5:00 PM, Jacob Champion <champion.p@gmail.com
>> <ma...@gmail.com>> wrote:
>>
>> On 12/12/2016 01:23 PM, Yann Ylavic wrote:
>>
>> On Mon, Dec 12, 2016 at 10:07 PM, Jacob Champion
>> <champion.p@gmail.com <ma...@gmail.com>> wrote:
>>
>>
>> What's the case where this catches recursion that the
>> previous logic in
>> r1773861 did not handle? I'm trying to write a test that
>> fails on r1773861
>> and succeeds on r1773865, but I haven't figured it out yet.
>>
>>
>> I think it's more r1773862 that fixes your test case.
>>
>>
>> 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.
>>
>> Can you send me a config snippet that reproduces the loop with
>> ErrorDocument? I'm not arguing against your followup patches; I just
>> want to make sure a correct test case gets into the suite. :D
>>
>>
>> Speaking of the test suite behavior, your mission had succeeded. My quad
>> core machine was pegged, X-Windows/Gnome unresponsive.
>>
>> Do we want to put such tests in the framework?
>
>
> I would say yes, definitely. Better for us to bring down a tester's machine
> with a regression and fix the problem before it goes live, than to spare the
> tester and end up shipping said regression.
>
>> If any of our perl gurus
>> have a good suggestion to throttle the top limit of cpu/time consumed,
>> that would be a good enhancement to the framework.
>
>
> Agreed!
BSD::Resource's setrlimit(RLIMIT_CPU, soft, hard)?
Re: svn commit: r1773865 -
/httpd/httpd/trunk/modules/http/http_filters.c
Posted by Jacob Champion <ch...@gmail.com>.
On 12/12/2016 03:10 PM, William A Rowe Jr wrote:
> On Mon, Dec 12, 2016 at 5:00 PM, Jacob Champion <champion.p@gmail.com
> <ma...@gmail.com>> wrote:
>
> On 12/12/2016 01:23 PM, Yann Ylavic wrote:
>
> On Mon, Dec 12, 2016 at 10:07 PM, Jacob Champion
> <champion.p@gmail.com <ma...@gmail.com>> wrote:
>
>
> What's the case where this catches recursion that the
> previous logic in
> r1773861 did not handle? I'm trying to write a test that
> fails on r1773861
> and succeeds on r1773865, but I haven't figured it out yet.
>
>
> I think it's more r1773862 that fixes your test case.
>
>
> 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.
>
> Can you send me a config snippet that reproduces the loop with
> ErrorDocument? I'm not arguing against your followup patches; I just
> want to make sure a correct test case gets into the suite. :D
>
>
> Speaking of the test suite behavior, your mission had succeeded. My quad
> core machine was pegged, X-Windows/Gnome unresponsive.
>
> Do we want to put such tests in the framework?
I would say yes, definitely. Better for us to bring down a tester's
machine with a regression and fix the problem before it goes live, than
to spare the tester and end up shipping said regression.
> If any of our perl gurus
> have a good suggestion to throttle the top limit of cpu/time consumed,
> that would be a good enhancement to the framework.
Agreed!
--Jacob
Re: svn commit: r1773865 - /httpd/httpd/trunk/modules/http/http_filters.c
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Mon, Dec 12, 2016 at 5:00 PM, Jacob Champion <ch...@gmail.com>
wrote:
> On 12/12/2016 01:23 PM, Yann Ylavic wrote:
>
>> On Mon, Dec 12, 2016 at 10:07 PM, Jacob Champion <ch...@gmail.com>
>> wrote:
>>
>>
>>> What's the case where this catches recursion that the previous logic in
>>> r1773861 did not handle? I'm trying to write a test that fails on
>>> r1773861
>>> and succeeds on r1773865, but I haven't figured it out yet.
>>>
>>
>> I think it's more r1773862 that fixes your test case.
>>
>
> 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.
>
> Can you send me a config snippet that reproduces the loop with
> ErrorDocument? I'm not arguing against your followup patches; I just want
> to make sure a correct test case gets into the suite. :D
Speaking of the test suite behavior, your mission had succeeded. My quad
core machine was pegged, X-Windows/Gnome unresponsive.
Do we want to put such tests in the framework? If any of our perl gurus
have a good suggestion to throttle the top limit of cpu/time consumed,
that would be a good enhancement to the framework.
Re: svn commit: r1773865 -
/httpd/httpd/trunk/modules/http/http_filters.c
Posted by Jacob Champion <ch...@gmail.com>.
On 12/12/2016 03:20 PM, Yann Ylavic wrote:
> On Tue, Dec 13, 2016 at 12:00 AM, Jacob Champion <ch...@gmail.com> wrote:
>> On 12/12/2016 01:23 PM, Yann Ylavic wrote:
>>>
>>> On Mon, Dec 12, 2016 at 10:07 PM, Jacob Champion <ch...@gmail.com>
>>> wrote:
>>>
>>>>
>>>> What's the case where this catches recursion that the previous logic in
>>>> r1773861 did not handle? I'm trying to write a test that fails on
>>>> r1773861
>>>> and succeeds on r1773865, but I haven't figured it out yet.
>>>
>>>
>>> I think it's more r1773862 that fixes your test case.
>>
>>
>> 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.
>
> Oh, right, I thought internal-requests were like sub-requests, with
> rr->pool being a subpool of r->pool, but that's not the case as you
> mention.
>
> So I guess r1773861 is OK already, though the below may be simpler/cleaner now:
Thanks for the clarification. I am all for simpler and cleaner, assuming
it works the same. :D
--Jacob
Re: svn commit: r1773865 - /httpd/httpd/trunk/modules/http/http_filters.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Tue, Dec 13, 2016 at 12:20 AM, Yann Ylavic <yl...@gmail.com> wrote:
>
> So I guess r1773861 is OK already, though the below may be simpler/cleaner now:
>
> Index: modules/http/http_filters.c
> ===================================================================
> --- modules/http/http_filters.c (revision 1773865)
> +++ modules/http/http_filters.c (working copy)
> @@ -689,13 +689,10 @@ static APR_INLINE int check_headers(request_rec *r
>
> static int check_headers_recursion(request_rec *r)
> {
> - request_rec *rr;
> - for (rr = r; rr; rr = rr->prev) {
> - void *dying = NULL;
> - apr_pool_userdata_get(&dying, "check_headers_recursion", rr->pool);
> - if (dying) {
> - return 1;
> - }
> + void *dying = NULL;
> + apr_pool_userdata_get(&dying, "check_headers_recursion", rr->pool);
s/rr/r/ here of course.
> + if (dying) {
> + return 1;
> }
> apr_pool_userdata_setn("true", "check_headers_recursion", NULL, r->pool);
> return 0;
> _
Re: svn commit: r1773865 - /httpd/httpd/trunk/modules/http/http_filters.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Tue, Dec 13, 2016 at 12:00 AM, Jacob Champion <ch...@gmail.com> wrote:
> On 12/12/2016 01:23 PM, Yann Ylavic wrote:
>>
>> On Mon, Dec 12, 2016 at 10:07 PM, Jacob Champion <ch...@gmail.com>
>> wrote:
>>
>>>
>>> What's the case where this catches recursion that the previous logic in
>>> r1773861 did not handle? I'm trying to write a test that fails on
>>> r1773861
>>> and succeeds on r1773865, but I haven't figured it out yet.
>>
>>
>> I think it's more r1773862 that fixes your test case.
>
>
> 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.
Oh, right, I thought internal-requests were like sub-requests, with
rr->pool being a subpool of r->pool, but that's not the case as you
mention.
So I guess r1773861 is OK already, though the below may be simpler/cleaner now:
Index: modules/http/http_filters.c
===================================================================
--- modules/http/http_filters.c (revision 1773865)
+++ modules/http/http_filters.c (working copy)
@@ -689,13 +689,10 @@ static APR_INLINE int check_headers(request_rec *r
static int check_headers_recursion(request_rec *r)
{
- request_rec *rr;
- for (rr = r; rr; rr = rr->prev) {
- void *dying = NULL;
- apr_pool_userdata_get(&dying, "check_headers_recursion", rr->pool);
- if (dying) {
- return 1;
- }
+ void *dying = NULL;
+ apr_pool_userdata_get(&dying, "check_headers_recursion", rr->pool);
+ if (dying) {
+ return 1;
}
apr_pool_userdata_setn("true", "check_headers_recursion", NULL, r->pool);
return 0;
_
Re: svn commit: r1773865 -
/httpd/httpd/trunk/modules/http/http_filters.c
Posted by Jacob Champion <ch...@gmail.com>.
On 12/12/2016 01:23 PM, Yann Ylavic wrote:
> On Mon, Dec 12, 2016 at 10:07 PM, Jacob Champion <ch...@gmail.com> wrote:
>
>>
>> What's the case where this catches recursion that the previous logic in
>> r1773861 did not handle? I'm trying to write a test that fails on r1773861
>> and succeeds on r1773865, but I haven't figured it out yet.
>
> I think it's more r1773862 that fixes your test case.
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.
Can you send me a config snippet that reproduces the loop with
ErrorDocument? I'm not arguing against your followup patches; I just
want to make sure a correct test case gets into the suite. :D
> In r1773861, the recursion wouldn't work with "ErrorDocument
> /some/file" for example, because ap_die() may either call
> ap_intern_redirects() which is caught by ap_is_initial_req(), or
> ap_send_error_response() directly which is caught by pool userdata
> "dying".
>
> r1773865 allows to not depend on ap_die() internals, and also allows
> to try ap_die() (once) for internal redirects that
> ap_http_header_filter() did not generated itself...
>
> Regards,
> Yann.
>
Re: svn commit: r1773865 - /httpd/httpd/trunk/modules/http/http_filters.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Mon, Dec 12, 2016 at 10:07 PM, Jacob Champion <ch...@gmail.com> wrote:
>
> What's the case where this catches recursion that the previous logic in
> r1773861 did not handle? I'm trying to write a test that fails on r1773861
> and succeeds on r1773865, but I haven't figured it out yet.
I think it's more r1773862 that fixes your test case.
In r1773861, the recursion wouldn't work with "ErrorDocument
/some/file" for example, because ap_die() may either call
ap_intern_redirects() which is caught by ap_is_initial_req(), or
ap_send_error_response() directly which is caught by pool userdata
"dying".
r1773865 allows to not depend on ap_die() internals, and also allows
to try ap_die() (once) for internal redirects that
ap_http_header_filter() did not generated itself...
Regards,
Yann.
Re: svn commit: r1773865 -
/httpd/httpd/trunk/modules/http/http_filters.c
Posted by Jacob Champion <ch...@gmail.com>.
On 12/12/2016 12:31 PM, ylavic@apache.org wrote:
> Author: ylavic
> Date: Mon Dec 12 20:31:44 2016
> New Revision: 1773865
>
> URL: http://svn.apache.org/viewvc?rev=1773865&view=rev
> Log:
> Follow up to r1773761: improved recursion detection.
>
> Modified:
> httpd/httpd/trunk/modules/http/http_filters.c
>
> Modified: httpd/httpd/trunk/modules/http/http_filters.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http/http_filters.c?rev=1773865&r1=1773864&r2=1773865&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/http/http_filters.c (original)
> +++ httpd/httpd/trunk/modules/http/http_filters.c Mon Dec 12 20:31:44 2016
> @@ -687,6 +687,20 @@ static APR_INLINE int check_headers(requ
> apr_table_do(check_header, &ctx, r->err_headers_out, NULL);
> }
>
> +static int check_headers_recursion(request_rec *r)
> +{
> + request_rec *rr;
> + for (rr = r; rr; rr = rr->prev) {
> + void *dying = NULL;
> + apr_pool_userdata_get(&dying, "check_headers_recursion", rr->pool);
> + if (dying) {
> + return 1;
> + }
> + }
> + apr_pool_userdata_setn("true", "check_headers_recursion", NULL, r->pool);
> + return 0;
> +}
> +
What's the case where this catches recursion that the previous logic in
r1773861 did not handle? I'm trying to write a test that fails on
r1773861 and succeeds on r1773865, but I haven't figured it out yet.
--Jacob