Re: svn commit: r1774010 - in /httpd/test/framework/trunk/t: conf/extra.conf.in modules/ext_filter.t

[William A Rowe Jr]

On Wed, Dec 14, 2016 at 2:51 AM, Joe Orton  wrote:

> On Tue, Dec 13, 2016 at 10:14:21AM -0500, 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:
>
> Ah, sorry about that & thanks for adjusting the config.
>
> I adjusted the test case to cope with a sed which adds a trailing
> newline now, does that pass with OS X sed?  I'm yet to work out why the
> test cases passes in trunk and fails in 2.4, mod_ext_filter is the same
> in both branches.
>

Seeing different issues on Fedora 24 under 2.x and 2.4 branches, neither
seems entirely good, 2.4 is especially strange.

On 2.x branch, Test #2 fails with a failed to create process.

# testing : slow filter process
# expected: 'foobar'
# received: '
# 
# 500 Internal Server Error
# 
# Internal Server Error
# The server encountered an internal error or
# misconfiguration and was unable to complete
# your request.
# Please contact the server administrator at
#  y...@example.com to inform them of the time this error occurred,
#  and the actions you performed just before this error.
# More information about this error may be available
# in the server error log.
# '
not ok 2

[Thu Dec 15 05:07:20.945238 2016] [example_hooks:notice] [pid 19326:tid
139620847179520] AH03297: x_create_connection()
[Thu Dec 15 05:07:20.945327 2016] [example_hooks:notice] [pid 19326:tid
139620847179520] AH03297: x_create_request()
[Thu Dec 15 05:07:20.945573 2016] [authz_core:debug] [pid 19326:tid
139620847179520] mod_authz_core.c(834): [client 127.0.0.1:49182] AH01628:
authorization result: granted (no directives)
[Thu Dec 15 05:07:20.945603 2016] [charset_lite:debug] [pid 19326:tid
139620847179520] mod_charset_lite.c(216): [client 127.0.0.1:49182] AH01448:
incomplete configuration: src unspecified, dst unspecified
[Thu Dec 15 05:07:20.945742 2016] [dialup:notice] [pid 19326:tid
139620847179520] (70023)This function has not been implemented on this
platform: [client 127.0.0.1:49182] AH02637: dialup: MPM doesn't support
suspending
[Thu Dec 15 05:07:20.981454 2016] [optional_hook_import:error] [pid
19326:tid 139620847179520] AH01866: Optional hook test said: GET
/index.html HTTP/1.1
[Thu Dec 15 05:07:20.981473 2016] [optional_fn_export:error] [pid 19326:tid
139620847179520] AH01871: Optional function test said: GET /index.html
HTTP/1.1
[Thu Dec 15 05:07:21.560737 2016] [example_hooks:notice] [pid 19326:tid
139620838786816] AH03297: x_create_connection()
[Thu Dec 15 05:07:21.560806 2016] [example_hooks:notice] [pid 19326:tid
139620838786816] AH03297: x_create_request()
[Thu Dec 15 05:07:21.560950 2016] [authz_core:debug] [pid 19326:tid
139620838786816] mod_authz_core.c(834): [client 127.0.0.1:49186] AH01628:
authorization result: granted (no directives)
[Thu Dec 15 05:07:21.560967 2016] [charset_lite:debug] [pid 19326:tid
139620838786816] mod_charset_lite.c(216): [client 127.0.0.1:49186] AH01448:
incomplete configuration: src unspecified, dst unspecified
[Thu Dec 15 05:07:21.561006 2016] [dialup:notice] [pid 19326:tid
139620838786816] (70023)This function has not been implemented on this
platform: [client 127.0.0.1:49186] AH02637: dialup: MPM doesn't support
suspending
[Thu Dec 15 05:07:21.572188 2016] [optional_hook_import:error] [pid
19326:tid 139620838786816] AH01866: Optional hook test said: GET
/apache/extfilter/out-foo/foobar.html HTTP/1.1
[Thu Dec 15 05:07:21.572199 2016] [optional_fn_export:error] [pid 19326:tid
139620838786816] AH01871: Optional function test said: GET
/apache/extfilter/out-foo/foobar.html HTTP/1.1
[Thu Dec 15 05:07:21.572237 2016] [ext_filter:error] [pid 19326:tid
139620838786816] (9)Bad file descriptor: [client 127.0.0.1:49186] AH01466:
apr_file_read(child output), len 18446744073709551615
[Thu Dec 15 05:07:21.572251 2016] [ext_filter:error] [pid 19326:tid
139620838786816] (9)Bad file descriptor: [client 127.0.0.1:49186] AH01468:
ef_unified_filter() failed
[Thu Dec 15 05:07:21.572267 2016] [example_hooks:notice] [pid 19326:tid
139620838786816] AH03297: x_create_request()
[Thu Dec 15 05:07:21.578555 2016] [authz_core:debug] [pid 19326:tid
139620838786816] mod_authz_core.c(834): [client 127.0.0.1:49186] AH01628:
authorization result: granted (no directives)
[Thu Dec 15 05:07:21.578578 2016] [charset_lite:debug] [pid 19326:tid
139620838786816] mod_charset_lite.c(216): [client 127.0.0.1:49186] AH01448:
incomplete configuration: src unspecified, dst unspecified
[Thu Dec 15 05:07:21.578616 2016] [dialup:notice] [pid 19326:tid
139620838786816] (70023)This function has not been implemented on this
platform: [client 127.0.0.1:49186] AH02637: dialup: MPM doesn't support
suspending
[Thu Dec 15 05:07:21.578683 2016] [ext_filter:error] [pid 19326:tid
139620838786816] (2)No such file or directory: [client 

Re: svn commit: r1773865 - /httpd/httpd/trunk/modules/http/http_filters.c

[Yann Ylavic]

On Wed, Dec 14, 2016 at 9:45 AM, Plüm, Rüdiger, Vodafone Group
 wrote:
>
>> -Ursprüngliche Nachricht-
>> Von: Yann Ylavic [mailto:ylavic@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.
Index: modules/http/http_filters.c
===
--- modules/http/http_filters.c	(revision 1774286)
+++ modules/http/http_filters.c	(working copy)
@@ -1188,7 +1188,6 @@ AP_DECLARE_NONSTD(int) ap_send_http_trace(request_
 
 typedef struct header_filter_ctx {
 int headers_sent;
-int headers_error;
 } header_filter_ctx;
 
 AP_CORE_DECLARE_NONSTD(apr_status_t) ap_http_header_filter(ap_filter_t *f,
@@ -1203,7 +1202,8 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_http_heade
 header_filter_ctx *ctx = f->ctx;
 const char *ctype;
 ap_bucket_error *eb = NULL;
-apr_bucket *eos = NULL;
+apr_status_t rv = APR_SUCCESS;
+int filter_error = 0;
 
 AP_DEBUG_ASSERT(!r->main);
 
@@ -1210,7 +1210,7 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_http_heade
 if (!ctx) {
 ctx = f->ctx = apr_pcalloc(r->pool, sizeof(header_filter_ctx));
 }
-if (ctx->headers_sent) {
+else if (ctx->headers_sent) {
 /* Eat body if response must not have one. */
 if (r->header_only || r->status == HTTP_NO_CONTENT) {
 apr_brigade_cleanup(b);
@@ -1217,9 +1217,7 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_http_heade
 return APR_SUCCESS;
 }
 }
-else if (!ctx->headers_error && !check_headers(r)) {
-ctx->headers_error = 1;
-}
+
 for (e = APR_BRIGADE_FIRST(b);
  e != APR_BRIGADE_SENTINEL(b);
  e = APR_BUCKET_NEXT(e))
@@ -1236,23 +1234,15 @@ 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 = e;
-}
 }
-if (ctx->headers_error) {
-if (!eos) {
-/* Eat body until EOS */
-apr_brigade_cleanup(b);
-return APR_SUCCESS;
-}
 
+if (!ctx->headers_sent && !check_headers(r)) {
 /* 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);
+apr_brigade_cleanup(b);
 
 /* 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
@@ -1260,17 +1250,18 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_http_heade
  * empty body (EOS only).
  */
 if (!check_headers_recursion(r)) {
-apr_brigade_cleanup(b);
 

Re: AW: svn commit: r1773865 - /httpd/httpd/trunk/modules/http/http_filters.c

[Jacob Champion]

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

[Jacob Champion]

On 12/14/2016 12:45 AM, Plüm, Rüdiger, Vodafone Group wrote:

-Ursprüngliche Nachricht-
Von: Yann Ylavic [mailto:ylavic@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

Re: T of 2.4.24

[Jim Jagielski]

Looking at a T of 2.4.24 either the 15th or 16th...

Re: Absorb win32-apxs into httpd distro?

[Issac Goldstand]

On 12/14/2016 10:13 AM, William A Rowe Jr wrote:
> Randy wrote http://www.apache.org/dist/perl/win32-bin/
>  - but I'm wondering
> who else here at httpd is interested in helping maintain and get this code
> into our own distribution? I've shipped this for a decade for my commercial
> customers, for every wrinkle and wart, I think many of our win32 users
> would appreciate this.
> 
> I trust the perl pmc and Randy would be good with this adoption?
> 
> Cheers,
> 
> Bill

+1 in concept, but although I'd be interested in theory in supporting
it, realistically I'd have little to no time to put into it for the time
being.

Re: svn commit: r1774010 - in /httpd/test/framework/trunk/t: conf/extra.conf.in modules/ext_filter.t

[Joe Orton]

On Tue, Dec 13, 2016 at 10:14:21AM -0500, 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:

Ah, sorry about that & thanks for adjusting the config.

I adjusted the test case to cope with a sed which adds a trailing 
newline now, does that pass with OS X sed?  I'm yet to work out why the 
test cases passes in trunk and fails in 2.4, mod_ext_filter is the same 
in both branches.

Regards, Joe

AW: svn commit: r1773865 - /httpd/httpd/trunk/modules/http/http_filters.c

[Plüm , Rüdiger , Vodafone Group]

> -Ursprüngliche Nachricht-
> Von: Yann Ylavic [mailto:ylavic@gmail.com]
> Gesendet: Mittwoch, 14. Dezember 2016 01:42
> An: httpd-dev 
> Betreff: 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).

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 @@ 

Re: Absorb win32-apxs into httpd distro?

[William A Rowe Jr]

On Wed, Dec 14, 2016 at 2:20 AM, Steve Hay 
wrote:

> On 14 December 2016 at 08:13, William A Rowe Jr 
> wrote:
> > Randy wrote http://www.apache.org/dist/perl/win32-bin/ - but I'm
> wondering
> > who else here at httpd is interested in helping maintain and get this
> code
> > into our own distribution? I've shipped this for a decade for my
> commercial
> > customers, for every wrinkle and wart, I think many of our win32 users
> > would appreciate this.
> >
> > I trust the perl pmc and Randy would be good with this adoption?
> >
>
> Randy sadly passed away some years ago, but I for one would welcome
> the adoption.
>

I was aware. Seemed like one more small appropriate tribute.

The latest version that I'm aware of lives in
> https://svn.apache.org/repos/asf/perl/apxs/trunk
>

Thanks for the pointer, and support of bringing this into the httpd distro.
Hoping you (and other perl-folk who know the code) might help review
the patches and ensure it is working well, moving forwards.

Re: Absorb win32-apxs into httpd distro?

[Steve Hay]

On 14 December 2016 at 08:13, William A Rowe Jr  wrote:
> Randy wrote http://www.apache.org/dist/perl/win32-bin/ - but I'm wondering
> who else here at httpd is interested in helping maintain and get this code
> into our own distribution? I've shipped this for a decade for my commercial
> customers, for every wrinkle and wart, I think many of our win32 users
> would appreciate this.
>
> I trust the perl pmc and Randy would be good with this adoption?
>

Randy sadly passed away some years ago, but I for one would welcome
the adoption.

The latest version that I'm aware of lives in
https://svn.apache.org/repos/asf/perl/apxs/trunk

Absorb win32-apxs into httpd distro?

[William A Rowe Jr]

Randy wrote http://www.apache.org/dist/perl/win32-bin/ - but I'm wondering
who else here at httpd is interested in helping maintain and get this code
into our own distribution? I've shipped this for a decade for my commercial
customers, for every wrinkle and wart, I think many of our win32 users
would appreciate this.

I trust the perl pmc and Randy would be good with this adoption?

Cheers,

Bill