[PATCH] APR thread handle leak on Windows

[Chris Storah <ch...@orionsmg.com>]

Enclosed is a patch to fix a leak in APR threads, due to _endthreadex  
not automatically closing the handle (unlike _endthread).

Chris

Re: [PATCH] APR thread handle leak on Windows

[Chris Storah <ch...@orionsmg.com>]

The endthreadex() would also need CloseHandle(thd->td).
i.e.
	...
	if (thd->td) {
		CloseHandle(thd->td);
		_endthreadex(0);
	}
	...


Chris

> William A. Rowe, Jr. wrote:
> > Chris Storah wrote:
> >
> >> Enclosed is a patch to fix a leak in APR threads, due to  
> _endthreadex
> >> not automatically closing the handle (unlike _endthread).
> >
> >
> > Chris,
> >
> > this is facinating, thank you for bringing it to our attention!!!
> >
>
> There are couple of more things:
> 1. dummy_worker must be __stdcall.
> 2. dummy_worker must call _endthreadex:
>    now its:
>     apr_thread_t *thd = (apr_thread_t *)opaque;
>     TlsSetValue(tls_apr_thread, thd->td);
>     return thd->func(thd, thd->data);
>    it should be:
>     void *rv;
>     apr_thread_t *thd = (apr_thread_t *)opaque;
>     TlsSetValue(tls_apr_thread, thd->td);
>     rv = thd->func(thd, thd->data);
>     TlsSetValue(tls_apr_thread, NULL);
>     if (thd->td)
>        _endthreadex(0);
>     return rv;
> This will ensure that either on thread_join and thread_exit
> both handle and per thread CRT data gets released.
> Now we have a situation where _beginthreadex->CloseHandle
> can be executed, while we should for each _beginthreadex
> have corresponding _endthreadex. This can happen in
> situations where we call thread_join on threads that
> does not call the thread_exit before returning.
>
> Regards,
> Mladen.

Re: [PATCH] APR thread handle leak on Windows

[Mladen Turk <mt...@apache.org>]

William A. Rowe, Jr. wrote:
 > Chris Storah wrote:
 >
 >> Enclosed is a patch to fix a leak in APR threads, due to _endthreadex
 >> not automatically closing the handle (unlike _endthread).
 >
 >
 > Chris,
 >
 > this is facinating, thank you for bringing it to our attention!!!
 >

There are couple of more things:
1. dummy_worker must be __stdcall.
2. dummy_worker must call _endthreadex:
    now its:
     apr_thread_t *thd = (apr_thread_t *)opaque;
     TlsSetValue(tls_apr_thread, thd->td);
     return thd->func(thd, thd->data);
    it should be:
     void *rv;
     apr_thread_t *thd = (apr_thread_t *)opaque;
     TlsSetValue(tls_apr_thread, thd->td);
     rv = thd->func(thd, thd->data);
     TlsSetValue(tls_apr_thread, NULL);
     if (thd->td)
        _endthreadex(0);
     return rv;
This will ensure that either on thread_join and thread_exit
both handle and per thread CRT data gets released.
Now we have a situation where _beginthreadex->CloseHandle
can be executed, while we should for each _beginthreadex
have corresponding _endthreadex. This can happen in
situations where we call thread_join on threads that
does not call the thread_exit before returning.

Regards,
Mladen.

Re: [PATCH] APR thread handle leak on Windows

["William A. Rowe, Jr." <wr...@rowe-clan.net>]

Chris Storah wrote:
> Enclosed is a patch to fix a leak in APR threads, due to _endthreadex  
> not automatically closing the handle (unlike _endthread).

Chris,

this is facinating, thank you for bringing it to our attention!!!

I'll review the patch to ensure your suggestion is in sync with the
Unix behavior, and commit it or something quite like it.

Thanks again!

Bill

Re: [PATCH] APR thread handle leak on Windows

["William A. Rowe, Jr." <wr...@rowe-clan.net>]

Chris, and devs, I'm confused;

the thd->td handle *is* closed in apr_thread_join.  If we deploy this patch
to accomodate a particular programming style, we lose valuable return context
information, but more importantly, if you aren't invoking apr_thread_join, then
on many flavors of *nix you aren't farming the zombie threads.

Note we also properly close thd->td within apr_thread_detach, for much the
same reasons one or the other must be invoked on *nix.

This patch is problematic for me; anyone else have some thoughts to share?

Bill

Chris Storah wrote:
> Enclosed is a patch to fix a leak in APR threads, due to _endthreadex  
> not automatically closing the handle (unlike _endthread).
> 
> Chris
> 
> 
> ------------------------------------------------------------------------
> 
> --- thread.c	2005-12-30 15:44:05.000000000 +1100
> +++ thread.c.orig	2005-12-29 10:05:45.000000000 +1100
> @@ -137,10 +137,6 @@
>      apr_pool_destroy(thd->pool);
>      thd->pool = NULL;
>  #ifndef _WIN32_WCE
> -	/* need to close the handle as _endthreadex does not automatically
> -	 * do this - unlike _endthread
> -	 */
> -	CloseHandle(thd->td);
>      _endthreadex(0);
>  #else
>      ExitThread(0);