On Tue, Mar 19, 2019 at 10:01 AM Michael Schlenker < michael.schlen...@contact-software.com> wrote: > > > -----Ursprüngliche Nachricht----- > > Von: Stefan Sperling [mailto:s...@stsp.name] > > Gesendet: Dienstag, 19. März 2019 15:35 > > An: Michael Schlenker <michael.schlen...@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?
