syslog() thread unsafety

[Eugene Grosbein]

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.
>>
>> [skip]
>>
>> 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.
>> getlogin.c
>> 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!

Eugene Grosbein