From owner-freebsd-threads@FreeBSD.ORG Wed Dec 8 15:39:19 2010 Return-Path: Delivered-To: freebsd-threads@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id C0301106564A for ; Wed, 8 Dec 2010 15:39:19 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from cyrus.watson.org (cyrus.watson.org [65.122.17.42]) by mx1.freebsd.org (Postfix) with ESMTP id 8F5728FC12 for ; Wed, 8 Dec 2010 15:39:19 +0000 (UTC) Received: from bigwig.baldwin.cx (66.111.2.69.static.nyinternet.net [66.111.2.69]) by cyrus.watson.org (Postfix) with ESMTPSA id 3B38E46B35 for ; Wed, 8 Dec 2010 10:39:19 -0500 (EST) Received: from jhbbsd.localnet (smtp.hudson-trading.com [209.249.190.9]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id 2D6478A01D for ; Wed, 8 Dec 2010 10:39:18 -0500 (EST) From: John Baldwin To: freebsd-threads@freebsd.org Date: Wed, 8 Dec 2010 10:39:16 -0500 User-Agent: KMail/1.13.5 (FreeBSD/7.3-CBSD-20101102; KDE/4.4.5; amd64; ; ) References: <201012081520.oB8FKDB2088586@freefall.freebsd.org> In-Reply-To: <201012081520.oB8FKDB2088586@freefall.freebsd.org> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-15" Content-Transfer-Encoding: 7bit Message-Id: <201012081039.16470.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.6 (bigwig.baldwin.cx); Wed, 08 Dec 2010 10:39:18 -0500 (EST) X-Virus-Scanned: clamav-milter 0.96.3 at bigwig.baldwin.cx X-Virus-Status: Clean X-Spam-Status: No, score=-1.9 required=4.2 tests=BAYES_00 autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on bigwig.baldwin.cx Subject: Re: threads/79887: [patch] freopen() isn't thread-safe X-BeenThere: freebsd-threads@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Threading on FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 08 Dec 2010 15:39:19 -0000 On Wednesday, December 08, 2010 10:20:13 am John Baldwin wrote: > The following reply was made to PR threads/79887; it has been noted by GNATS. > > From: John Baldwin > To: David Xu > Cc: bug-followup@freebsd.org, > tejblum@yandex-team.ru > Subject: Re: threads/79887: [patch] freopen() isn't thread-safe > Date: Wed, 8 Dec 2010 10:03:34 -0500 > > On Tuesday, December 07, 2010 9:43:35 pm David Xu wrote: > > John Baldwin wrote: > > > David, > > > > > > I think the submitter's analysis is correct that the only place that can set > > > the close function pointer is funopen() and that for that case (and any other > > > "fake" files), the file descriptor will be -1. If the fd is >= 0, then it > > > must be a file-descriptor-backed FILE, and relying on dup2() to close the fd > > > is ok. > > > > > > As the manpage notes, the most common usage is to redirect stderr or stdout by > > > doing 'freopen("/dev/null", "w", stderr)'. The bug allows some other random > > > code that is calling open() in another thread to have that open() return 2 > > > during the window where fd '2' is closed during freopen(). That other file > > > descriptor then gets trounced by the dup2() call in freopen() to point to > > > something else. > > > > > > The code likely uses _close() rather than close() directly to be cleaner. > > > Given that this is stdio, I don't think we are really worried about the > > > performance impact of one extra wrapper function. > > > > > > I think the original patch is most likely correct. > > > > > > > The patch works, I just don't like the design of the > > (*fp->_close)(fp->_cookie) > > it seems the patch make freopen bypass it. > > I think the patch can be committed, but I am busy and have > > no time to do it by myself. > > Actually, the freopen() code honors custom _close() routines earlier when it > checks for _file being < 0. I do really think this is ok. _close() is not > public, it is only allowed to be set via funopen(). We also need the dup2() > change to effectively implement this function's rationale, which is a way to > redirect stdin, stdout, and stderr. > > I will take care of committing this today, with an extra bit of comment. I hadn't seen Daniel's reply when I wrote this last bit. Here is a slightly updated version of the patch with an extra comment and a change to use _close() directly: Index: freopen.c =================================================================== --- freopen.c (revision 216266) +++ freopen.c (working copy) @@ -150,14 +150,6 @@ /* Get a new descriptor to refer to the new file. */ f = _open(file, oflags, DEFFILEMODE); - if (f < 0 && isopen) { - /* If out of fd's close the old one and try again. */ - if (errno == ENFILE || errno == EMFILE) { - (void) (*fp->_close)(fp->_cookie); - isopen = 0; - f = _open(file, oflags, DEFFILEMODE); - } - } sverrno = errno; finish: @@ -165,9 +157,11 @@ * Finish closing fp. Even if the open succeeded above, we cannot * keep fp->_base: it may be the wrong size. This loses the effect * of any setbuffer calls, but stdio has always done this before. + * + * Leave the existing file descriptor open until dup2() is called + * below to avoid races where a concurrent open() in another thread + * could claim the existing descriptor. */ - if (isopen) - (void) (*fp->_close)(fp->_cookie); if (fp->_flags & __SMBF) free((char *)fp->_bf._base); fp->_w = 0; @@ -186,6 +180,8 @@ memset(&fp->_mbstate, 0, sizeof(mbstate_t)); if (f < 0) { /* did not get it after all */ + if (isopen) + (void) (*fp->_close)(fp->_cookie); fp->_flags = 0; /* set it free */ FUNLOCKFILE(fp); errno = sverrno; /* restore in case _close clobbered */ @@ -197,11 +193,12 @@ * to maintain the descriptor. Various C library routines (perror) * assume stderr is always fd STDERR_FILENO, even if being freopen'd. */ - if (wantfd >= 0 && f != wantfd) { + if (wantfd >= 0) { if (_dup2(f, wantfd) >= 0) { (void)_close(f); f = wantfd; - } + } else + (void)_close(fp->_file); } /* We could use 'wantfd' instead of 'fp->_file' in the last hunk above if folks feel that is clearer. -- John Baldwin