Re: segfault analysis

2015-10-23 Thread Yann Ylavic 
On Fri, Oct 23, 2015 at 10:42 AM, Stefan Eissing
 wrote:
>
> Can I recover any files/lines from these
> === Backtrace: =
> /lib64/libc.so.6[0x32a507174f]
> /lib64/libc.so.6(cfree+0x4b)[0x32a5075a4b]
> /opt/appl/apr-1.5.2/lib/libapr-1.so.0(apr_allocator_destroy+0x1b)[0x2abbed566e6b]
> /opt/appl/httpd/modules/mod_http2.so[0x2abbf10e082e]
> /opt/appl/httpd/modules/mod_http2.so[0x2abbf10e32ee]
> /opt/appl/httpd/modules/mod_http2.so[0x2abbf10db00a]
> /opt/appl/httpd/modules/mod_http2.so[0x2abbf10db233]
> /opt/appl/httpd/modules/mod_http2.so[0x2abbf10de351]
> /opt/appl/httpd-2.4.17/bin/httpd(ap_run_process_connection+0x4a)[0x456efa]
> /opt/appl/httpd-2.4.17/bin/httpd[0x468fa0]
> /lib64/libpthread.so.0[0x32a5c0683d]
> /lib64/libc.so.6(clone+0x6d)[0x32a50d526d]
> === Memory map: 

That's an apr_pool_destroy() from either h2_mplx_destroy() or
h2_worker_destroy(), the only http2's pools that have their own
allocator.

I think the (++attempts >= 6) case in h2_mplx_release_and_join() could
trigger this, do we really want a timedwait here?
Also, apr_thread_cond_signal() in release() should probably be under
mutex, something like (2.4.x):

Index: modules/http2/h2_mplx.c
===
--- modules/http2/h2_mplx.c(revision 1710105)
+++ modules/http2/h2_mplx.c(working copy)
@@ -143,12 +143,18 @@ static void reference(h2_mplx *m)
 apr_atomic_inc32(>refs);
 }

-static void release(h2_mplx *m)
+static void release(h2_mplx *m, int lock)
 {
 if (!apr_atomic_dec32(>refs)) {
+if (lock) {
+apr_thread_mutex_lock(m->lock);
+}
 if (m->join_wait) {
 apr_thread_cond_signal(m->join_wait);
 }
+if (lock) {
+apr_thread_mutex_lock(m->unlock);
+}
 }
 }

@@ -158,7 +164,7 @@ void h2_mplx_reference(h2_mplx *m)
 }
 void h2_mplx_release(h2_mplx *m)
 {
-release(m);
+release(m, 1);
 }

 static void workers_register(h2_mplx *m) {
@@ -189,7 +195,7 @@ apr_status_t h2_mplx_release_and_join(h2_mplx *m,
 if (APR_SUCCESS == status) {
 int attempts = 0;

-release(m);
+release(m, 0);
 while (apr_atomic_read32(>refs) > 0) {
 m->join_wait = wait;
 ap_log_cerror(APLOG_MARK, (attempts? APLOG_INFO : APLOG_DEBUG),
--


>
> or do I have to ask the user to run a -g version and report on that?

That could be useful, along with "CoreDumpDirectory /tmp" (and ulimit
-c unlimited for the process launching httpd) which would provide the
corresponding core file.

>
> High load wreckage in production, always tricky...

Indeed...


Regards,
Yann.

Re: segfault analysis

2015-10-23 Thread Stefan Eissing 
Yes, you are correct. The mutex holds. I had my head in java synchronized waits 
for a second *shudder*...

> Am 23.10.2015 um 12:05 schrieb Yann Ylavic :
> 
> On Fri, Oct 23, 2015 at 11:59 AM, Stefan Eissing
>  wrote:
>> 
>> The problem with the unlock in release is that the h2_mplx might have been 
>> destroyed by then, as the signal might give the lock to someone waiting.
> 
> Hm, h2_mplx_release_and_join() (which destroys the mplx) won't be
> woken up until the mutex is released, since
> apr_thread_cond_timedwait() acquires the mutex.

Re: segfault analysis

2015-10-23 Thread Stefan Eissing 
;-)

The problem with the unlock in release is that the h2_mplx might have been 
destroyed by then, as the signal might give the lock to someone waiting. I 
could move that mutex into h2_session ownership, just lending it to h2_mplx...

//Stefan

> Am 23.10.2015 um 11:22 schrieb Yann Ylavic :
> 
> On Fri, Oct 23, 2015 at 11:18 AM, Yann Ylavic  wrote:
>> 
>> Index: modules/http2/h2_mplx.c
>> ===
>> --- modules/http2/h2_mplx.c(revision 1710105)
>> +++ modules/http2/h2_mplx.c(working copy)
>> @@ -143,12 +143,18 @@ static void reference(h2_mplx *m)
>> apr_atomic_inc32(>refs);
>> }
>> 
>> -static void release(h2_mplx *m)
>> +static void release(h2_mplx *m, int lock)
>> {
>> if (!apr_atomic_dec32(>refs)) {
>> +if (lock) {
>> +apr_thread_mutex_lock(m->lock);
>> +}
>> if (m->join_wait) {
>> apr_thread_cond_signal(m->join_wait);
>> }
>> +if (lock) {
>> +apr_thread_mutex_lock(m->unlock);
> 
> We'd rather unlock here :)
> 
>> +}
>> }
>> }

Re: segfault analysis

2015-10-23 Thread Yann Ylavic 
On Fri, Oct 23, 2015 at 12:13 PM, Yann Ylavic  wrote:
>
> since the worker destroys itself in its own thread (pool == w->pool).
>
> Also, where are the worker threads joined?

Actually I find both joining the worker thread and then
worker_destroy() should be done in h2_workers_destroy(), after
worker->aborted is signalled.

Re: mod_proxy's aside connections proposal

2015-10-23 Thread Eric Covener 
On Tue, Mar 3, 2015 at 9:20 AM, Jim Jagielski  wrote:
> I guess the main question is what are the actual use-cases?


NTLM at the very least. That's the context it comes up in periodically.

Re: segfault analysis

2015-10-23 Thread Yann Ylavic 
On Fri, Oct 23, 2015 at 10:42 AM, Stefan Eissing
 wrote:
>
> In regard to https://bz.apache.org/bugzilla/show_bug.cgi?id=58524

We possibly need this too:

Index: modules/http2/h2_worker.c
===
--- modules/http2/h2_worker.c(revision 1710105)
+++ modules/http2/h2_worker.c(working copy)
@@ -110,7 +110,7 @@ h2_worker *h2_worker_create(int id,
 return NULL;
 }

-apr_thread_create(>thread, attr, execute, w, pool);
+apr_thread_create(>thread, attr, execute, w, parent_pool);
 }
 return w;
 }
--

since the worker destroys itself in its own thread (pool == w->pool).

Also, where are the worker threads joined?

Re: segfault analysis

2015-10-23 Thread Yann Ylavic 
On Fri, Oct 23, 2015 at 11:18 AM, Yann Ylavic  wrote:
>
> Index: modules/http2/h2_mplx.c
> ===
> --- modules/http2/h2_mplx.c(revision 1710105)
> +++ modules/http2/h2_mplx.c(working copy)
> @@ -143,12 +143,18 @@ static void reference(h2_mplx *m)
>  apr_atomic_inc32(>refs);
>  }
>
> -static void release(h2_mplx *m)
> +static void release(h2_mplx *m, int lock)
>  {
>  if (!apr_atomic_dec32(>refs)) {
> +if (lock) {
> +apr_thread_mutex_lock(m->lock);
> +}
>  if (m->join_wait) {
>  apr_thread_cond_signal(m->join_wait);
>  }
> +if (lock) {
> +apr_thread_mutex_lock(m->unlock);

We'd rather unlock here :)

> +}
>  }
>  }

Re: segfault analysis

2015-10-23 Thread Yann Ylavic 
On Fri, Oct 23, 2015 at 11:59 AM, Stefan Eissing
 wrote:
>
> The problem with the unlock in release is that the h2_mplx might have been 
> destroyed by then, as the signal might give the lock to someone waiting.

Hm, h2_mplx_release_and_join() (which destroys the mplx) won't be
woken up until the mutex is released, since
apr_thread_cond_timedwait() acquires the mutex.

Re: segfault analysis

2015-10-23 Thread Stefan Eissing 
Nice analysis, btw. I think you found the problem. I'll replace the timed wait 
by a simple wait...

> Am 23.10.2015 um 11:22 schrieb Yann Ylavic :
> 
> On Fri, Oct 23, 2015 at 11:18 AM, Yann Ylavic  wrote:
>> 
>> Index: modules/http2/h2_mplx.c
>> ===
>> --- modules/http2/h2_mplx.c(revision 1710105)
>> +++ modules/http2/h2_mplx.c(working copy)
>> @@ -143,12 +143,18 @@ static void reference(h2_mplx *m)
>> apr_atomic_inc32(>refs);
>> }
>> 
>> -static void release(h2_mplx *m)
>> +static void release(h2_mplx *m, int lock)
>> {
>> if (!apr_atomic_dec32(>refs)) {
>> +if (lock) {
>> +apr_thread_mutex_lock(m->lock);
>> +}
>> if (m->join_wait) {
>> apr_thread_cond_signal(m->join_wait);
>> }
>> +if (lock) {
>> +apr_thread_mutex_lock(m->unlock);
> 
> We'd rather unlock here :)
> 
>> +}
>> }
>> }

Re: segfault analysis

2015-10-23 Thread Yann Ylavic 
On Fri, Oct 23, 2015 at 12:23 PM, Stefan Eissing
 wrote:
>
> Hmm, I think the best way to tackle this is to transfer the worker into a 
> zombie set from worker_done(). Then in h2_workers_register(), which is called 
> regularly and often, join the threads and destroy the zombie workers.

That's fine, given that workers are created/destroyed regularly
(didn't see that, h2_workers_create() seems to be called from child
init hook only).

Btw, you could also register the join logic in the worker pool
(pre_cleanup), and simply call apr_pool_destroy(w->pool) in the zombie
collector.

Re: segfault analysis

2015-10-23 Thread Stefan Eissing 
Code review is a fine thing...

Hmm, I think the best way to tackle this is to transfer the worker into a 
zombie set from worker_done(). Then in h2_workers_register(), which is called 
regularly and often, join the threads and destroy the zombie workers.

Allocating the worker from parent_pool is not good as workers appear/disappear 
during load changes and the parent pool will just continue to grow.

//Stefan

> Am 23.10.2015 um 12:13 schrieb Yann Ylavic :
> 
> On Fri, Oct 23, 2015 at 10:42 AM, Stefan Eissing
>  wrote:
>> 
>> In regard to https://bz.apache.org/bugzilla/show_bug.cgi?id=58524
> 
> We possibly need this too:
> 
> Index: modules/http2/h2_worker.c
> ===
> --- modules/http2/h2_worker.c(revision 1710105)
> +++ modules/http2/h2_worker.c(working copy)
> @@ -110,7 +110,7 @@ h2_worker *h2_worker_create(int id,
> return NULL;
> }
> 
> -apr_thread_create(>thread, attr, execute, w, pool);
> +apr_thread_create(>thread, attr, execute, w, parent_pool);
> }
> return w;
> }
> --
> 
> since the worker destroys itself in its own thread (pool == w->pool).
> 
> Also, where are the worker threads joined?

Re: [users@httpd] Chunked transfer delay with httpd 2.4 on Windows.

2015-10-23 Thread Andy Wang 



On 10/22/2015 04:50 PM, Yann Ylavic wrote:

On Thu, Oct 22, 2015 at 3:42 PM, Andy Wang  wrote:


Tested with the patch and looks good.


Not that much actually, the patch fails to consume the CRLFs, and
hence can end up in an infinite loop.

So I'm attaching a new one here (committed in trunk with a larger
scope, this version is for 2.4.x and limited to your use case).
Could you give it a (new) try please (I have already done some testing
but it's probably worth passing your tests, before I can propose its
backport to 2.4.x)?
While the previous patch could not handle more than a single
(trailing) [CR]LF, this new one should (up to ten, which is the new
limit for tolerated blank lines in between requests).



The new one works as well.  My VMs that I use for my actual test cases 
are all in a logistics nightmare right now, so I can't do anything more 
thorough than the specific point test, but hopefully when I build the 
next httpd that contains this they'll be back up and I can put things 
through a more thorough cycle.


I'm pretty much only equipped to reproduce the specific problem right now.

That said, this whole, why does it only happen on Windows and why can't 
I simulate a request using another client has been bugging me, so I 
decided to go back to pre-patch and enable dumpio and try to compare the 
two.


guess what
I cannot get the problem to occur with dumpio enabled now.  As soon as 
it's enabled, the requests respond immediately.


Disable dumpio, 5000ms.

I give up.  The plugin developer fixed their plugin, you made apache 
more accepting (thank you very much again), and I'm going to just 
pretend this whole issue never happened :)


Andy

Fwd: [Bug 57785] REDIRECT_URL is not suitable for use in server-generated pages

2015-10-23 Thread Eric Covener 
My opinion is to make it opt-in before the next 2.4, but I am not
committed to that.

Before I do doc, any other thoughts?

http://people.apache.org/~covener/patches/redirecturl-optin.diff

This adds a directive to core.c called QualifyRedirectURL that  must
be set to ON to run the 2.4.17 code.

-- Forwarded message --
From:  
Date: Fri, Oct 23, 2015 at 12:55 PM
Subject: [Bug 57785] REDIRECT_URL is not suitable for use in
server-generated pages
To: b...@httpd.apache.org


https://bz.apache.org/bugzilla/show_bug.cgi?id=57785

--- Comment #7 from Jacob P  ---
Here at cPanel we are reverting our 2.4.17 release and are going to re-release
2.4.16. Too many issues with mod_rewrite/REDIRECT_URL causing a *lot* of
applications to stop working.

--
You are receiving this mail because:
You are the assignee for the bug.

-
To unsubscribe, e-mail: bugs-unsubscr...@httpd.apache.org
For additional commands, e-mail: bugs-h...@httpd.apache.org



-- 
Eric Covener
cove...@gmail.com

Re: master connection + mod_ssl + http2

2015-10-23 Thread Jim Jagielski 

> On Oct 21, 2015, at 10:25 AM, Graham Leggett  wrote:
> 
> On 21 Oct 2015, at 2:42 PM, Stefan Eissing  
> wrote:
> 
>> The basic changes:
>> 1. conn_rec->master is NULL for HTTP/1.1 connections, but points to the 
>> "real" connection for HTTP/2 requests.
>> 2. mod_ssl no longer initalizes any SSLConnRec* for slave connections 
>> (conn_rec->master != NULL)
>> 3. lookup of ssl variables uses the master's sslconn->ssl if none is found 
>> on the connection itself
>> 4. ssl_hook_Access() that checks renegotiation fails with a FORBIDDEN for a 
>> slave connection with a note for the reason.
>>  This should allow mod_http2 to generate the correct HTTP/2 stream error
>> 5. ssl_hook_ReadReq() that checks for wrong host names now has an additional 
>> check for TLS compatiblity which compares
>>  protocol, cipher suite, certificate and key file/path names and verify mode 
>> of the request server against the
>>  handshake server. This compatibility is strict equality and not as 
>> sophisticated as the renegotiation checks.
>> 
>> With these changes, mod_http2 has less work for the slave connection setup 
>> and no longer needs to disable ssl for those. While mod_ssl continues to be 
>> ignorant of mod_http2, as the same restrictions would apply to any protocol 
>> with slave connections. With a minor bump in MMN we can have this in the 
>> next 2.4.
> 
> Not having looked at the patch yet, the above seems to make sense.
> 

Sorry for the lateness:

+1

Re: [Bug 57785] REDIRECT_URL is not suitable for use in server-generated pages

2015-10-23 Thread Jacob Perkins 
+1 on this as well. That looks like a good fix that won’t break existing 
functionality. I know a lot of apps are working on updates now to fix these 
changes, but it’ll still be good to have this pushed back a bit.

Thank you!
—
Jacob Perkins
Product Owner
cPanel Inc.

jacob.perk...@cpanel.net 
Office:  713-529-0800 x 4046
Cell:  713-560-8655

> On Oct 23, 2015, at 2:39 PM, Yann Ylavic  wrote:
> 
> On Fri, Oct 23, 2015 at 9:22 PM, Eric Covener  wrote:
>> My opinion is to make it opt-in before the next 2.4, but I am not
>> committed to that.
>> 
>> Before I do doc, any other thoughts?
>> 
>> http://people.apache.org/~covener/patches/redirecturl-optin.diff
> 
> Looks sensible, +1



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [Bug 57785] REDIRECT_URL is not suitable for use in server-generated pages

2015-10-23 Thread Yann Ylavic 
On Fri, Oct 23, 2015 at 9:22 PM, Eric Covener  wrote:
> My opinion is to make it opt-in before the next 2.4, but I am not
> committed to that.
>
> Before I do doc, any other thoughts?
>
> http://people.apache.org/~covener/patches/redirecturl-optin.diff

Looks sensible, +1

Re: [Bug 57785] REDIRECT_URL is not suitable for use in server-generated pages

2015-10-23 Thread Nick Kew 
On Fri, 23 Oct 2015 15:22:27 -0400
Eric Covener  wrote:

> My opinion is to make it opt-in before the next 2.4, but I am not
> committed to that.

Hehe.  I was leaning towards introducing separate vars for
full URL and fragment.  But your patch looks good to me.

-- 
Nick Kew

segfault analysis

2015-10-23 Thread Stefan Eissing 
A question to the more experienced httpd debuggers:

In regard to https://bz.apache.org/bugzilla/show_bug.cgi?id=58524

Can I recover any files/lines from these
=== Backtrace: =
/lib64/libc.so.6[0x32a507174f]
/lib64/libc.so.6(cfree+0x4b)[0x32a5075a4b]
/opt/appl/apr-1.5.2/lib/libapr-1.so.0(apr_allocator_destroy+0x1b)[0x2abbed566e6b]
/opt/appl/httpd/modules/mod_http2.so[0x2abbf10e082e]
/opt/appl/httpd/modules/mod_http2.so[0x2abbf10e32ee]
/opt/appl/httpd/modules/mod_http2.so[0x2abbf10db00a]
/opt/appl/httpd/modules/mod_http2.so[0x2abbf10db233]
/opt/appl/httpd/modules/mod_http2.so[0x2abbf10de351]
/opt/appl/httpd-2.4.17/bin/httpd(ap_run_process_connection+0x4a)[0x456efa]
/opt/appl/httpd-2.4.17/bin/httpd[0x468fa0]
/lib64/libpthread.so.0[0x32a5c0683d]
/lib64/libc.so.6(clone+0x6d)[0x32a50d526d]
=== Memory map: 

or do I have to ask the user to run a -g version and report on that?

High load wreckage in production, always tricky...

Thanks,

  Stefan