You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Michael Schlenker <Mi...@contact-software.com> on 2019/03/19 11:12:25 UTC
APR thread_mutex_cleanup on windows mishandles being called twice
Hi,
just filed https://bz.apache.org/bugzilla/show_bug.cgi?id=63271
(Patch attached)
It seems to be the reason behind various crashes/restarts seen in Apache httpd on Windows, especially when mod_cache_disk is in use.
Basically when the mutex is cleaned up twice, it calls CloseHandle() on uninitialized memory, which causes First Chance Exceptions in the debugger
(if invalid handle) or closes some random Handle behind the back of its real owner (e.g. internal handles of the userspace leading to access violations inside CreateProcess,
httpd Events used to signal between parent and child, etc.).
It would be great if this could make it into 1.7.
Thanks,
Michael
----------------------
Michael Schlenker
Senior Software Engineer
CONTACT Software GmbH Tel.: +49 (421) 20153-80
Wiener Straße 1-3 Fax: +49 (421) 20153-41
28359 Bremen
E-Mail: michael.schlenker@contact-software.com
http://www.contact-software.com/
Registered office: Bremen, Germany
Managing directors: Karl Heinz Zachries, Ralf Holtgrefe
Court of register: Amtsgericht Bremen HRB 13215
Re: APR thread_mutex_cleanup on windows mishandles being called twice
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Tue, Mar 19, 2019 at 12:12 PM William A Rowe Jr <wr...@rowe-clan.net>
wrote:
>
>>>
>>> The proper reset value wouldn't be NULL but INVALID_HANDLE_VALUE, which
>>> is defined like:
>>>
>>>
>>>
>>> #define INVALID_HANDLE_VALUE ((HANDLE)(LONG_PTR)-1)
>>>
>>>
>>> Trying this again... we patch this;
>
> @@ -61,10 +64,11 @@
> * use a [slower] mutex object, instead.
> */
> IF_WIN_OS_IS_UNICODE {
> InitializeCriticalSection(&(*mutex)->section);
> (*mutex)->type = thread_mutex_critical_section;
> + (*mutex)->handle = INVALID_HANDLE_VALUE;
> }
> #endif
> #if APR_HAS_ANSI_FS
>
>
Michael, sorry, I'm unable to make this change at this time.
The fix we agree on is already made by ylavic in;
http://svn.apache.org/viewvc?view=revision&revision=1667900
buried in a much larger contribution to 1.7.0. If you would like to offer
it as a patch to apr/trunk (and apr/branches/1.7.x) we need to evaluate how
INVALID_HANDLE_VALUE would behave in lieu of NULL across the functions.
I'm satisfied that the new NULL behavior is preferable to leaving a closed
handle lying around for double-closure, and that the defect in your PR is
'fixed'.
Re: APR thread_mutex_cleanup on windows mishandles being called twice
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Tue, Mar 19, 2019 at 12:10 PM William A Rowe Jr <wr...@rowe-clan.net>
wrote:
> On Tue, Mar 19, 2019 at 11:41 AM Michael Schlenker <
> Michael.Schlenker@contact-software.com> wrote:
>
>> *Von:* William A Rowe Jr [mailto:wrowe@rowe-clan.net]
>> *Gesendet:* Dienstag, 19. März 2019 16:36
>> *An:* Michael Schlenker <Mi...@contact-software.com>
>> *Cc:* Stefan Sperling <st...@stsp.name>; dev@apr.apache.org
>> *Betreff:* Re: APR thread_mutex_cleanup on windows mishandles being
>> called twice
>>
>> > >
>> > > You're right that acting on a corrupt memory is bad.
>> > >
>> > > It looks like your proposed patch detects and then skips a redundant
>> call to
>> > > DeleteCriticalSection() when a mutex is unlocked twice:
>> > >
>> > > if (lock->type == thread_mutex_critical_section) {
>> > > lock->type = -1;
>> > > DeleteCriticalSection(&lock->section);
>> > > + }
>> > > + else if (lock->type == -1) {
>> > > + /* do nothing */
>> > > }
>> > >
>> > > I'd prefer thread_mutex_cleanup() to loudly complain when this
>> happens,
>> > > and perhaps even abort the program, forcing API users to fix their
>> code.
>> >
>> > Agreed. That should probably be a kind of assert that aborts or fails
>> loud, not sure what the APR convention/function for that case would be.
>> >
>> > Just initializing the handle member to default INVALID_HANDLE is enough
>> to prevent the memory corruption/freed random handles.
>>
>> If this is the consensus, to avoid further corruption, I'd suggest we
>> reset (*mutex)->handle = NULL on first close.
>>
>>
>>
>> Still should trigger a fault where it is double-closed. Won't silently
>> continue to close random handles. And avoids the extra test.
>>
>>
>>
>> Do we agree on that band-aid?
>>
>>
>>
>> The proper reset value wouldn't be NULL but INVALID_HANDLE_VALUE, which
>> is defined like:
>>
>>
>>
>> #define INVALID_HANDLE_VALUE ((HANDLE)(LONG_PTR)-1)
>>
>>
>>
>> Otherwise agreed.
>>
>
Trying this again... we patch this;
@@ -61,10 +64,11 @@
* use a [slower] mutex object, instead.
*/
IF_WIN_OS_IS_UNICODE {
InitializeCriticalSection(&(*mutex)->section);
(*mutex)->type = thread_mutex_critical_section;
+ (*mutex)->handle = INVALID_HANDLE_VALUE;
}
#endif
#if APR_HAS_ANSI_FS
ELSE_WIN_OS_IS_ANSI {
(*mutex)->type = thread_mutex_nested_mutex;
And we omit this change, causing us to attempt to
CloseHandle(INVALID_HANDLE_VALUE)...
@@ -28,10 +28,13 @@
apr_thread_mutex_t *lock = data;
if (lock->type == thread_mutex_critical_section) {
lock->type = -1;
DeleteCriticalSection(&lock->section);
+ }
+ else if (lock->type == -1) {
+ /* do nothing */
}
else {
if (!CloseHandle(lock->handle)) {
return apr_get_os_error();
}
Just want to be clear that we concur, before I commit.
Re: APR thread_mutex_cleanup on windows mishandles being called twice
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Tue, Mar 19, 2019 at 11:41 AM Michael Schlenker <
Michael.Schlenker@contact-software.com> wrote:
> *Von:* William A Rowe Jr [mailto:wrowe@rowe-clan.net]
> *Gesendet:* Dienstag, 19. März 2019 16:36
> *An:* Michael Schlenker <Mi...@contact-software.com>
> *Cc:* Stefan Sperling <st...@stsp.name>; dev@apr.apache.org
> *Betreff:* Re: APR thread_mutex_cleanup on windows mishandles being
> called twice
>
> > >
> > > You're right that acting on a corrupt memory is bad.
> > >
> > > It looks like your proposed patch detects and then skips a redundant
> call to
> > > DeleteCriticalSection() when a mutex is unlocked twice:
> > >
> > > if (lock->type == thread_mutex_critical_section) {
> > > lock->type = -1;
> > > DeleteCriticalSection(&lock->section);
> > > + }
> > > + else if (lock->type == -1) {
> > > + /* do nothing */
> > > }
> > >
> > > I'd prefer thread_mutex_cleanup() to loudly complain when this happens,
> > > and perhaps even abort the program, forcing API users to fix their
> code.
> >
> > Agreed. That should probably be a kind of assert that aborts or fails
> loud, not sure what the APR convention/function for that case would be.
> >
> > Just initializing the handle member to default INVALID_HANDLE is enough
> to prevent the memory corruption/freed random handles.
>
> If this is the consensus, to avoid further corruption, I'd suggest we
> reset (*mutex)->handle = NULL on first close.
>
>
>
> Still should trigger a fault where it is double-closed. Won't silently
> continue to close random handles. And avoids the extra test.
>
>
>
> Do we agree on that band-aid?
>
>
>
> The proper reset value wouldn't be NULL but INVALID_HANDLE_VALUE, which is
> defined like:
>
>
>
> #define INVALID_HANDLE_VALUE ((HANDLE)(LONG_PTR)-1)
>
>
>
> Otherwise agreed.
>
>
And, drop the test, e.g...
we omit this change;
--- apr-1.6.5/locks/win32/thread_mutex.c 2017-05-24 17:21:43.000000000 +0200
+++ apr-1.6.5/locks/win32/thread_mutex.c 2019-01-09 14:27:59.041406600 +0100
@@ -28,10 +28,13 @@
apr_thread_mutex_t *lock = data;
if (lock->type == thread_mutex_critical_section) {
lock->type = -1;
DeleteCriticalSection(&lock->section);
+ }
+ else if (lock->type == -1) {
+ /* do nothing */
}
else {
if (!CloseHandle(lock->handle)) {
return apr_get_os_error();
}
--- apr-1.6.5/locks/win32/thread_mutex.c 2017-05-24 17:21:43.000000000 +0200
+++ apr-1.6.5/locks/win32/thread_mutex.c 2019-01-09 14:27:59.041406600
+0100@@ -61,10 +64,11 @@
* use a [slower] mutex object, instead.
*/
IF_WIN_OS_IS_UNICODE {
InitializeCriticalSection(&(*mutex)->section);
(*mutex)->type = thread_mutex_critical_section;
+ (*mutex)->handle = INVALID_HANDLE_VALUE;
}
#endif
#if APR_HAS_ANSI_FS
ELSE_WIN_OS_IS_ANSI {
(*mutex)->type = thread_mutex_nested_mutex;
AW: APR thread_mutex_cleanup on windows mishandles being called twice
Posted by Michael Schlenker <Mi...@contact-software.com>.
Von: William A Rowe Jr [mailto:wrowe@rowe-clan.net]
Gesendet: Dienstag, 19. März 2019 16:36
An: Michael Schlenker <Mi...@contact-software.com>
Cc: Stefan Sperling <st...@stsp.name>; dev@apr.apache.org
Betreff: Re: APR thread_mutex_cleanup on windows mishandles being called twice
> >
> > You're right that acting on a corrupt memory is bad.
> >
> > It looks like your proposed patch detects and then skips a redundant call to
> > DeleteCriticalSection() when a mutex is unlocked twice:
> >
> > if (lock->type == thread_mutex_critical_section) {
> > lock->type = -1;
> > DeleteCriticalSection(&lock->section);
> > + }
> > + else if (lock->type == -1) {
> > + /* do nothing */
> > }
> >
> > I'd prefer thread_mutex_cleanup() to loudly complain when this happens,
> > and perhaps even abort the program, forcing API users to fix their code.
>
> Agreed. That should probably be a kind of assert that aborts or fails loud, not sure what the APR convention/function for that case would be.
>
> Just initializing the handle member to default INVALID_HANDLE is enough to prevent the memory corruption/freed random handles.
If this is the consensus, to avoid further corruption, I'd suggest we reset (*mutex)->handle = NULL on first close.
Still should trigger a fault where it is double-closed. Won't silently continue to close random handles. And avoids the extra test.
Do we agree on that band-aid?
The proper reset value wouldn't be NULL but INVALID_HANDLE_VALUE, which is defined like:
#define INVALID_HANDLE_VALUE ((HANDLE)(LONG_PTR)-1)
Otherwise agreed.
Michael
Re: APR thread_mutex_cleanup on windows mishandles being called twice
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Tue, Mar 19, 2019 at 10:01 AM Michael Schlenker <
Michael.Schlenker@contact-software.com> wrote:
>
> > -----Ursprüngliche Nachricht-----
> > Von: Stefan Sperling [mailto:stsp@stsp.name]
> > Gesendet: Dienstag, 19. März 2019 15:35
> > An: Michael Schlenker <Mi...@contact-software.com>
> > Cc: dev@apr.apache.org
> > Betreff: Re: APR thread_mutex_cleanup on windows mishandles being
> > called twice
> >
> > On Tue, Mar 19, 2019 at 11:12:25AM +0000, Michael Schlenker wrote:
> > > Hi,
> > >
> > > just filed https://bz.apache.org/bugzilla/show_bug.cgi?id=63271
> > >
> > > (Patch attached)
> > >
> > > It seems to be the reason behind various crashes/restarts seen in
Apache
> > httpd on Windows, especially when mod_cache_disk is in use.
> > > Basically when the mutex is cleaned up twice, it calls CloseHandle()
> > > on uninitialized memory, which causes First Chance Exceptions in the
> > > debugger (if invalid handle) or closes some random Handle behind the
back
> > of its real owner (e.g. internal handles of the userspace leading to
access
> > violations inside CreateProcess, httpd Events used to signal between
parent
> > and child, etc.).
> > >
> > > It would be great if this could make it into 1.7.
> > >
> > > Thanks,
> > > Michael
> >
> > You're right that acting on a corrupt memory is bad.
> >
> > It looks like your proposed patch detects and then skips a redundant
call to
> > DeleteCriticalSection() when a mutex is unlocked twice:
> >
> > if (lock->type == thread_mutex_critical_section) {
> > lock->type = -1;
> > DeleteCriticalSection(&lock->section);
> > + }
> > + else if (lock->type == -1) {
> > + /* do nothing */
> > }
> >
> > I'd prefer thread_mutex_cleanup() to loudly complain when this happens,
> > and perhaps even abort the program, forcing API users to fix their code.
>
> Agreed. That should probably be a kind of assert that aborts or fails
loud, not sure what the APR convention/function for that case would be.
>
> Just initializing the handle member to default INVALID_HANDLE is enough
to prevent the memory corruption/freed random handles.
If this is the consensus, to avoid further corruption, I'd suggest we
reset (*mutex)->handle
= NULL on first close.
Still should trigger a fault where it is double-closed. Won't silently
continue to close random handles. And avoids the extra test.
Do we agree on that band-aid?
AW: APR thread_mutex_cleanup on windows mishandles being called twice
Posted by Michael Schlenker <Mi...@contact-software.com>.
> -----Ursprüngliche Nachricht-----
> Von: Stefan Sperling [mailto:stsp@stsp.name]
> Gesendet: Dienstag, 19. März 2019 15:35
> An: Michael Schlenker <Mi...@contact-software.com>
> Cc: dev@apr.apache.org
> Betreff: Re: APR thread_mutex_cleanup on windows mishandles being
> called twice
>
> On Tue, Mar 19, 2019 at 11:12:25AM +0000, Michael Schlenker wrote:
> > Hi,
> >
> > just filed https://bz.apache.org/bugzilla/show_bug.cgi?id=63271
> >
> > (Patch attached)
> >
> > It seems to be the reason behind various crashes/restarts seen in Apache
> httpd on Windows, especially when mod_cache_disk is in use.
> > Basically when the mutex is cleaned up twice, it calls CloseHandle()
> > on uninitialized memory, which causes First Chance Exceptions in the
> > debugger (if invalid handle) or closes some random Handle behind the back
> of its real owner (e.g. internal handles of the userspace leading to access
> violations inside CreateProcess, httpd Events used to signal between parent
> and child, etc.).
> >
> > It would be great if this could make it into 1.7.
> >
> > Thanks,
> > Michael
>
> You're right that acting on a corrupt memory is bad.
>
> It looks like your proposed patch detects and then skips a redundant call to
> DeleteCriticalSection() when a mutex is unlocked twice:
>
> if (lock->type == thread_mutex_critical_section) {
> lock->type = -1;
> DeleteCriticalSection(&lock->section);
> + }
> + else if (lock->type == -1) {
> + /* do nothing */
> }
>
> I'd prefer thread_mutex_cleanup() to loudly complain when this happens,
> and perhaps even abort the program, forcing API users to fix their code.
Agreed. That should probably be a kind of assert that aborts or fails loud, not sure what the APR convention/function for that case would be.
Just initializing the handle member to default INVALID_HANDLE is enough to prevent the memory corruption/freed random handles.
Michael
Re: APR thread_mutex_cleanup on windows mishandles being called twice
Posted by Stefan Sperling <st...@stsp.name>.
On Tue, Mar 19, 2019 at 11:12:25AM +0000, Michael Schlenker wrote:
> Hi,
>
> just filed https://bz.apache.org/bugzilla/show_bug.cgi?id=63271
>
> (Patch attached)
>
> It seems to be the reason behind various crashes/restarts seen in Apache httpd on Windows, especially when mod_cache_disk is in use.
> Basically when the mutex is cleaned up twice, it calls CloseHandle() on uninitialized memory, which causes First Chance Exceptions in the debugger
> (if invalid handle) or closes some random Handle behind the back of its real owner (e.g. internal handles of the userspace leading to access violations inside CreateProcess,
> httpd Events used to signal between parent and child, etc.).
>
> It would be great if this could make it into 1.7.
>
> Thanks,
> Michael
You're right that acting on a corrupt memory is bad.
It looks like your proposed patch detects and then skips a redundant
call to DeleteCriticalSection() when a mutex is unlocked twice:
if (lock->type == thread_mutex_critical_section) {
lock->type = -1;
DeleteCriticalSection(&lock->section);
+ }
+ else if (lock->type == -1) {
+ /* do nothing */
}
I'd prefer thread_mutex_cleanup() to loudly complain when this happens,
and perhaps even abort the program, forcing API users to fix their code.