On Tue, Mar 19, 2019 at 11:41 AM Michael Schlenker < michael.schlen...@contact-software.com> wrote:
> *Von:* William A Rowe Jr [mailto:wr...@rowe-clan.net] > *Gesendet:* Dienstag, 19. März 2019 16:36 > *An:* Michael Schlenker <michael.schlen...@contact-software.com> > *Cc:* Stefan Sperling <s...@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;
