syslog() thread unsafety

[Konstantin Belousov]

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

OTOH, if you configured the thread for async cancellation mode, then
cancellation can happen anywhere. If you called any non-signal-safe
function, any invariant can be broken. That is, the cancellation cleanup
is not supposed to be useful with async cancellation.  And you are not
supposed to call into stdio or syslog() while in async.

> 
> Anyway, we have several other places in the lib/ with similar code
> possibly missing pthread_cleanup_push():
> 
> lib/libc/gen: popen.c,
I doubt that popen() needs the cancellation protection.  The _close()
calls there are explicitely not the cancellation points, they are
direct syscall invocations, and are not subject to cancellation testing.

The only potentially problematic case could be the fclose() call in the locked
region.  But again, fclose(3) only potential cancellation point is the
fp->_close() call.  For fdopen()-ed FILEs, _close is set to the __sclose()
wrapper which only calls _close().

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

> 
> Please consider committing the fix at least for syslog.c
Confirm that it fixed your case, then I will commit the change.