From owner-freebsd-stable@freebsd.org Wed Jun 14 17:09:09 2017 Return-Path: Delivered-To: freebsd-stable@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 8FE07BEFA8C for ; Wed, 14 Jun 2017 17:09:09 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from kib.kiev.ua (kib.kiev.ua [IPv6:2001:470:d5e7:1::1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 376D471982; Wed, 14 Jun 2017 17:09:09 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from tom.home (kib@localhost [127.0.0.1]) by kib.kiev.ua (8.15.2/8.15.2) with ESMTPS id v5EH94BL011855 (version=TLSv1.2 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Wed, 14 Jun 2017 20:09:04 +0300 (EEST) (envelope-from kostikbel@gmail.com) DKIM-Filter: OpenDKIM Filter v2.10.3 kib.kiev.ua v5EH94BL011855 Received: (from kostik@localhost) by tom.home (8.15.2/8.15.2/Submit) id v5EH94GS011854; Wed, 14 Jun 2017 20:09:04 +0300 (EEST) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: tom.home: kostik set sender to kostikbel@gmail.com using -f Date: Wed, 14 Jun 2017 20:09:04 +0300 From: Konstantin Belousov To: Eugene Grosbein Cc: FreeBSD Stable , Gleb Smirnoff Subject: Re: syslog() thread unsafety Message-ID: <20170614170904.GR2088@kib.kiev.ua> References: <59413EF0.20608@grosbein.net> <20170614141238.GM2088@kib.kiev.ua> <594166CB.60709@grosbein.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <594166CB.60709@grosbein.net> User-Agent: Mutt/1.8.2 (2017-04-18) X-Spam-Status: No, score=-2.0 required=5.0 tests=ALL_TRUSTED,BAYES_00, DKIM_ADSP_CUSTOM_MED,FREEMAIL_FROM,NML_ADSP_CUSTOM_MED autolearn=no autolearn_force=no version=3.4.1 X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on tom.home X-BeenThere: freebsd-stable@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Production branch of FreeBSD source code List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 14 Jun 2017 17:09:09 -0000 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.