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