Date: Thu, 15 Jun 2017 10:37:50 +0700 From: Eugene Grosbein <eugen@grosbein.net> To: Konstantin Belousov <kostikbel@gmail.com> Cc: FreeBSD Stable <freebsd-stable@FreeBSD.org>, Gleb Smirnoff <glebius@FreeBSD.org> Subject: Re: syslog() thread unsafety Message-ID: <5942010E.40907@grosbein.net> In-Reply-To: <20170614170904.GR2088@kib.kiev.ua> References: <59413EF0.20608@grosbein.net> <20170614141238.GM2088@kib.kiev.ua> <594166CB.60709@grosbein.net> <20170614170904.GR2088@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?5942010E.40907>