syslog() thread unsafety
eugen at grosbein.net
Thu Jun 15 03:38:06 UTC 2017
15.06.2017 0:09, Konstantin Belousov wrote:
> On Wed, Jun 14, 2017 at 11:39:39PM +0700, Eugene Grosbein wrote:
>> 14.06.2017 21:12, Konstantin Belousov wrote:
>>> If the issue is that mpd5 cancels logging thread, and this leaves the
>>> mutex in the locked state, the right solution is to establish a cleanup
>>> handler around the locked region. Note that this can only work if the
>>> cancellation is in deferred mode, async mode is unsafe by definition.
>>> Try something like this, untested even a minimal bit.
>> I've given it a spin with unpatched mpd5 and it seems to work just fine now.
>> I'm curious, should these two lines be swapped?
>> + THREAD_LOCK();
>> + pthread_cleanup_push(syslog_cancel_cleanup, NULL);
>> It seems it could be a race between another thread's pthread_cancel()
>> and pthread_cleanup_push() here.
> As I mentioned in my previous reply, you can only rely on some
> guarantees for the deferred cancellation mode. In this mode, only some
> calls are allowed to become cancellation points. In particular, the
> cancellation cannot happen between THREAD_LOCK() and push().
Forgot to mention that mpd5 does not change the mode to PTHREAD_CANCEL_ASYNCHRONOUS
from the default deferred.
> That is, popen() looks safe.
>> lib/libc/stdio: findfp.c, fclose.c
> I already wrote about fclose() above, might be it indeed should grow the
> cleanup handler for the general case. I do not see any code which would
> need cancellation protection in findfp, what exactly do you mean there ?
> Same for getlogin().
That was just quick scan for THREAD_LOCK() :-)
I'm new to these details of pthreads.
>> Please consider committing the fix at least for syslog.c
> Confirm that it fixed your case, then I will commit the change.
Well, my stress test for mpd5 has been running for several hours
and no hang still. Without your patch, it locked in a minute,
so it seems the case is fixed, thanks!
More information about the freebsd-stable