AW: svn commit: r1773865 - /httpd/httpd/trunk/modules/http/http_filters.c
> -Ursprüngliche Nachricht- > Von: Jacob Champion [mailto:champio...@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 > 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: r1774010 - in /httpd/test/framework/trunk/t: conf/extra.conf.in modules/ext_filter.t
This fails on all systems that have sed in /usr/bin/sed
or someplace other than /bin :(
> On Dec 13, 2016, at 8:20 AM, jor...@apache.org wrote:
>
> Author: jorton
> Date: Tue Dec 13 13:20:32 2016
> New Revision: 1774010
>
> URL: http://svn.apache.org/viewvc?rev=1774010&view=rev
> Log:
> Add basic test case for mod_ext_filter & specific test for PR 60375.
>
> Added:
>httpd/test/framework/trunk/t/modules/ext_filter.t
> Modified:
>httpd/test/framework/trunk/t/conf/extra.conf.in
>
> Modified: httpd/test/framework/trunk/t/conf/extra.conf.in
> URL:
> http://svn.apache.org/viewvc/httpd/test/framework/trunk/t/conf/extra.conf.in?rev=1774010&r1=1774009&r2=1774010&view=diff
> ==
> --- httpd/test/framework/trunk/t/conf/extra.conf.in (original)
> +++ httpd/test/framework/trunk/t/conf/extra.conf.in Tue Dec 13 13:20:32 2016
> @@ -1049,3 +1049,29 @@ LimitRequestFields32
>
>
>
> +
> +
> +#
> +# t/modules/ext_filter.t test config
> +#
> +
> +
> +ExtFilterDefine foo-to-bar mode=output cmd="/bin/sed s,foo,bar,g"
> +ExtFilterDefine ifoo-to-bar mode=input cmd="/bin/sed s,foo,bar,g"
> +AliasMatch /apache/extfilter/[^/]+/(.*) @DocumentRoot@/$1
> +
> +
> + SetOutputFilter foo-to-bar
> +
> +
> +
> + SetInputFilter ifoo-to-bar
> +
> +
> +
> + SetOutputFilter foo-to-bar
> + LimitRequestBody 6
> +
> +
> +
> +
>
> Added: httpd/test/framework/trunk/t/modules/ext_filter.t
> URL:
> http://svn.apache.org/viewvc/httpd/test/framework/trunk/t/modules/ext_filter.t?rev=1774010&view=auto
> ==
> --- httpd/test/framework/trunk/t/modules/ext_filter.t (added)
> +++ httpd/test/framework/trunk/t/modules/ext_filter.t Tue Dec 13 13:20:32 2016
> @@ -0,0 +1,32 @@
> +use strict;
> +use warnings FATAL => 'all';
> +
> +use Apache::Test;
> +use Apache::TestRequest;
> +use Apache::TestUtil;
> +
> +Apache::TestRequest::user_agent(keep_alive => 1);
> +
> +my $iters = 10;
> +my $tests = 3 + $iters * 2;
> +
> +plan tests => $tests, need need_module('ext_filter');
> +
> +if (Apache::TestConfig::WINFU()) {
> +skip "Unix-only test" foreach 1..$tests;
> +}
> +
> +ok t_cmp(GET_BODY("/apache/extfilter/out-foo/foobar.html"), "barbar", "sed
> output filter");
> +
> +my $r = POST "/apache/extfilter/in-foo/modules/cgi/perl_echo.pl", content =>
> "foobar";
> +
> +ok t_cmp($r->code, 200, "echo worked");
> +ok t_cmp($r->content, "barbar", "request body filtered");
> +
> +# PR 60375 -- appears to be intermittent failure with 2.4.x ... but works
> with trunk?
> +foreach (1..$iters) {
> +$r = POST "/apache/extfilter/out-limit/modules/cgi/perl_echo.pl",
> content => "foo and bar";
> +
> +ok t_cmp($r->code, 413, "got 413 error");
> +ok t_cmp($r->content, qr/413 Request Entity Too Large/, "got 413 error
> body");
> +}
>
>
Re: svn commit: r1773865 - /httpd/httpd/trunk/modules/http/http_filters.c
On Tue, Dec 13, 2016 at 10:48 AM, Plüm, Rüdiger, Vodafone Group
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.
re: svn commit: r1774014 - /httpd/httpd/branches/2.4.x/STATUS
Hi, I can't send a patch because I'm on travel, but there is a minor issue with r1768245. I think that should be or . I thought I had sent a patch for it, but can find it in SVN history. Maybe, I forgot to commit, or it was for another similar issue in another file. CJ > > > - > + *) mod_socache_memcache: Provide memcache STATs to mod_status > + trunk patch: http://svn.apache.org/r1768245 > + trunk patch: http://svn.apache.org/r1770828 > + 2.4.x patch: trunk works > + +1: jim, icing, covener > >
Re: svn commit: r1774014 - /httpd/httpd/branches/2.4.x/STATUS
On Tue, Dec 13, 2016 at 8:54 AM, Marion et Christophe JAILLET wrote: > Hi, > > > > I can't send a patch because I'm on travel, but there is a minor issue with > r1768245. > > I think that should be or . > > > > I thought I had sent a patch for it, but can find it in SVN history. > > Maybe, I forgot to commit, or it was for another similar issue in another > file. > Yes, I see what you mean. Thanks!
Re: svn commit: r1774014 - /httpd/httpd/branches/2.4.x/STATUS
Handled in: Committed revision 1774018. Committed revision 1774019. > On Dec 13, 2016, at 8:57 AM, Eric Covener wrote: > > On Tue, Dec 13, 2016 at 8:54 AM, Marion et Christophe JAILLET > wrote: >> Hi, >> >> >> >> I can't send a patch because I'm on travel, but there is a minor issue with >> r1768245. >> >> I think that should be or . >> >> >> >> I thought I had sent a patch for it, but can find it in SVN history. >> >> Maybe, I forgot to commit, or it was for another similar issue in another >> file. >> > > Yes, I see what you mean. Thanks!
Re: svn commit: r1774010 - in /httpd/test/framework/trunk/t: conf/extra.conf.in modules/ext_filter.t
> On Dec 13, 2016, at 8:33 AM, Jim Jagielski wrote: > > This fails on all systems that have sed in /usr/bin/sed > or someplace other than /bin :( > Hmmm... even w/ the change to /usr/bin/sed I'm getting test #1 failing: t/modules/ext_filter.t .. 1..23 # Running under perl version 5.020003 for darwin # Current time local: Tue Dec 13 10:10:15 2016 # Current time GMT: Tue Dec 13 15:10:15 2016 # Using Test.pm version 1.26 # Using Apache/Test.pm version 1.41 # testing : sed output filter # expected: 'barbar' # received: 'barbar # ' not ok 1 # Failed test 1 in t/modules/ext_filter.t at line 19 Wondering if this is an OSX sed issue. Will see how gsed handles this.
Re: svn commit: r1774010 - in /httpd/test/framework/trunk/t: conf/extra.conf.in modules/ext_filter.t
Confirming that 2.4.x HEAD fails as noted in PR 60375 > On Dec 13, 2016, at 10:14 AM, Jim Jagielski wrote: > > >> On Dec 13, 2016, at 8:33 AM, Jim Jagielski wrote: >> >> This fails on all systems that have sed in /usr/bin/sed >> or someplace other than /bin :( >> > > Hmmm... even w/ the change to /usr/bin/sed I'm getting > test #1 failing: > > t/modules/ext_filter.t .. > 1..23 > # Running under perl version 5.020003 for darwin > # Current time local: Tue Dec 13 10:10:15 2016 > # Current time GMT: Tue Dec 13 15:10:15 2016 > # Using Test.pm version 1.26 > # Using Apache/Test.pm version 1.41 > # testing : sed output filter > # expected: 'barbar' > # received: 'barbar > # ' > not ok 1 > # Failed test 1 in t/modules/ext_filter.t at line 19 > > Wondering if this is an OSX sed issue. Will see how gsed > handles this. >
Re: svn commit: r1774010 - in /httpd/test/framework/trunk/t: conf/extra.conf.in modules/ext_filter.t
Here is the error-log entry: [Tue Dec 13 10:27:36.041785 2016] [ext_filter:error] [pid 58675:tid 123145321619456] (9)Bad file descriptor: [client 127.0.0.1:63016] AH01464: apr_file_close(child input) [Tue Dec 13 10:27:36.041902 2016] [ext_filter:error] [pid 58675:tid 123145321619456] (9)Bad file descriptor: [client 127.0.0.1:63016] AH01468: ef_unified_filter() failed > On Dec 13, 2016, at 10:35 AM, Jim Jagielski wrote: > > Confirming that 2.4.x HEAD fails as noted in PR 60375 > >> On Dec 13, 2016, at 10:14 AM, Jim Jagielski wrote: >> >> >>> On Dec 13, 2016, at 8:33 AM, Jim Jagielski wrote: >>> >>> This fails on all systems that have sed in /usr/bin/sed >>> or someplace other than /bin :( >>> >> >> Hmmm... even w/ the change to /usr/bin/sed I'm getting >> test #1 failing: >> >> t/modules/ext_filter.t .. >> 1..23 >> # Running under perl version 5.020003 for darwin >> # Current time local: Tue Dec 13 10:10:15 2016 >> # Current time GMT: Tue Dec 13 15:10:15 2016 >> # Using Test.pm version 1.26 >> # Using Apache/Test.pm version 1.41 >> # testing : sed output filter >> # expected: 'barbar' >> # received: 'barbar >> # ' >> not ok 1 >> # Failed test 1 in t/modules/ext_filter.t at line 19 >> >> Wondering if this is an OSX sed issue. Will see how gsed >> handles this. >> >
Re: svn commit: r1773865 - /httpd/httpd/trunk/modules/http/http_filters.c
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 > 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
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
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
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
On Tue, Dec 13, 2016 at 8:37 PM, Ruediger Pluem 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
On Tue, Dec 13, 2016 at 6:42 PM, Yann Ylavic wrote: > On Tue, Dec 13, 2016 at 8:37 PM, Ruediger Pluem 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.
