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.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. >> > 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.
