Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Fri, 2 Nov 2018 10:29:15 -0400
From: Rich Felker <dalias@...c.org>
To: "dirk@...iot.com" <dirk@...iot.com>
Cc: musl <musl@...ts.openwall.com>
Subject: Re: Deadlock when calling fflush/fclose in multiple threads

On Fri, Nov 02, 2018 at 01:11:00PM +0800, dirk@...iot.com wrote:
> Hi,
> 
> We got deadlock on fflush/fclose with musl-1.1.19 (openwrt 18.06).
> Actually we using lua's popen in mutiple threads, following is gdb
> trace.
> 
> I am new to musl libc source code, fflush(NULL) will call __ofl_lock
> and then try to lock and flush every stream, fclose will lock the
> stream and then __ofl_lock. The question is the fflush/fclose api
> thread-safe? What i have got from man document is that linux
> fflush/fclose is thread-safe api.

Your analysis is exactly correct. Calling fflush(NULL) frequently (or
at all) is a really bad idea because of how it scales and how
serializing it is, but it is valid, and the deadlock is a bug in musl.

The current placement of the ofl update seems to have been based on
minimizing how serializing fclose is, and on avoiding taking the
global lock for F_PERM (stdin/out/err) FILEs (which is largely a
useless optimization since the operation can happen at most 3 times).
Just moving it above the FLOCK (and making it not conditional on
F_PERM, to avoid data races) would solve this, but there's a deeper
bug here too.

By removing the FILE being closed from the open file list (and
unlocking the open file list, without which the removal can't be seen)
before it's flushed and closed, fclose creates a race window where
fflush(NULL) or exit() from another thread can complete without this
file being flushed, potentially causing data loss.

I think we just have to move the __ofl_lock to the top of the
function, before FLOCK, and the __ofl_unlock to after the
fflush/close. Unfortunately this makes fclose much more serializing
than it was before, but I don't see any way to avoid it.

Rich

Powered by blists - more mailing lists

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.