Re: svn commit: r1706275 - /httpd/httpd/trunk/modules/ssl/ssl_engine_io.c

[Joe Orton]

On Tue, Oct 06, 2015 at 02:37:32PM +, Plüm, Rüdiger, Vodafone Group wrote:
> The drawback is that it will flush every time the handshake writes.
> The handshake may write multiple times before it wants to read.
> So the current approach probably causes bigger amounts of data sent
> across the wire at once then the approach below and thus is more in line with 
> the standard
> approach in httpd to avoid sending small chunks if not needed.
> So I would keep the current approach.

Looking at 1.0.1e, the state machine loop in ssl3_accept(), the coding 
style is pretty clearly:

1. send something
2. flush
3. switch to new state.

e.g. like this

ret=ssl3_send_hello_request(s);
if (ret <= 0) goto end;
s->s3->tmp.next_state=SSL3_ST_SW_HELLO_REQ_C;
s->state=SSL3_ST_SW_FLUSH;

...and then the SW_FLUSH mode switches to the tmp.next_state.

Hence In the server case, it seems reasonable to rely on BIO_flush() 
being called at the "right" times during the handshake.  Modulo the odd 
bug!

But ssl/s3_clnt.c is not following that coding style at all, and it only 
does a flush after completing the handshake.  So I'd say the right thing 
here is to FLUSH after every packet which comes through the write BIO 
when the SSL state machine is in the middle of a "connect", i.e. 
handshake as client.

tl;dr: I think Yann's patch should be right if the test is switched from 
"always flush if !SSL_is_init_finished(ssl)" to "always flush if 
SSL_in_connect_init(ssl)"???

Regards, Joe

Re: svn commit: r1704683 - /httpd/httpd/trunk/docs/manual/mod/mod_remoteip.xml

[Eric Covener]

On Tue, Sep 22, 2015 at 11:09 PM, William A Rowe Jr  wrote:
> On Tue, Sep 22, 2015 at 8:48 PM, Eric Covener  wrote:
>>
>> Maybe my followup is better phrased.  No issue with handling of internal
>> IPs.
>>
>> Currently, we act like RemoteIPTrustedProxy * by default (once they've
>> named the XFF header) and warn people they'd better restrict it.
>
>
> I agree that was not the original design and we should address it with a fix
> rather than a docs fix, IMHO.  'Trusted' is the exception, not the general
> case.

bump. I don't love the idea of changing the 2.4 defaults.

Current doc already says "Unless these other directives are used,
mod_remoteip will trust all hosts presenting a RemoteIPHeader IP
value." so I thought it was wise  to reinforce this in other sections.
  Doc is not back-ported yet.

Re: svn commit: r1706669 - in /httpd/httpd/trunk: ./ include/ modules/http/ modules/ssl/ server/ server/mpm/event/ server/mpm/motorz/ server/mpm/simple/

[Yann Ylavic]

On Tue, Oct 6, 2015 at 5:34 PM, Graham Leggett  wrote:
>
> apr_bucket_simple_copy() looks wrong - in theory we should have a proper copy 
> function that does the right thing with the second copy, for example by not 
> copying the pool. If we blindly copy the pool (or the request containing the 
> pool) I see nothing that would prevent an attempt to free the pool twice.

Agreed, we probably need something like this:

Index: server/eor_bucket.c
===
--- server/eor_bucket.c(revision 1707064)
+++ server/eor_bucket.c(working copy)
@@ -91,6 +91,17 @@ static void eor_bucket_destroy(void *data)
 }
 }

+static apr_status_t eor_bucket_copy(apr_bucket *a, apr_bucket **b)
+{
+*b = apr_bucket_alloc(sizeof(**b), a->list); /* XXX: check for failure? */
+**b = *a;
+
+/* we don't wan't request to be destroyed twice */
+(*b)->data = NULL;
+
+return APR_SUCCESS;
+}
+
 AP_DECLARE_DATA const apr_bucket_type_t ap_bucket_type_eor = {
 "EOR", 5, APR_BUCKET_METADATA,
 eor_bucket_destroy,
@@ -97,6 +108,6 @@ AP_DECLARE_DATA const apr_bucket_type_t ap_bucket_
 eor_bucket_read,
 apr_bucket_setaside_noop,
 apr_bucket_split_notimpl,
-apr_bucket_simple_copy
+eor_bucket_copy
 };

--

Regards,
Yann.

Re: svn commit: r1706669 - in /httpd/httpd/trunk: ./ include/ modules/http/ modules/ssl/ server/ server/mpm/event/ server/mpm/motorz/ server/mpm/simple/

[Yann Ylavic]

On Tue, Oct 6, 2015 at 3:51 PM, Yann Ylavic  wrote:
> On Tue, Oct 6, 2015 at 3:41 PM, Graham Leggett  wrote:
>> On 6 Oct 2015, at 14:32, Yann Ylavic  wrote:
>>
>>> So it seems to relate to the EOR bucket, first not being passed
>>> through, and second leading to a double-free or alike.
>>>
>>> Did not investigate further...
>>
>> Did you rebuild you tree completely clean before trying it out?
>>
>> Whenever I got weird behavior a clean rebuild sorted it out. The full test 
>> suite works fine for me using the event mpm.
>
> Just did that, same thing.
> I was using mpm_worker, but now tried mpm_event with same segfaults.

Looks like mod_bucketeer is the culprit.
It fails to remove itself from the filter stack on EOS, and hence
makes a copy of the EOR bucket (as any METADATA bucket) for its own
brigade when it sees it.
Then the request gets destroyed twice (once per brigade it is in) on cleanup.

I fail to see where your patch comes into play here though, for now.
I also suspect EOR buckets shouldn't be copyable anyway, and bucketeer
should do the right thing, still the new filter should probably work
with the existing...

Re: svn commit: r1706669 - in /httpd/httpd/trunk: ./ include/ modules/http/ modules/ssl/ server/ server/mpm/event/ server/mpm/motorz/ server/mpm/simple/

[Graham Leggett]

On 06 Oct 2015, at 3:36 PM, Stefan Eissing  wrote:

> FYI: /trunk no longer works with mod_http2. 2.4.x does. I see missing 
> response data, it seems, so the most likely cause are the changes in filter 
> handling. Did not find the time to investigate further.
> 
> Please be aware the mod_h2 uses pool/brigade hierarchies in new and 
> unexpected ways. The request has been processed and EOR bucket has been 
> handled in another thread, however data/file handles might have been 
> transferred to the main thread and are being cleaned up when the main 
> connection thinks a stream is done. If those data gets set aside, the memory 
> might get freed/reused too early. Maybe mod_http2 needs to come up with its 
> own EOR buckets on the main connection to allow for cleanup synchronization. 
> Not sure…

I pulled my hair out looking for the exact same problem.

It turned out to be a bug in the core where the EOR bucket was being injected 
directly into the connection filters and thus bypassing request filters 
containing setaside data. When the EOR bucket “overtakes” other buckets, the 
data vanishes.

The solution is to make sure that the EOR bucket genuinely does get injected at 
the very end of the request. To do this I found the last internal redirected 
request (iterate through r->next until r->next is NULL) and injected the EOR 
bucket into that filter.

(I have found a problem where we’re injecting an EOR bucket made out of the 
last request instead of the main request, will fix that)

A quick scan of the modules/http2 directory shows no calls to 
ap_bucket_eor_create(), I suspect you might be relying on http1.1 functionality 
to do this in modules/http/http_request.c. Ideally mod_http2 should be choosing 
the right time to end the request based on the http2 protocol.

Regards,
Graham
—

AW: svn commit: r1706275 - /httpd/httpd/trunk/modules/ssl/ssl_engine_io.c

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

> -Ursprüngliche Nachricht-
> Von: Yann Ylavic [mailto:ylavic@gmail.com]
> Gesendet: Dienstag, 6. Oktober 2015 16:06
> An: dev@httpd.apache.org
> Betreff: Re: svn commit: r1706275 -
> /httpd/httpd/trunk/modules/ssl/ssl_engine_io.c
> 
> On Thu, Oct 1, 2015 at 8:22 PM, Ruediger Pluem 
> wrote:
> >
> > The issue is that openssl during the connect handshake to a clieent
> does not tell httpd to flush. Hence the CLIENT_HELLO
> > remains in the core output filter buffer and openssl waits for the
> SERVER_HELLO from the remote server which of course
> > does not happen without the CLIENT_HELLO having been sent there.
> >
> 
> I also tried the following patch which also passes the test framework
> and is maybe more straightforward since it flushes on write (during
> handshake only), thus avoiding any flush (and round-trip) on read.
> 
> WDYT?

The drawback is that it will flush every time the handshake writes.
The handshake may write multiple times before it wants to read.
So the current approach probably causes bigger amounts of data sent
across the wire at once then the approach below and thus is more in line with 
the standard
approach in httpd to avoid sending small chunks if not needed.
So I would keep the current approach.

Regards

Rüdiger

Re: svn commit: r1706669 - in /httpd/httpd/trunk: ./ include/ modules/http/ modules/ssl/ server/ server/mpm/event/ server/mpm/motorz/ server/mpm/simple/

[Graham Leggett]

On 06 Oct 2015, at 5:22 PM, Yann Ylavic  wrote:

>> Just did that, same thing.
>> I was using mpm_worker, but now tried mpm_event with same segfaults.
> 
> Looks like mod_bucketeer is the culprit.
> It fails to remove itself from the filter stack on EOS, and hence
> makes a copy of the EOR bucket (as any METADATA bucket) for its own
> brigade when it sees it.
> Then the request gets destroyed twice (once per brigade it is in) on cleanup.
> 
> I fail to see where your patch comes into play here though, for now.
> I also suspect EOR buckets shouldn't be copyable anyway, and bucketeer
> should do the right thing, still the new filter should probably work
> with the existing...

The core used to inject EOR buckets into the connection filters, bypassing the 
request filters. Now that request filters can buffer data, injecting EOR into 
the connection filters causes the end of requests to be chopped off, so now EOR 
is injected at the very end of the last request filter, as it should have been.

In theory none of this should matter, if EOR is copied it should set up the 
proper cleanups to clean up the second copy. A quick look at eor_bucket.c shows 
the following:

AP_DECLARE_DATA const apr_bucket_type_t ap_bucket_type_eor = {
"EOR", 5, APR_BUCKET_METADATA,
eor_bucket_destroy,
eor_bucket_read,
apr_bucket_setaside_noop,
apr_bucket_split_notimpl,
apr_bucket_simple_copy
};

apr_bucket_simple_copy() looks wrong - in theory we should have a proper copy 
function that does the right thing with the second copy, for example by not 
copying the pool. If we blindly copy the pool (or the request containing the 
pool) I see nothing that would prevent an attempt to free the pool twice.

Regards,
Graham
—

AW: svn commit: r1706275 - /httpd/httpd/trunk/modules/ssl/ssl_engine_io.c

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

> -Ursprüngliche Nachricht-
> Von: Yann Ylavic [mailto:ylavic@gmail.com]
> Gesendet: Dienstag, 6. Oktober 2015 18:18
> An: dev@httpd.apache.org
> Betreff: Re: svn commit: r1706275 -
> /httpd/httpd/trunk/modules/ssl/ssl_engine_io.c
> 
> On Tue, Oct 6, 2015 at 6:00 PM, Yann Ylavic 
> wrote:
> > On Tue, Oct 6, 2015 at 5:44 PM, Joe Orton  wrote:
> >>
> >> Hence In the server case, it seems reasonable to rely on BIO_flush()
> >> being called at the "right" times during the handshake.  Modulo the
> odd
> >> bug!
> >>
> >> But ssl/s3_clnt.c is not following that coding style at all, and it
> only
> >> does a flush after completing the handshake.  So I'd say the right
> thing
> >> here is to FLUSH after every packet which comes through the write BIO
> >> when the SSL state machine is in the middle of a "connect", i.e.
> >> handshake as client.
> >>
> >> tl;dr: I think Yann's patch should be right if the test is switched
> from
> >> "always flush if !SSL_is_init_finished(ssl)" to "always flush if
> >> SSL_in_connect_init(ssl)"???
> >
> > Yes, I came to the same conclusion, but decided to use
> > SSL_is_init_finished(ssl) anyway because for the server case it seems
> > that openssl uses it own buffering mechanism to avoid writing small
> > chunks (after the client-hello is received), so possibly we could rely
> > on it (this also simplifies the logic).
> 
> Also it seems that for the SSL_ST_RENEGOTIATE state in ssl3_accept(),
> buffering is disabled by openssl (at least in 1.0.2d).
> SSL_is_init_finished(ssl) should catch this case too...

I am confused now. I understood that the state machine for the server case is 
fine. Hence that it flushes automatically where needed. So we only should and 
need to take care of the client case.
How does using !SSL_is_init_finished(ssl) simplifies the logic?


Regards

Rüdiger

AW: svn commit: r1706669 - in /httpd/httpd/trunk: ./ include/ modules/http/ modules/ssl/ server/ server/mpm/event/ server/mpm/motorz/ server/mpm/simple/

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

> -Ursprüngliche Nachricht-
> Von: Yann Ylavic [mailto:ylavic@gmail.com]
> Gesendet: Dienstag, 6. Oktober 2015 17:54
> An: dev@httpd.apache.org
> Betreff: Re: svn commit: r1706669 - in /httpd/httpd/trunk: ./ include/
> modules/http/ modules/ssl/ server/ server/mpm/event/ server/mpm/motorz/
> server/mpm/simple/
> 
> On Tue, Oct 6, 2015 at 5:34 PM, Graham Leggett  wrote:
> >
> > apr_bucket_simple_copy() looks wrong - in theory we should have a
> proper copy function that does the right thing with the second copy, for
> example by not copying the pool. If we blindly copy the pool (or the
> request containing the pool) I see nothing that would prevent an attempt
> to free the pool twice.
> 
> Agreed, we probably need something like this:
> 
> Index: server/eor_bucket.c
> ===
> --- server/eor_bucket.c(revision 1707064)
> +++ server/eor_bucket.c(working copy)
> @@ -91,6 +91,17 @@ static void eor_bucket_destroy(void *data)
>  }
>  }
> 
> +static apr_status_t eor_bucket_copy(apr_bucket *a, apr_bucket **b)
> +{
> +*b = apr_bucket_alloc(sizeof(**b), a->list); /* XXX: check for
> failure? */
> +**b = *a;
> +

We could use apr_bucket_simple_copy(a, b) instead of the above.

> +/* we don't wan't request to be destroyed twice */
> +(*b)->data = NULL;

Hm. Shouldn't the last EOR bucket of a particular request destroyed call
eor_bucket_cleanup? That would require some kind of a reference like refcount 
buckets provide.

> +
> +return APR_SUCCESS;
> +}
> +
>  AP_DECLARE_DATA const apr_bucket_type_t ap_bucket_type_eor = {
>  "EOR", 5, APR_BUCKET_METADATA,
>  eor_bucket_destroy,
> @@ -97,6 +108,6 @@ AP_DECLARE_DATA const apr_bucket_type_t ap_bucket_


Regards

Rüdiger

Re: svn commit: r1706669 - in /httpd/httpd/trunk: ./ include/ modules/http/ modules/ssl/ server/ server/mpm/event/ server/mpm/motorz/ server/mpm/simple/

[Jim Jagielski]

> On Oct 6, 2015, at 1:29 PM, Graham Leggett  wrote:
> 
> On 06 Oct 2015, at 7:00 PM, Yann Ylavic  wrote:
> 
>> Yet another filter which does not remove itself after the EOS, and
>> destroys the EOR bucket while still using *r after.
> 
> I am wondering if the EOR bucket is a suboptimal way to clean up after 
> ourselves.
> 
> The MPM itself is in a position to know when the filter buffers are all empty 
> and it is safe to delete a request pool. I am imagining a “shutdown set”, 
> which contains the pools which have been marked as ready to shut down. If 
> there is a pool in the shutdown set, walk the outstanding filters and look 
> for any filters whose pools are descendants of that pool, and check if the 
> filters are empty. If so, apr_pool_destroy() and we’re done. If the “shutdown 
> set” is empty, do nothing.
> 
> Instead of creating an EOR bucket, we call an MPM API that says “add this 
> pool to the shutdown set”, and then forget about it.
> 
> Legacy MPMs fall back to EOR behaviour, but we deprecate it.
> 
> I think the above is a far safer approach than assuming filters will “do the 
> right thing”.
> 

Basic idea seems sound... +1

RE: svn commit: r1706669 - in /httpd/httpd/trunk: ./ include/ modules/http/ modules/ssl/ server/ server/mpm/event/ server/mpm/motorz/ server/mpm/simple/

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

> -Original Message-
> From: Yann Ylavic [mailto:ylavic@gmail.com]
> Sent: Dienstag, 6. Oktober 2015 01:40
> To: dev@httpd.apache.org
> Subject: Re: svn commit: r1706669 - in /httpd/httpd/trunk: ./ include/
> modules/http/ modules/ssl/ server/ server/mpm/event/ server/mpm/motorz/
> server/mpm/simple/
> 
> > Modified: httpd/httpd/trunk/server/mpm/event/event.c
> > URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/server/mpm/event/event.c?re
> v=1706669=1706668=1706669=diff
> >
> ==
> 
> > --- httpd/httpd/trunk/server/mpm/event/event.c (original)
> > +++ httpd/httpd/trunk/server/mpm/event/event.c Sun Oct  4 10:10:51 2015
> > @@ -1146,19 +1146,38 @@ read_request:
> >  }
> >
> []
> > +
> > +rindex = apr_hash_first(NULL, c->filters);
> > +while (rindex) {
> > +ap_filter_t *f = apr_hash_this_val(rindex);
> > +
> > +if (!APR_BRIGADE_EMPTY(f->bb)) {
> > +
> > +rv = ap_pass_brigade(f, c->empty);
> > +apr_brigade_cleanup(c->empty);
> > +if (APR_SUCCESS != rv) {
> > +ap_log_cerror(
> > +APLOG_MARK, APLOG_DEBUG, rv, c,
> APLOGNO(00470)
> > +"write failure in '%s' output filter", f-
> >frec->name);
> > +break;
> > +}
> > +
> > +if (ap_filter_should_yield(f)) {
> > +data_in_output_filters = 1;
> > +}
> > +}
> > +
> > +rindex = apr_hash_next(rindex);
> >  }
> 
> This pattern looks shared by all (relevant) MPMs, couldn't we make it
> a function? Maybe ap_reinstate_conn()?

Sounds sensible.

> 
> Also it seems that we leak the hash iterator here (on c->pool).

Why do we leak here? apr_hash_first(NULL, is used. So no new iterator is 
allocated.

> Couldn't we use a (single) linked list for c->filters since
> ap_filter_t already has a 'next' field?
> 
> >
> > Modified: httpd/httpd/trunk/server/util_filter.c
> > URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/server/util_filter.c?rev=17
> 06669=1706668=1706669=diff
> >
> ==
> 
> > --- httpd/httpd/trunk/server/util_filter.c (original)
> > +++ httpd/httpd/trunk/server/util_filter.c Sun Oct  4 10:10:51 2015
> []
> > @@ -635,7 +653,8 @@ AP_DECLARE(apr_status_t) ap_save_brigade
> >  apr_status_t rv, srv = APR_SUCCESS;
> >
> >  /* If have never stored any data in the filter, then we had better
> > - * create an empty bucket brigade so that we can concat.
> > + * create an empty bucket brigade so that we can concat. Register
> > + * a cleanup to zero out the pointer if the pool is cleared.
> 
> Maybe this comment belongs in ap_filter_setaside_brigade()?
> 
> >   */
> >  if (!(*saveto)) {
> >  *saveto = apr_brigade_create(p, f->c->bucket_alloc);
> > @@ -673,6 +692,248 @@ AP_DECLARE(apr_status_t) ap_save_brigade
> >  return srv;
> >  }
> >
> > +static apr_status_t filters_cleanup(void *data)
> > +{
> > +ap_filter_t **key = data;
> > +
> > +apr_hash_set((*key)->c->filters, key, sizeof(ap_filter_t **),
> NULL);
> > +
> > +return APR_SUCCESS;
> > +}
> 
> Shouldn't we call filters_cleanup() from ap_remove_output_filter() too?

I think so.

> > +
> > +f->bb = apr_brigade_create(pool, f->c->bucket_alloc);
> > +
> > +apr_pool_pre_cleanup_register(pool, key, filters_cleanup);
> > +
> > +}
> > +
> > +/* decide what pool we setaside to, request pool or deferred
> pool? */
> > +if (f->r) {
> > +pool = f->r->pool;
> > +APR_BRIGADE_CONCAT(f->bb, bb);
> > +}
> > +else {
> > +if (!f->deferred_pool) {
> > +apr_pool_create(>deferred_pool, f->c->pool);
> > +apr_pool_tag(f->deferred_pool, "deferred_pool");
> > +}
> > +pool = f->deferred_pool;
> > +return ap_save_brigade(f, >bb, , pool);
> > +}
> 
> Shouldn't ap_save_brigade() be moved below the "else"?

Looks like it should, plus removing APR_BRIGADE_CONCAT(f->bb, bb); above.
I think we need to set aside the buckets of bb even if we have f->r as bb may
contain transient buckets.

Regards

Rüdiger

Re: svn commit: r1703305 - /httpd/httpd/trunk/modules/aaa/mod_auth_digest.c

[Marion & Christophe JAILLET]

Le 05/10/2015 12:03, Plüm, Rüdiger, Vodafone Group a écrit :



-Original Message-
From: Marion & Christophe JAILLET [mailto:christophe.jail...@wanadoo.fr]
Sent: Samstag, 3. Oktober 2015 21:57
To: dev@httpd.apache.org
Subject: Re: svn commit: r1703305 -
/httpd/httpd/trunk/modules/aaa/mod_auth_digest.c


Le 01/10/2015 20:32, Ruediger Pluem a écrit :

On 09/16/2015 12:20 AM, jaillet...@apache.org wrote:

Author: jailletc36
Date: Tue Sep 15 22:20:45 2015
New Revision: 1703305

URL: http://svn.apache.org/r1703305
Log:
Remove code related to 'AuthDigestEnableQueryStringHack'

This has been undocumented for about 3 years now (see r1415960)

Modified:
  httpd/httpd/trunk/modules/aaa/mod_auth_digest.c

Could this cause

t/modules/digest.t

test 9 to fail?

As we removed the code for good reason we should also remove the test if

this is the case then.

Regards

Rüdiger

Hi,
yes, you are right.

I would proposed the below patch:

+1


Regards

Rüdiger



Done in r1706952

CJ

Re: svn commit: r1706669 - in /httpd/httpd/trunk: ./ include/ modules/http/ modules/ssl/ server/ server/mpm/event/ server/mpm/motorz/ server/mpm/simple/

[Graham Leggett]

On 06 Oct 2015, at 7:29 PM, Graham Leggett  wrote:

> I am wondering if the EOR bucket is a suboptimal way to clean up after 
> ourselves.
> 
> The MPM itself is in a position to know when the filter buffers are all empty 
> and it is safe to delete a request pool. I am imagining a “shutdown set”, 
> which contains the pools which have been marked as ready to shut down. If 
> there is a pool in the shutdown set, walk the outstanding filters and look 
> for any filters whose pools are descendants of that pool, and check if the 
> filters are empty. If so, apr_pool_destroy() and we’re done. If the “shutdown 
> set” is empty, do nothing.
> 
> Instead of creating an EOR bucket, we call an MPM API that says “add this 
> pool to the shutdown set”, and then forget about it.
> 
> Legacy MPMs fall back to EOR behaviour, but we deprecate it.
> 
> I think the above is a far safer approach than assuming filters will “do the 
> right thing”.

Having done some further digging that isn’t that workable. While the presence 
of a filter with setaside data in it tells us definitely that we shouldn’t 
destroy the brigade yet, it is very difficult to determine when we should.

I think we’ll need to compromise and provide an async switch that in v2.4 
defaults to “connection” (restrict async behaviour to connection filters only), 
and can be optionally set to “request” (async behaviour for connection and 
request filters) if the filters in use allow it.

Regards,
Graham
—

Re: svn commit: r1706669 - in /httpd/httpd/trunk: ./ include/ modules/http/ modules/ssl/ server/ server/mpm/event/ server/mpm/motorz/ server/mpm/simple/

[Yann Ylavic]

On Tue, Oct 6, 2015 at 9:37 AM, Plüm, Rüdiger, Vodafone Group
 wrote:
>
>> From: Yann Ylavic [mailto:ylavic@gmail.com]
>>
>> Also it seems that we leak the hash iterator here (on c->pool).
>
> Why do we leak here? apr_hash_first(NULL, is used. So no new iterator is 
> allocated.

Correct, I thought apr_hash_first(NULL, ...) was implicitly using
hash->pool to allocate an iterator, but that's not the case (it's a
struct member).

>> Couldn't we use a (single) linked list for c->filters since
>> ap_filter_t already has a 'next' field?

So using an apr_hash_t is indeed better.

Regards,
Yann.

Re: svn commit: r1706669 - in /httpd/httpd/trunk: ./ include/ modules/http/ modules/ssl/ server/ server/mpm/event/ server/mpm/motorz/ server/mpm/simple/

[Yann Ylavic]

On Tue, Oct 6, 2015 at 9:37 AM, Plüm, Rüdiger, Vodafone Group
 wrote:
>
>> -Original Message-
>> From: Yann Ylavic [mailto:ylavic@gmail.com]
>>
>> > Modified: httpd/httpd/trunk/server/mpm/event/event.c
>> > URL:
>> http://svn.apache.org/viewvc/httpd/httpd/trunk/server/mpm/event/event.c?re
>> v=1706669=1706668=1706669=diff
>> >
>> ==
>> 
>> > --- httpd/httpd/trunk/server/mpm/event/event.c (original)
>> > +++ httpd/httpd/trunk/server/mpm/event/event.c Sun Oct  4 10:10:51 2015
>> > @@ -1146,19 +1146,38 @@ read_request:
>> >  }
>> >
>> []
>> > +
>> > +rindex = apr_hash_first(NULL, c->filters);
>> > +while (rindex) {
>> > +ap_filter_t *f = apr_hash_this_val(rindex);
>> > +
>> > +if (!APR_BRIGADE_EMPTY(f->bb)) {
>> > +
>> > +rv = ap_pass_brigade(f, c->empty);
>> > +apr_brigade_cleanup(c->empty);
>> > +if (APR_SUCCESS != rv) {
>> > +ap_log_cerror(
>> > +APLOG_MARK, APLOG_DEBUG, rv, c,
>> APLOGNO(00470)
>> > +"write failure in '%s' output filter", f-
>> >frec->name);
>> > +break;
>> > +}
>> > +
>> > +if (ap_filter_should_yield(f)) {
>> > +data_in_output_filters = 1;
>> > +}
>> > +}
>> > +
>> > +rindex = apr_hash_next(rindex);
>> >  }
>>
>> This pattern looks shared by all (relevant) MPMs, couldn't we make it
>> a function? Maybe ap_reinstate_conn()?
>
> Sounds sensible.

By defining this function in util_filter
(ap_filter_reinstate_conn()?), we also possibly could get rid of
c->empty (which looks quite hacky), using/hidding an empty brigade per
filter (in opaque data).

Regards,
Yann.

Re: Supporting "SSL:" in the expression parser via mod_ssl

[Rainer Jung]

Am 06.10.2015 um 00:44 schrieb Stefan Fritsch:

On Wednesday 30 September 2015 23:26:30, Rainer Jung wrote:

I noticed that currently the expression parser in 2.4/trunk does not
support the SSL:VARIABLE lookups that mod_rewrite supports.

The expression parser uses ":" as an alternative function call
syntax, so HTTP:VARIABLE is the same as HTTP(VARIABLE) which in
turn executes http(VARIABLE). The same is true for ENV:VARIABLE
which maps to env(VARIABLE) etc.

We already do support the syntax SSL_VARIABLE to look up SSL
variables, because mod_ssl registers in the expression parser for
variables whose name starts with "SSL_". But mod_ssl does not
register for a function named SSL (or ssl), so the SSL: notation
does not work.

Is that just an omission, or was that intentional? If not
intentional, I would apply the following (yet untested) simple
patch to trunk mod_ssl and propose for backport. It registers the
SSL (and ssl) function in the expression parser:


I think that was just an omission. +1 to your patch.


I tested it an it would work, but one implementation detail remains to 
discuss. The %{SSL:variable} syntax goes back to mod_rewrite. There 
"variable" is the full name of the mod_ssl variable, i.e.


   %{SSL:SSL_PROTOCOL}

mod_rewrite looked up varname by calling ssl_var_lookup() which itself 
resolves lots of variables, even many non-ssl ones. In the ssl case it 
quickly falls through and then strips the "SSL_" prefix and calls 
ssl_var_lookup_ssl().


For 2.4 and the nes %{SSL:variable} impl in the expr parser we have 
three options:


1) Support only variable name with the "SSL_" prefix already stripped 
and call ssl_var_lookup_ssl().


Example: %{SSL:PROTOCOL}

There's less redundancy in the notation, but it is incompatible with 
mod_rewrite's use of %{SSL:variable}. Therefore I'd say it is too 
confusing and would be -1 to this option.


2) Demand including the "SSL_" prefix in the variable name, strip it 
when resolving the value and call ssl_var_lookup_ssl() for the shortened 
name. Throw error if the variable name does not start with "SSL_".


Example: %{SSL:SSL_PROTOCOL}

This is compatible with mod_rewrite, but only as long as you really use 
a variable whose name begins with "SSL_". I guess mostly this will be 
the case, but people might have used e.g. %{SSL:HTTPS}, which would now 
throw an error.


3) Support any mod_ssl variable. Demand using the original full name and 
call ssl_var_lookup() with the full name.


Example: %{SSL:SSL_PROTOCOL}

This is the most compatible and complete solution. The only drawback is 
the use of ssl_var_lookup() which adds a bit of overhead for the general 
case and itself seems to be a candidate for long term replacement by 
ap_expr. At that point in the future it would bite us to let ap_expr 
base its SSL: implementation on a function, that could be replaced by 
ap_expr itself (and ssl_var_lookup_ssl()). Nevertheless I would vote for 
3) as the solution to chose.


I will commit 3) but are very open to other opinions before suggesting a 
backport.


Regards,

Rainer

RE: svn commit: r1706669 - in /httpd/httpd/trunk: ./ include/ modules/http/ modules/ssl/ server/ server/mpm/event/ server/mpm/motorz/ server/mpm/simple/

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

> -Original Message-
> From: Yann Ylavic [mailto:ylavic@gmail.com]
> Sent: Dienstag, 6. Oktober 2015 11:13
> To: dev@httpd.apache.org
> Subject: Re: svn commit: r1706669 - in /httpd/httpd/trunk: ./ include/
> modules/http/ modules/ssl/ server/ server/mpm/event/ server/mpm/motorz/
> server/mpm/simple/
> 
> On Tue, Oct 6, 2015 at 9:37 AM, Plüm, Rüdiger, Vodafone Group
>  wrote:
> >
> >> -Original Message-
> >> From: Yann Ylavic [mailto:ylavic@gmail.com]
> >>
> >> > Modified: httpd/httpd/trunk/server/mpm/event/event.c
> >> > URL:
> >>
> http://svn.apache.org/viewvc/httpd/httpd/trunk/server/mpm/event/event.c?re
> >> v=1706669=1706668=1706669=diff
> >> >
> >>
> ==
> >> 
> >> > --- httpd/httpd/trunk/server/mpm/event/event.c (original)
> >> > +++ httpd/httpd/trunk/server/mpm/event/event.c Sun Oct  4 10:10:51
> 2015
> >> > @@ -1146,19 +1146,38 @@ read_request:
> >> >  }
> >> >
> >> []
> >> > +
> >> > +rindex = apr_hash_first(NULL, c->filters);
> >> > +while (rindex) {
> >> > +ap_filter_t *f = apr_hash_this_val(rindex);
> >> > +
> >> > +if (!APR_BRIGADE_EMPTY(f->bb)) {
> >> > +
> >> > +rv = ap_pass_brigade(f, c->empty);
> >> > +apr_brigade_cleanup(c->empty);
> >> > +if (APR_SUCCESS != rv) {
> >> > +ap_log_cerror(
> >> > +APLOG_MARK, APLOG_DEBUG, rv, c,
> >> APLOGNO(00470)
> >> > +"write failure in '%s' output filter",
> f-
> >> >frec->name);
> >> > +break;
> >> > +}
> >> > +
> >> > +if (ap_filter_should_yield(f)) {
> >> > +data_in_output_filters = 1;
> >> > +}
> >> > +}
> >> > +
> >> > +rindex = apr_hash_next(rindex);
> >> >  }
> >>
> >> This pattern looks shared by all (relevant) MPMs, couldn't we make it
> >> a function? Maybe ap_reinstate_conn()?
> >
> > Sounds sensible.
> 
> By defining this function in util_filter
> (ap_filter_reinstate_conn()?), we also possibly could get rid of
> c->empty (which looks quite hacky), using/hidding an empty brigade per
> filter (in opaque data).

Although the brigade is empty, don't we need to prevent the brigade being used 
by multiple threads in parallel?
Using one per connection prevents this.

Regards

Rüdiger

Re: Supporting "SSL:" in the expression parser via mod_ssl

[Yann Ylavic]

On Tue, Oct 6, 2015 at 11:55 AM, Rainer Jung  wrote:
>
> I will commit 3) but are very open to other opinions before suggesting a
> backport.

+1

Regards,
Yann.

RE: Supporting "SSL:" in the expression parser via mod_ssl

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

+1 

Regards

Rüdiger

> -Original Message-
> From: Yann Ylavic [mailto:ylavic@gmail.com]
> Sent: Dienstag, 6. Oktober 2015 12:07
> To: dev@httpd.apache.org
> Subject: Re: Supporting "SSL:" in the expression parser via mod_ssl
> 
> On Tue, Oct 6, 2015 at 11:55 AM, Rainer Jung 
> wrote:
> >
> > I will commit 3) but are very open to other opinions before suggesting a
> > backport.
> 
> +1
> 
> Regards,
> Yann.

Re: svn commit: r1706669 - in /httpd/httpd/trunk: ./ include/ modules/http/ modules/ssl/ server/ server/mpm/event/ server/mpm/motorz/ server/mpm/simple/

[Graham Leggett]

On 06 Oct 2015, at 1:39 AM, Yann Ylavic  wrote:

>> +
>> +/** Buffered data associated with the current filter. */
>> +apr_bucket_brigade *bb;
>> +
>> +/** Dedicated pool to use for deferred writes. */
>> +apr_pool_t *deferred_pool;
> 
> Could we make these opaque, eg:
> 
> # util_filter.h:
> struct ap_filter_data;
> struct ap_filter_t {
>...
>struct ap_filter_data *data;
> };
> 
> # util_filter.c:
> struct ap_filter_data
> {
>   /** Buffered data associated with the current filter. */
>   apr_bucket_brigade *bb;
> 
>   /** Dedicated pool to use for deferred writes. */
>   apr_pool_t *deferred_pool;
> };
> 
> This would prevent them from being mangled anywhere (they are
> internals to util_filter.c anyway).

There weren’t originally internal to util_filter.c, it’s only subsequently 
turned out that way. Doing that comes at a cost of a further 8 bytes per filter 
per request through, which starts adding up.

I would rather finish the filter suspension work before doing any optimising 
like this, it’s too soon at this stage.

> It could also possibly avoid walking the brigade for every
> ap_filter_reinstate_brigade() call…

Unfortunately you can’t avoid walking the brigade in the 
ap_filter_reinstate_brigade(), as this is the flow control logic that keeps the 
server safe from consuming too much data.

Fortunately this loop only really does anything at the bottleneck point in the 
filter stack, which is typically the first filter that does any meaningful 
work. After this point buckets come down the filter stack one bucket at a time, 
and the loop becomes one big if statement for subsequent filters.

> This pattern looks shared by all (relevant) MPMs, couldn't we make it
> a function? Maybe ap_reinstate_conn()?

Eventually yes.

For reasons I wasn’t sure of, each MPM was subtly different to the next in this 
loop. I want to first investigate why they work differently before baking this 
behaviour in.

I also want to solve the filter suspend problem before trying to optimise like 
this.

> Also it seems that we leak the hash iterator here (on c->pool).

We don’t, when you create an iterator with a NULL pool it uses an iterator 
internal to the hash. If we passed c->pool to apr_hash_first(), we would leak.

> Couldn't we use a (single) linked list for c->filters since
> ap_filter_t already has a 'next' field?

We can’t alas, as c->filters is not a linked list but rather a set.

Most specifically, it is the set of filters that have set aside data. Setting 
aside data is optional, some filters will elect not to do so, and synchronous 
filters will definitely not do so.

Filters in this set are eligible for being kicked. If you’re not in the set, 
you get left alone.

>> @@ -635,7 +653,8 @@ AP_DECLARE(apr_status_t) ap_save_brigade
>> apr_status_t rv, srv = APR_SUCCESS;
>> 
>> /* If have never stored any data in the filter, then we had better
>> - * create an empty bucket brigade so that we can concat.
>> + * create an empty bucket brigade so that we can concat. Register
>> + * a cleanup to zero out the pointer if the pool is cleared.
> 
> Maybe this comment belongs in ap_filter_setaside_brigade()?

Will check.

>> +static apr_status_t filters_cleanup(void *data)
>> +{
>> +ap_filter_t **key = data;
>> +
>> +apr_hash_set((*key)->c->filters, key, sizeof(ap_filter_t **), NULL);
>> +
>> +return APR_SUCCESS;
>> +}
> 
> Shouldn't we call filters_cleanup() from ap_remove_output_filter() too?

I deliberately decided not to.

The majority of calls to ap_remove_output_filter() are made when filters step 
out the way before data starts flowing, and if we tried to run the cleanup we’d 
be performing an unnecessary apr_hash_set() on every invocation before the 
filter was ever added to the hash.

The point at which it would make a difference is after the EOS bucket is 
handled, but at this point it doesn’t matter, all we’re doing is saving one 
NULL check in the MPM on a loop with a handful of iterations, it’s not worth 
the expense.

> Why not simply use:
>   key = apr_pmemdup(pool, , sizeof f);
>   apr_hash_set(f->c->filters, , sizeof key, f)
> here, and:
>   ap_filter_t *f = data;
>   apr_hash_set(f->c->filters, , sizeof f, NULL);
> in filters_cleanup() above?

We could but it’s more expensive. The memdup part is replaced by simply 
assigning a pointer.

> The linked list could possibly be simpler here too (using the 'next' field).
> Above filters_cleanup() would have to start from the beginning of the
> list each time to remove the entries, though.

As described above, this is a set, not a linked list.

>> +/* decide what pool we setaside to, request pool or deferred pool? 
>> */
>> +if (f->r) {
>> +pool = f->r->pool;
>> +APR_BRIGADE_CONCAT(f->bb, bb);
>> +}
>> +else {
>> +if (!f->deferred_pool) {
>> +apr_pool_create(>deferred_pool, f->c->pool);
>> +  

Re: svn commit: r1707163 - /httpd/httpd/trunk/server/util_filter.c

[Yann Ylavic]

On Wed, Oct 7, 2015 at 12:38 AM,   wrote:
> Author: minfrin
> Date: Tue Oct  6 22:38:28 2015
> New Revision: 1707163
>
> URL: http://svn.apache.org/viewvc?rev=1707163=rev
> Log:
> Make sure that transient buckets are morphed into real buckets before
> the filter returns.
>
> Modified:
> httpd/httpd/trunk/server/util_filter.c
>
> Modified: httpd/httpd/trunk/server/util_filter.c
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/server/util_filter.c?rev=1707163=1707162=1707163=diff
> ==
> --- httpd/httpd/trunk/server/util_filter.c (original)
> +++ httpd/httpd/trunk/server/util_filter.c Tue Oct  6 22:38:28 2015
> @@ -736,6 +736,16 @@ AP_DECLARE(apr_status_t) ap_filter_setas
>
>  /* decide what pool we setaside to, request pool or deferred pool? */
>  if (f->r) {
> +apr_bucket *e;
> +for (e = APR_BRIGADE_FIRST(bb); e != APR_BRIGADE_SENTINEL(bb); e 
> =
> +APR_BUCKET_NEXT(e)) {
> +if (APR_BUCKET_IS_TRANSIENT(e)) {
> +int rv = apr_bucket_setaside(e, f->r->pool);
> +if (rv != APR_SUCCESS) {
> +return rv;
> +}
> +}
> +}
>  pool = f->r->pool;
>  APR_BRIGADE_CONCAT(f->bb, bb);
>  }

I don't understand this, why using ap_save_brigade() for both cases
(i.e. setting aside or reading the other types of buckets) would lead
to synchronous behaviour?
It is when r->pool gets destroyed?
If so, how is it different from the buckets currently set aside on r->pool?

We could also possibly always use the deferred_pool (moving it to
conn_rec if we don't want one per reinstate-able filter).

Regards,
Yann.

Re: svn commit: r1706275 - /httpd/httpd/trunk/modules/ssl/ssl_engine_io.c

[Yann Ylavic]

On Tue, Oct 6, 2015 at 8:41 PM, Plüm, Rüdiger, Vodafone Group
 wrote:
>
>
> I am confused now. I understood that the state machine for the server case is 
> fine. Hence that it flushes automatically where needed. So we only should and 
> need to take care of the client case.
> How does using !SSL_is_init_finished(ssl) simplifies the logic?

Sorry for the confusion.
What I meant is that it simplifies the logic in mod_ssl to use
SSL_is_init_finished() unconditionally, since it (obviously) addresses
all the cases: buggy openssl versions, client and server.
It is required for the former case anyway, and is identical to
SSL_in_connect_init() for the client case.
For the server case, openssl will use its own buffering mechanism
during the handshake "so that the output is sent in a way that TCP
likes", according to the comment in the code, so we shouldn't be
flushing small chunks.
Yet for the server case still, openssl will issue its own flush
appropriately, so we may introduce a spurious flush by doing this
(something I didn't think about).

Thus I agree that Joe's proposal is better: SSL_in_connect_init() but
for the buggy case (openssl < 0.9.8m) where we need the generic
SSL_is_init_finished().
Will commit this, thanks for the feedbacks.

Regards,
Yann.

RE: svn commit: r1706669 - in /httpd/httpd/trunk: ./ include/ modules/http/ modules/ssl/ server/ server/mpm/event/ server/mpm/motorz/ server/mpm/simple/

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

> -Original Message-
> From: Graham Leggett [mailto:minf...@sharp.fm]
> Sent: Dienstag, 6. Oktober 2015 12:16
> To: dev@httpd.apache.org
> Subject: Re: svn commit: r1706669 - in /httpd/httpd/trunk: ./ include/
> modules/http/ modules/ssl/ server/ server/mpm/event/ server/mpm/motorz/
> server/mpm/simple/
> 
> On 06 Oct 2015, at 1:39 AM, Yann Ylavic  wrote:
> 
> 
> >> +/* decide what pool we setaside to, request pool or deferred
> pool? */
> >> +if (f->r) {
> >> +pool = f->r->pool;
> >> +APR_BRIGADE_CONCAT(f->bb, bb);
> >> +}
> >> +else {
> >> +if (!f->deferred_pool) {
> >> +apr_pool_create(>deferred_pool, f->c->pool);
> >> +apr_pool_tag(f->deferred_pool, "deferred_pool");
> >> +}
> >> +pool = f->deferred_pool;
> >> +return ap_save_brigade(f, >bb, , pool);
> >> +}
> >
> > Shouldn't ap_save_brigade() be moved below the "else”?
> 
> It shouldn’t no.
> 
> ap_save_brigade() performs setaside, and that is precisely what we want to
> avoid for request filters. In request filters we can safely
> APR_BRIGADE_CONCAT and not worry.
> 
> If you moved ap_save_brigade() the server behaviour would become
> synchronous again for data that wasn’t a file.
> 

How can you be sure that you don't have transient buckets in the brigade that 
point to memory that changed or is invalid, once you reinstate the brigade?

Regards

Rüdiger

Re: svn commit: r1706669 - in /httpd/httpd/trunk: ./ include/ modules/http/ modules/ssl/ server/ server/mpm/event/ server/mpm/motorz/ server/mpm/simple/

[Yann Ylavic]

On Tue, Oct 6, 2015 at 11:31 AM, Plüm, Rüdiger, Vodafone Group
 wrote:
>
>
>> -Original Message-
>> From: Yann Ylavic [mailto:ylavic@gmail.com]
>> Sent: Dienstag, 6. Oktober 2015 11:13
>> To: dev@httpd.apache.org
>> Subject: Re: svn commit: r1706669 - in /httpd/httpd/trunk: ./ include/
>> modules/http/ modules/ssl/ server/ server/mpm/event/ server/mpm/motorz/
>> server/mpm/simple/
>>
>> On Tue, Oct 6, 2015 at 9:37 AM, Plüm, Rüdiger, Vodafone Group
>>  wrote:
>> >
>> >> -Original Message-
>> >> From: Yann Ylavic [mailto:ylavic@gmail.com]
>> >>
>> >> > Modified: httpd/httpd/trunk/server/mpm/event/event.c
>> >> > URL:
>> >>
>> http://svn.apache.org/viewvc/httpd/httpd/trunk/server/mpm/event/event.c?re
>> >> v=1706669=1706668=1706669=diff
>> >> >
>> >>
>> ==
>> >> 
>> >> > --- httpd/httpd/trunk/server/mpm/event/event.c (original)
>> >> > +++ httpd/httpd/trunk/server/mpm/event/event.c Sun Oct  4 10:10:51
>> 2015
>> >> > @@ -1146,19 +1146,38 @@ read_request:
>> >> >  }
>> >> >
>> >> []
>> >> > +
>> >> > +rindex = apr_hash_first(NULL, c->filters);
>> >> > +while (rindex) {
>> >> > +ap_filter_t *f = apr_hash_this_val(rindex);
>> >> > +
>> >> > +if (!APR_BRIGADE_EMPTY(f->bb)) {
>> >> > +
>> >> > +rv = ap_pass_brigade(f, c->empty);
>> >> > +apr_brigade_cleanup(c->empty);
>> >> > +if (APR_SUCCESS != rv) {
>> >> > +ap_log_cerror(
>> >> > +APLOG_MARK, APLOG_DEBUG, rv, c,
>> >> APLOGNO(00470)
>> >> > +"write failure in '%s' output filter",
>> f-
>> >> >frec->name);
>> >> > +break;
>> >> > +}
>> >> > +
>> >> > +if (ap_filter_should_yield(f)) {
>> >> > +data_in_output_filters = 1;
>> >> > +}
>> >> > +}
>> >> > +
>> >> > +rindex = apr_hash_next(rindex);
>> >> >  }
>> >>
>> >> This pattern looks shared by all (relevant) MPMs, couldn't we make it
>> >> a function? Maybe ap_reinstate_conn()?
>> >
>> > Sounds sensible.
>>
>> By defining this function in util_filter
>> (ap_filter_reinstate_conn()?), we also possibly could get rid of
>> c->empty (which looks quite hacky), using/hidding an empty brigade per
>> filter (in opaque data).
>
> Although the brigade is empty, don't we need to prevent the brigade being 
> used by multiple threads in parallel?
> Using one per connection prevents this.

Hmm, the filters are also allocated per connection/request (on
c/r->pool), and shouldn't be called concurrently right?

BTW, I wonder if we can "reinstate" the filters in arbitrary order
like in the above loop (the order seems to depend on the calls to
ap_filter_setaside_brigade() and internal apr_hash_t ordering).
Don't we need to start from r->ouput_filters down to the last filter
and call ap_pass_brigade(f, c->empty) if f is in the hashtable?

Re: svn commit: r1706669 - in /httpd/httpd/trunk: ./ include/ modules/http/ modules/ssl/ server/ server/mpm/event/ server/mpm/motorz/ server/mpm/simple/

[Graham Leggett]

On 06 Oct 2015, at 12:26 PM, Plüm, Rüdiger, Vodafone Group 
 wrote:

> How can you be sure that you don't have transient buckets in the brigade that 
> point to memory that changed or is invalid, once you reinstate the brigade?

Because it’s a request filter, not a connection filter. Request filters set 
aside brigades all the time in their local contexts, this is normal.

Regards,
Graham
—

Re: svn commit: r1706669 - in /httpd/httpd/trunk: ./ include/ modules/http/ modules/ssl/ server/ server/mpm/event/ server/mpm/motorz/ server/mpm/simple/

[Graham Leggett]

On 06 Oct 2015, at 12:36 PM, Yann Ylavic  wrote:

>> Although the brigade is empty, don't we need to prevent the brigade being 
>> used by multiple threads in parallel?
>> Using one per connection prevents this.
> 
> Hmm, the filters are also allocated per connection/request (on
> c/r->pool), and shouldn't be called concurrently right?

Yes.

> BTW, I wonder if we can "reinstate" the filters in arbitrary order
> like in the above loop (the order seems to depend on the calls to
> ap_filter_setaside_brigade() and internal apr_hash_t ordering).
> Don't we need to start from r->ouput_filters down to the last filter
> and call ap_pass_brigade(f, c->empty) if f is in the hashtable?

No - we don’t “reinstate” filters in any way, we just kick them. We kick each 
eligible filter exactly once on each pass, and the order doesn’t matter. Every 
filter with data in it gets a kick to ensure no filter is starved, all filters 
without data are silently ignored.

When a kick arrives and the filter has something to write, the normal filter 
stack write order is preserved by ap_pass_brigade() and everything happens in 
the right order naturally without us having to do anything special.

Typically only two filters at a time - the bottleneck and the network filter - 
will have data in it, all other filters will be empty. If mod_cache is present, 
then three filters are likely to have data in it (mod_cache, the bottleneck 
filter and the network filter), and three filters will get kicked.

When I was investigating this I was originally looking at a skiplist to keep 
the ordering, but once I analysed it it turned out to be unnecessary.

Regards,
Graham
—

Re: svn commit: r1706669 - in /httpd/httpd/trunk: ./ include/ modules/http/ modules/ssl/ server/ server/mpm/event/ server/mpm/motorz/ server/mpm/simple/

[Yann Ylavic]

On Tue, Oct 6, 2015 at 12:15 PM, Graham Leggett  wrote:
> On 06 Oct 2015, at 1:39 AM, Yann Ylavic  wrote:
>
>>> +
>>> +/** Buffered data associated with the current filter. */
>>> +apr_bucket_brigade *bb;
>>> +
>>> +/** Dedicated pool to use for deferred writes. */
>>> +apr_pool_t *deferred_pool;
>>
>> Could we make these opaque, eg:
[]
>>
>> This would prevent them from being mangled anywhere (they are
>> internals to util_filter.c anyway).
>
> There weren’t originally internal to util_filter.c, it’s only subsequently 
> turned out that way. Doing that comes at a cost of a further 8 bytes per 
> filter per request through, which starts adding up.

8 or a few more bytes is a concern, really?

>
> I would rather finish the filter suspension work before doing any optimising 
> like this, it’s too soon at this stage.

Fair enough.

>
>> It could also possibly avoid walking the brigade for every
>> ap_filter_reinstate_brigade() call…
>
> Unfortunately you can’t avoid walking the brigade in the 
> ap_filter_reinstate_brigade(), as this is the flow control logic that keeps 
> the server safe from consuming too much data.

Well, there are only invariants here, the loop always computes the
same values for the buffered_bb or am I missing something?

>
>> Also it seems that we leak the hash iterator here (on c->pool).
>
> We don’t, when you create an iterator with a NULL pool it uses an iterator 
> internal to the hash. If we passed c->pool to apr_hash_first(), we would leak.

Correct, Rüdiger spotted it already, my bad.

>
>> Shouldn't we call filters_cleanup() from ap_remove_output_filter() too?
>
> I deliberately decided not to.
>
> The majority of calls to ap_remove_output_filter() are made when filters step 
> out the way before data starts flowing, and if we tried to run the cleanup 
> we’d be performing an unnecessary apr_hash_set() on every invocation before 
> the filter was ever added to the hash.
>
> The point at which it would make a difference is after the EOS bucket is 
> handled, but at this point it doesn’t matter, all we’re doing is saving one 
> NULL check in the MPM on a loop with a handful of iterations, it’s not worth 
> the expense.



>
>> Why not simply use:
>>   key = apr_pmemdup(pool, , sizeof f);
>>   apr_hash_set(f->c->filters, , sizeof key, f)
>> here, and:
>>   ap_filter_t *f = data;
>>   apr_hash_set(f->c->filters, , sizeof f, NULL);
>> in filters_cleanup() above?
>
> We could but it’s more expensive. The memdup part is replaced by simply 
> assigning a pointer.

Not sure about the overhead, memcpy is optimized for small sizes anyway.
At least both "sizeof(ap_filter_t **)" here and in filters_cleanup()
should be replaced by "sizeof(ap_filter_t *)" which is syntactically
more correct (even if both are equal in C).

>
>>> +*flush_upto = NULL;
>>> +
>>> +bytes_in_brigade = 0;
>>> +non_file_bytes_in_brigade = 0;
>>> +eor_buckets_in_brigade = 0;
>>> +morphing_bucket_in_brigade = 0;
>>
>> Per the ealier suggestion to make ap_filter_data opaque, these could
>> be part of the struct (and reentrant).
>> We could then PREPEND after the loop below, and avoid potentially to
>> walk the same buckets each time.
>
> Alas as described above that’s not possible, as it breaks the flow control 
> mechanism that prevents the server generating too much data.

Unless what's in buffered_bb is opaque (immutable between calls)...

Regards,
Yann.

Re: svn commit: r1706669 - in /httpd/httpd/trunk: ./ include/ modules/http/ modules/ssl/ server/ server/mpm/event/ server/mpm/motorz/ server/mpm/simple/

[Yann Ylavic]

On Tue, Oct 6, 2015 at 12:57 PM, Graham Leggett  wrote:
>
> On 06 Oct 2015, at 12:36 PM, Yann Ylavic  wrote:
>> BTW, I wonder if we can "reinstate" the filters in arbitrary order
>> like in the above loop (the order seems to depend on the calls to
>> ap_filter_setaside_brigade() and internal apr_hash_t ordering).
>> Don't we need to start from r->ouput_filters down to the last filter
>> and call ap_pass_brigade(f, c->empty) if f is in the hashtable?
>
> No - we don’t “reinstate” filters in any way, we just kick them. We kick each 
> eligible filter exactly once on each pass, and the order doesn’t matter. 
> Every filter with data in it gets a kick to ensure no filter is starved, all 
> filters without data are silently ignored.

What I meant is that passing an empty brigade to a reinstate-able
filter may make it pass its data to the next filter, so we may want to
do this only on the first reinstate-able filter starting from
r->output_filters.

So it seems to me that order matters, but I must be missing something,
need to think more about this.


Regards,
Yann.

Re: T of 2.4.17 this week

[Daniel Gruno]

On 10/06/2015 02:01 PM, Jeff Trawick wrote:
> On Tue, Oct 6, 2015 at 7:44 AM, Daniel Gruno  > wrote:
> 
> On 10/05/2015 06:35 PM, Daniel Gruno wrote:
> > +1!
> > There's so much new goodness we have to share! :)
> >
> > On 10/5/2015, 5:54:04 PM, Jim Jagielski  wrote:
> >> I propose a T of 2.4.17 this week with a release for next. I
> >> will RM.
> >>
> >> Comments?
> >>
> 
> Also, if someone could verify that the tweaks I made to mod_lua makes it
> compile properly on Ubutnu 14.04 (with lua5.2-dev installed), that would
> be super swell!
> 
> 
> The build looks okay here, but I haven't tried any scripts...

Awesome, as long as the .so file gets built, I'm happy :)
Thanks for checking!

With regards,
Daniel.

Re: svn commit: r1706669 - in /httpd/httpd/trunk: ./ include/ modules/http/ modules/ssl/ server/ server/mpm/event/ server/mpm/motorz/ server/mpm/simple/

[Yann Ylavic]

On Tue, Oct 6, 2015 at 3:51 PM, Yann Ylavic  wrote:
>
> Just did that, same thing.
> I was using mpm_worker, but now tried mpm_event with same segfaults.

FWIW, I'm running an (old) Debian 6.0.10 (squeeze, 64bit).

Re: svn commit: r1706669 - in /httpd/httpd/trunk: ./ include/ modules/http/ modules/ssl/ server/ server/mpm/event/ server/mpm/motorz/ server/mpm/simple/

[Graham Leggett]

On 06 Oct 2015, at 1:07 PM, Yann Ylavic  wrote:

> What I meant is that passing an empty brigade to a reinstate-able
> filter may make it pass its data to the next filter, so we may want to
> do this only on the first reinstate-able filter starting from
> r->output_filters.

If we did that we create the potential for filter starvation.

The mod_cache filter is a classic case of this - it wants to call 
ap_filter_should_yield() to work out whether downstream is ready for more data, 
or whether it should just loop around and cache more data from the backend. But 
downstream is never ready for more data, because we stopped kicking filters 
when we got to mod_cache. The request hangs.

The safest course of action to ensure we never second guess what a filter is 
trying to do is to kick all the eligible ones, once.

Regards,
Graham
—

Re: svn commit: r1706669 - in /httpd/httpd/trunk: ./ include/ modules/http/ modules/ssl/ server/ server/mpm/event/ server/mpm/motorz/ server/mpm/simple/

[Graham Leggett]

On 06 Oct 2015, at 12:36 PM, Graham Leggett  wrote:

>> How can you be sure that you don't have transient buckets in the brigade 
>> that point to memory that changed or is invalid, once you reinstate the 
>> brigade?
> 
> Because it’s a request filter, not a connection filter. Request filters set 
> aside brigades all the time in their local contexts, this is normal.

I see what you mean - transient buckets specifically will need to be setaside 
before the loop ends. Will take a look.

Regards,
Graham
—

Re: svn commit: r1706669 - in /httpd/httpd/trunk: ./ include/ modules/http/ modules/ssl/ server/ server/mpm/event/ server/mpm/motorz/ server/mpm/simple/

[Yann Ylavic]

On Tue, Oct 6, 2015 at 3:41 PM, Graham Leggett  wrote:
> On 6 Oct 2015, at 14:32, Yann Ylavic  wrote:
>
>> So it seems to relate to the EOR bucket, first not being passed
>> through, and second leading to a double-free or alike.
>>
>> Did not investigate further...
>
> Did you rebuild you tree completely clean before trying it out?
>
> Whenever I got weird behavior a clean rebuild sorted it out. The full test 
> suite works fine for me using the event mpm.

Just did that, same thing.
I was using mpm_worker, but now tried mpm_event with same segfaults.

Re: svn commit: r1706275 - /httpd/httpd/trunk/modules/ssl/ssl_engine_io.c

[Yann Ylavic]

On Thu, Oct 1, 2015 at 8:22 PM, Ruediger Pluem  wrote:
>
> The issue is that openssl during the connect handshake to a clieent does not 
> tell httpd to flush. Hence the CLIENT_HELLO
> remains in the core output filter buffer and openssl waits for the 
> SERVER_HELLO from the remote server which of course
> does not happen without the CLIENT_HELLO having been sent there.
>

I also tried the following patch which also passes the test framework
and is maybe more straightforward since it flushes on write (during
handshake only), thus avoiding any flush (and round-trip) on read.

WDYT?

Index: modules/ssl/ssl_engine_io.c
===
--- modules/ssl/ssl_engine_io.c(revision 1706668)
+++ modules/ssl/ssl_engine_io.c(working copy)
@@ -214,6 +214,21 @@ static int bio_filter_out_write(BIO *bio, const ch
 e = apr_bucket_transient_create(in, inl, outctx->bb->bucket_alloc);
 APR_BRIGADE_INSERT_TAIL(outctx->bb, e);

+/* In theory, OpenSSL should flush as necessary, but it is known
+ * not to do so correctly in some cases (< 0.9.8m; see PR 46952),
+ * or on the proxy/client side (after ssl23_client_hello(), e.g.
+ * ssl/proxy.t test suite).
+ *
+ * Historically, this flush call was performed only for an SSLv2
+ * connection or for a proxy connection.  Calling _out_flush can
+ * be expensive in cases where requests/reponses are pipelined,
+ * so limit the performance impact to handshake time.
+ */
+if (!SSL_is_init_finished(outctx->filter_ctx->pssl)) {
+e = apr_bucket_flush_create(outctx->bb->bucket_alloc);
+APR_BRIGADE_INSERT_TAIL(outctx->bb, e);
+}
+
 if (bio_filter_out_pass(outctx) < 0) {
 return -1;
 }
@@ -452,7 +467,6 @@ static int bio_filter_in_read(BIO *bio, char *in,
 apr_size_t inl = inlen;
 bio_filter_in_ctx_t *inctx = (bio_filter_in_ctx_t *)(bio->ptr);
 apr_read_type_e block = inctx->block;
-int need_flush;

 inctx->rc = APR_SUCCESS;

@@ -466,27 +480,6 @@ static int bio_filter_in_read(BIO *bio, char *in,
 return -1;
 }

-/* In theory, OpenSSL should flush as necessary, but it is known
- * not to do so correctly in some cases (< 0.9.8m; see PR 46952),
- * or on the proxy/client side (after ssl23_client_hello(), e.g.
- * ssl/proxy.t test suite).
- *
- * Historically, this flush call was performed only for an SSLv2
- * connection or for a proxy connection.  Calling _out_flush can
- * be expensive in cases where requests/reponses are pipelined,
- * so limit the performance impact to handshake time.
- */
-#if OPENSSL_VERSION_NUMBER < 0x0009080df
-need_flush = 1;
-#else
-need_flush = !SSL_is_init_finished(inctx->ssl);
-#endif
-if (need_flush && bio_filter_out_flush(inctx->bio_out) < 0) {
-bio_filter_out_ctx_t *outctx = inctx->bio_out->ptr;
-inctx->rc = outctx->rc;
-return -1;
-}
-
 BIO_clear_retry_flags(bio);

 if (!inctx->bb) {
--

Regards,
Yann.

Re: T of 2.4.17 this week

[Daniel Gruno]

On 10/05/2015 06:35 PM, Daniel Gruno wrote:
> +1!
> There's so much new goodness we have to share! :)
> 
> On 10/5/2015, 5:54:04 PM, Jim Jagielski  wrote: 
>> I propose a T of 2.4.17 this week with a release for next. I
>> will RM.
>>
>> Comments?
>>

Also, if someone could verify that the tweaks I made to mod_lua makes it
compile properly on Ubutnu 14.04 (with lua5.2-dev installed), that would
be super swell!

With regards,
Daniel.

Re: T of 2.4.17 this week

[Jeff Trawick]

On Tue, Oct 6, 2015 at 7:44 AM, Daniel Gruno  wrote:

> On 10/05/2015 06:35 PM, Daniel Gruno wrote:
> > +1!
> > There's so much new goodness we have to share! :)
> >
> > On 10/5/2015, 5:54:04 PM, Jim Jagielski  wrote:
> >> I propose a T of 2.4.17 this week with a release for next. I
> >> will RM.
> >>
> >> Comments?
> >>
>
> Also, if someone could verify that the tweaks I made to mod_lua makes it
> compile properly on Ubutnu 14.04 (with lua5.2-dev installed), that would
> be super swell!
>
>
The build looks okay here, but I haven't tried any scripts...

VERSION="14.04.3 LTS, Trusty Tahr"

liblua5.2-0:amd64 install
liblua5.2-dev:amd64 install

checking whether to enable mod_lua... checking dependencies
checking for pow in -lm... yes
checking for sqrt in -lm... yes
checking for lua.h in ./include/lua-5.2... no
checking for lua.h in ./include/lua5.2... no
checking for lua.h in ./include/lua52... no
checking for lua.h in ./include... no
checking for lua.h in ./include/lua-5.1... no
checking for lua.h in ./include/lua5.1... no
checking for lua.h in ./include/lua51... no
checking for lua.h in /usr/local/include/lua-5.2... no
checking for lua.h in /usr/local/include/lua5.2... no
checking for lua.h in /usr/local/include/lua52... no
checking for lua.h in /usr/local/include... no
checking for lua.h in /usr/local/include/lua-5.1... no
checking for lua.h in /usr/local/include/lua5.1... no
checking for lua.h in /usr/local/include/lua51... no
checking for lua.h in /usr/include/lua-5.2... no
checking for lua.h in /usr/include/lua5.2... yes
checking for luaL_newstate in -llua5.2... yes
configure: using '-L/usr/lib -llua5.2 -lm' for Lua Library
  setting MOD_INCLUDES to "-I/usr/include/lua5.2"
  setting MOD_LUA_LDADD to "-L/usr/lib -llua5.2 -lm"
checking whether to enable mod_lua... shared
  adding "-I$(top_srcdir)/modules/lua" to INCLUDES

(no compile warnings)

$ ~/inst/24-64/bin/apachectl -M | grep lua
 lua_module (shared)


With regards,
> Daniel.
>



-- 
Born in Roswell... married an alien...
http://emptyhammock.com/

Re: svn commit: r1706669 - in /httpd/httpd/trunk: ./ include/ modules/http/ modules/ssl/ server/ server/mpm/event/ server/mpm/motorz/ server/mpm/simple/

[Yann Ylavic]

On Sun, Oct 4, 2015 at 12:10 PM,   wrote:
> Author: minfrin
> Date: Sun Oct  4 10:10:51 2015
> New Revision: 1706669
>
> URL: http://svn.apache.org/viewvc?rev=1706669=rev
> Log:
> core: Extend support for asynchronous write completion from the
> network filter to any connection or request filter.

Just wanted to run the tests framework on trunk for a local change but
it seems that this commit (bisected to) makes many tests to segfault.

Here is the backtrace:

Program terminated with signal 6, Aborted.
(gdb) bt
#0  0x7f1984bd1ed5 in *__GI_raise (sig=) at
../nptl/sysdeps/unix/sysv/linux/raise.c:64
#1  0x7f1984bd4ce0 in *__GI_abort () at abort.c:92
#2  0x7f1984c0b9db in __libc_message (do_abort=,
fmt=) at ../sysdeps/unix/sysv/linux/libc_fatal.c:189
#3  0x7f1984c15236 in malloc_printerr (action=3,
str=0x7f1984ccce7e "corrupted double-linked list", ptr=) at malloc.c:6296
#4  0x7f1984c1566d in malloc_consolidate (av=) at
malloc.c:5174
#5  0x7f1984c16b78 in _int_free (av=0x7f1984f04e40, p=0xc9f140) at
malloc.c:5047
#6  0x7f1984c1a01c in *__GI___libc_free (mem=) at
malloc.c:3739
#7  0x7f19859dad6f in allocator_free (allocator=0x9cf580,
node=0xc9f150) at memory/unix/apr_pools.c:474
#8  0x7f19859db527 in apr_pool_destroy (pool=0xc91108) at
memory/unix/apr_pools.c:1016
#9  0x00459dcf in eor_bucket_destroy (data=0xc91180) at eor_bucket.c:90
#10 0x7f19859a6517 in apr_brigade_cleanup (data=0xc99b40) at
buckets/apr_brigade.c:51
#11 0x00488c20 in ap_process_request_after_handler
(r=0xc91180) at http_request.c:283
#12 0x00488f2c in ap_process_async_request (r=0xc91180) at
http_request.c:371
#13 0x00488f58 in ap_process_request (r=0xc91180) at http_request.c:381
#14 0x00485190 in ap_process_http_sync_connection (c=0xb21168)
at http_core.c:210
#15 0x0048529e in ap_process_http_connection (c=0xb21168) at
http_core.c:251
#16 0x00478bf5 in ap_run_process_connection (c=0xb21168) at
connection.c:41
#17 0x004790d7 in ap_process_connection (c=0xb21168,
csd=0xb20f50) at connection.c:206
#18 0x7f197bb3b8fb in process_socket (thd=0x944940, p=0xb20ec8,
sock=0xb20f50, my_child_num=0, my_thread_num=0, bucket_alloc=0xc4aed8)
at worker.c:632
#19 0x7f197bb3c576 in worker_thread (thd=0x944940, dummy=0x9c8b90)
at worker.c:992
#20 0x7f19859ea6af in dummy_worker (opaque=0x944940) at
threadproc/unix/thread.c:145
#21 0x7f198533b8ca in start_thread (arg=) at
pthread_create.c:300
#22 0x7f1984c7608d in clone () at
../sysdeps/unix/sysv/linux/x86_64/clone.S:112
#23 0x in ?? ()
(gdb) frame 11
#11 0x00488c20 in ap_process_request_after_handler
(r=0xc91180) at http_request.c:283
283 apr_brigade_cleanup(bb);
(gdb) l 280
275 ap_pass_brigade(r->output_filters, bb);
276
277 /* The EOR bucket has either been handled by an output filter (eg.
278  * deleted or moved to a buffered_bb => no more in bb), or an error
279  * occured before that (eg. c->aborted => still in bb) and we ought
280  * to destroy it now. So cleanup any remaining bucket along with
281  * the orphan request (if any).
282  */
283 apr_brigade_cleanup(bb);
284

So it seems to relate to the EOR bucket, first not being passed
through, and second leading to a double-free or alike.

Did not investigate further...

Regards,
Yann.

Re: svn commit: r1706669 - in /httpd/httpd/trunk: ./ include/ modules/http/ modules/ssl/ server/ server/mpm/event/ server/mpm/motorz/ server/mpm/simple/

[Yann Ylavic]

On Tue, Oct 6, 2015 at 3:32 PM, Yann Ylavic  wrote:
>
> Just wanted to run the tests framework on trunk for a local change but
> it seems that this commit (bisected to) makes many tests to segfault.

Forgot to mention, same thing with follow up r1706670 applied.

Re: svn commit: r1706669 - in /httpd/httpd/trunk: ./ include/ modules/http/ modules/ssl/ server/ server/mpm/event/ server/mpm/motorz/ server/mpm/simple/

[Stefan Eissing]

FYI: /trunk no longer works with mod_http2. 2.4.x does. I see missing response 
data, it seems, so the most likely cause are the changes in filter handling. 
Did not find the time to investigate further.

Please be aware the mod_h2 uses pool/brigade hierarchies in new and unexpected 
ways. The request has been processed and EOR bucket has been handled in another 
thread, however data/file handles might have been transferred to the main 
thread and are being cleaned up when the main connection thinks a stream is 
done. If those data gets set aside, the memory might get freed/reused too 
early. Maybe mod_http2 needs to come up with its own EOR buckets on the main 
connection to allow for cleanup synchronization. Not sure...

//Stefan

> Am 06.10.2015 um 13:18 schrieb Graham Leggett :
> 
> On 06 Oct 2015, at 12:36 PM, Graham Leggett  wrote:
> 
>>> How can you be sure that you don't have transient buckets in the brigade 
>>> that point to memory that changed or is invalid, once you reinstate the 
>>> brigade?
>> 
>> Because it’s a request filter, not a connection filter. Request filters set 
>> aside brigades all the time in their local contexts, this is normal.
> 
> I see what you mean - transient buckets specifically will need to be setaside 
> before the loop ends. Will take a look.
> 
> Regards,
> Graham
> —
> 

bytes GmbH
Hafenweg 16, 48155 Münster, Germany
Phone: +49 251 2807760. Amtsgericht Münster: HRB5782

Re: svn commit: r1706669 - in /httpd/httpd/trunk: ./ include/ modules/http/ modules/ssl/ server/ server/mpm/event/ server/mpm/motorz/ server/mpm/simple/

[Graham Leggett]

On 6 Oct 2015, at 14:32, Yann Ylavic  wrote:

> So it seems to relate to the EOR bucket, first not being passed
> through, and second leading to a double-free or alike.
> 
> Did not investigate further...

Did you rebuild you tree completely clean before trying it out?

Whenever I got weird behavior a clean rebuild sorted it out. The full test 
suite works fine for me using the event mpm.

Regards,
Graham
--

Re: svn commit: r1706669 - in /httpd/httpd/trunk: ./ include/ modules/http/ modules/ssl/ server/ server/mpm/event/ server/mpm/motorz/ server/mpm/simple/

[Jim Jagielski]

+1 (concept)

> On Oct 6, 2015, at 11:53 AM, Yann Ylavic  wrote:
> 
> On Tue, Oct 6, 2015 at 5:34 PM, Graham Leggett  wrote:
>> 
>> apr_bucket_simple_copy() looks wrong - in theory we should have a proper 
>> copy function that does the right thing with the second copy, for example by 
>> not copying the pool. If we blindly copy the pool (or the request containing 
>> the pool) I see nothing that would prevent an attempt to free the pool twice.
> 
> Agreed, we probably need something like this:
> 
> Index: server/eor_bucket.c
> ===
> --- server/eor_bucket.c(revision 1707064)
> +++ server/eor_bucket.c(working copy)
> @@ -91,6 +91,17 @@ static void eor_bucket_destroy(void *data)
> }
> }
> 
> +static apr_status_t eor_bucket_copy(apr_bucket *a, apr_bucket **b)
> +{
> +*b = apr_bucket_alloc(sizeof(**b), a->list); /* XXX: check for failure? 
> */
> +**b = *a;
> +
> +/* we don't wan't request to be destroyed twice */
> +(*b)->data = NULL;
> +
> +return APR_SUCCESS;
> +}
> +
> AP_DECLARE_DATA const apr_bucket_type_t ap_bucket_type_eor = {
> "EOR", 5, APR_BUCKET_METADATA,
> eor_bucket_destroy,
> @@ -97,6 +108,6 @@ AP_DECLARE_DATA const apr_bucket_type_t ap_bucket_
> eor_bucket_read,
> apr_bucket_setaside_noop,
> apr_bucket_split_notimpl,
> -apr_bucket_simple_copy
> +eor_bucket_copy
> };
> 
> --
> 
> Regards,
> Yann.

Re: svn commit: r1704683 - /httpd/httpd/trunk/docs/manual/mod/mod_remoteip.xml

[Jim Jagielski]

> On Oct 6, 2015, at 10:47 AM, Eric Covener  wrote:
> 
> On Tue, Sep 22, 2015 at 11:09 PM, William A Rowe Jr  
> wrote:
>> On Tue, Sep 22, 2015 at 8:48 PM, Eric Covener  wrote:
>>> 
>>> Maybe my followup is better phrased.  No issue with handling of internal
>>> IPs.
>>> 
>>> Currently, we act like RemoteIPTrustedProxy * by default (once they've
>>> named the XFF header) and warn people they'd better restrict it.
>> 
>> 
>> I agree that was not the original design and we should address it with a fix
>> rather than a docs fix, IMHO.  'Trusted' is the exception, not the general
>> case.
> 
> bump. I don't love the idea of changing the 2.4 defaults.

+1

> 
> Current doc already says "Unless these other directives are used,
> mod_remoteip will trust all hosts presenting a RemoteIPHeader IP
> value." so I thought it was wise  to reinforce this in other sections.
>  Doc is not back-ported yet.

Re: T of 2.4.17 this week

[olli hauer]

On 2015-10-05 23:07, Yann Ylavic wrote:
> On Mon, Oct 5, 2015 at 8:30 PM, olli hauer  wrote:
>>
>> would you mind to look at #58126 ?
>>
>> It contains a simple patch for acinclude.m4 to suppress warnings about 
>> underquoted calls to AC_DEFUN.
> 
> Committed in r1706918, backport proposed in r1706923.
> 
> Regards,
> Yann.
> 

Thanks! :)

Re: svn commit: r1706669 - in /httpd/httpd/trunk: ./ include/ modules/http/ modules/ssl/ server/ server/mpm/event/ server/mpm/motorz/ server/mpm/simple/

[Graham Leggett]

On 06 Oct 2015, at 7:00 PM, Yann Ylavic  wrote:

> Yet another filter which does not remove itself after the EOS, and
> destroys the EOR bucket while still using *r after.

I am wondering if the EOR bucket is a suboptimal way to clean up after 
ourselves.

The MPM itself is in a position to know when the filter buffers are all empty 
and it is safe to delete a request pool. I am imagining a “shutdown set”, which 
contains the pools which have been marked as ready to shut down. If there is a 
pool in the shutdown set, walk the outstanding filters and look for any filters 
whose pools are descendants of that pool, and check if the filters are empty. 
If so, apr_pool_destroy() and we’re done. If the “shutdown set” is empty, do 
nothing.

Instead of creating an EOR bucket, we call an MPM API that says “add this pool 
to the shutdown set”, and then forget about it.

Legacy MPMs fall back to EOR behaviour, but we deprecate it.

I think the above is a far safer approach than assuming filters will “do the 
right thing”.

Regards,
Graham
—

Re: svn commit: r1706275 - /httpd/httpd/trunk/modules/ssl/ssl_engine_io.c

[Yann Ylavic]

On Tue, Oct 6, 2015 at 6:00 PM, Yann Ylavic  wrote:
> On Tue, Oct 6, 2015 at 5:44 PM, Joe Orton  wrote:
>>
>> Hence In the server case, it seems reasonable to rely on BIO_flush()
>> being called at the "right" times during the handshake.  Modulo the odd
>> bug!
>>
>> But ssl/s3_clnt.c is not following that coding style at all, and it only
>> does a flush after completing the handshake.  So I'd say the right thing
>> here is to FLUSH after every packet which comes through the write BIO
>> when the SSL state machine is in the middle of a "connect", i.e.
>> handshake as client.
>>
>> tl;dr: I think Yann's patch should be right if the test is switched from
>> "always flush if !SSL_is_init_finished(ssl)" to "always flush if
>> SSL_in_connect_init(ssl)"???
>
> Yes, I came to the same conclusion, but decided to use
> SSL_is_init_finished(ssl) anyway because for the server case it seems
> that openssl uses it own buffering mechanism to avoid writing small
> chunks (after the client-hello is received), so possibly we could rely
> on it (this also simplifies the logic).

Also it seems that for the SSL_ST_RENEGOTIATE state in ssl3_accept(),
buffering is disabled by openssl (at least in 1.0.2d).
SSL_is_init_finished(ssl) should catch this case too...

Regards,
Yann.

Re: svn commit: r1706669 - in /httpd/httpd/trunk: ./ include/ modules/http/ modules/ssl/ server/ server/mpm/event/ server/mpm/motorz/ server/mpm/simple/

[Yann Ylavic]

On Tue, Oct 6, 2015 at 5:53 PM, Yann Ylavic  wrote:
> On Tue, Oct 6, 2015 at 5:34 PM, Graham Leggett  wrote:
>>
>> apr_bucket_simple_copy() looks wrong - in theory we should have a proper 
>> copy function that does the right thing with the second copy, for example by 
>> not copying the pool. If we blindly copy the pool (or the request containing 
>> the pool) I see nothing that would prevent an attempt to free the pool twice.
>
> Agreed, we probably need something like this:

Committed in r1707084, all tests pass now.

Re: svn commit: r1706669 - in /httpd/httpd/trunk: ./ include/ modules/http/ modules/ssl/ server/ server/mpm/event/ server/mpm/motorz/ server/mpm/simple/

[Yann Ylavic]

On Tue, Oct 6, 2015 at 6:29 PM, Yann Ylavic  wrote:
>
> all tests pass now.

I spoke too soon...

New segfault is in mod_substitute:

Program terminated with signal 11, Segmentation fault.

(gdb) bt
#0  0x7fa02fbaec2b in allocator_free (allocator=0x1cedcb0,
node=0x0) at memory/unix/apr_pools.c:421
#1  0x7fa02fbaf3a2 in apr_pool_clear (pool=0x1d104e8) at
memory/unix/apr_pools.c:947
#2  0x7fa02b2f1b79 in substitute_filter (f=0x1d0bdb8,
bb=0x1cf27e8) at mod_substitute.c:552
#3  0x0043ff57 in ap_pass_brigade (next=0x1d0bdb8,
bb=0x1cf27e8) at util_filter.c:608
#4  0x00488ce4 in ap_process_request_after_handler
(r=0x1cf3e50) at http_request.c:275
#5  0x00488ffc in ap_process_async_request (r=0x1cf3e50) at
http_request.c:371
#6  0x0048513d in ap_process_http_async_connection
(c=0x1cee0b8) at http_core.c:154
#7  0x00485360 in ap_process_http_connection (c=0x1cee0b8) at
http_core.c:248
#8  0x00478cc5 in ap_run_process_connection (c=0x1cee0b8) at
connection.c:41
#9  0x7fa025d0b7e1 in process_socket (thd=0x1b2fcd8, p=0x1cedda8,
sock=0x1cede20, cs=0x1cee028, my_child_num=0, my_thread_num=0) at
event.c:1136
#10 0x7fa025d0e334 in worker_thread (thd=0x1b2fcd8,
dummy=0x1c98c20) at event.c:2247
#11 0x7fa02fbbe6af in dummy_worker (opaque=0x1b2fcd8) at
threadproc/unix/thread.c:145
#12 0x7fa02f50f8ca in start_thread (arg=) at
pthread_create.c:300
#13 0x7fa02ee4a08d in clone () at
../sysdeps/unix/sysv/linux/x86_64/clone.S:112
#14 0x in ?? ()

Yet another filter which does not remove itself after the EOS, and
destroys the EOR bucket while still using *r after.

Re: svn commit: r1706275 - /httpd/httpd/trunk/modules/ssl/ssl_engine_io.c

[Yann Ylavic]

On Tue, Oct 6, 2015 at 5:44 PM, Joe Orton  wrote:
>
> Hence In the server case, it seems reasonable to rely on BIO_flush()
> being called at the "right" times during the handshake.  Modulo the odd
> bug!
>
> But ssl/s3_clnt.c is not following that coding style at all, and it only
> does a flush after completing the handshake.  So I'd say the right thing
> here is to FLUSH after every packet which comes through the write BIO
> when the SSL state machine is in the middle of a "connect", i.e.
> handshake as client.
>
> tl;dr: I think Yann's patch should be right if the test is switched from
> "always flush if !SSL_is_init_finished(ssl)" to "always flush if
> SSL_in_connect_init(ssl)"???

Yes, I came to the same conclusion, but decided to use
SSL_is_init_finished(ssl) anyway because for the server case it seems
that openssl uses it own buffering mechanism to avoid writing small
chunks (after the client-hello is received), so possibly we could rely
on it (this also simplifies the logic).

Re: svn commit: r1706669 - in /httpd/httpd/trunk: ./ include/ modules/http/ modules/ssl/ server/ server/mpm/event/ server/mpm/motorz/ server/mpm/simple/

[Yann Ylavic]

On Tue, Oct 6, 2015 at 7:00 PM, Yann Ylavic  wrote:
> On Tue, Oct 6, 2015 at 6:29 PM, Yann Ylavic  wrote:
>>
>> all tests pass now.
>
> I spoke too soon...
>
> New segfault is in mod_substitute:

Fixed in r1707091.
Now all tests really pass :p

I wonder how we should address the possible (third-party-)breakage due
to ap_pass_brigade(c->ouput_filters, EOR) =>
ap_pass_brigade(r->ouput_filters, EOR) in
ap_process_request_after_handler(), should this be backported in
2.4.x...

Re: svn commit: r1706669 - in /httpd/httpd/trunk: ./ include/ modules/http/ modules/ssl/ server/ server/mpm/event/ server/mpm/motorz/ server/mpm/simple/

[Yann Ylavic]

On Tue, Oct 6, 2015 at 5:22 PM, Yann Ylavic  wrote:
> still the new filter should probably work
> with the existing...

s/new filter/new filter stack mechanism/