From owner-freebsd-threads@FreeBSD.ORG  Wed Dec  8 04:08:11 2010
Return-Path: <owner-freebsd-threads@FreeBSD.ORG>
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 B0A11106564A;
	Wed,  8 Dec 2010 04:08:11 +0000 (UTC)
	(envelope-from davidxu@freebsd.org)
Received: from freefall.freebsd.org (freefall.freebsd.org
	[IPv6:2001:4f8:fff6::28])
	by mx1.freebsd.org (Postfix) with ESMTP id 9F2998FC0C;
	Wed,  8 Dec 2010 04:08:11 +0000 (UTC)
Received: from xyf.my.dom (localhost [127.0.0.1])
	by freefall.freebsd.org (8.14.4/8.14.4) with ESMTP id oB848ARx056082;
	Wed, 8 Dec 2010 04:08:11 GMT (envelope-from davidxu@freebsd.org)
Message-ID: <4CFF04AA.6060905@freebsd.org>
Date: Wed, 08 Dec 2010 12:08:10 +0800
From: David Xu <davidxu@freebsd.org>
User-Agent: Thunderbird 2.0.0.24 (X11/20100630)
MIME-Version: 1.0
To: Daniel Eischen <deischen@freebsd.org>
References: <201012080250.oB82o6X5072936@freefall.freebsd.org>
	<Pine.GSO.4.64.1012072245570.20951@sea.ntplx.net>
In-Reply-To: <Pine.GSO.4.64.1012072245570.20951@sea.ntplx.net>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Cc: freebsd-threads@freebsd.org
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 <freebsd-threads.freebsd.org>
List-Unsubscribe: <http://lists.freebsd.org/mailman/listinfo/freebsd-threads>, 
	<mailto:freebsd-threads-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/freebsd-threads>
List-Post: <mailto:freebsd-threads@freebsd.org>
List-Help: <mailto:freebsd-threads-request@freebsd.org?subject=help>
List-Subscribe: <http://lists.freebsd.org/mailman/listinfo/freebsd-threads>,
	<mailto:freebsd-threads-request@freebsd.org?subject=subscribe>
X-List-Received-Date: Wed, 08 Dec 2010 04:08:11 -0000

Daniel Eischen wrote:
> On Wed, 8 Dec 2010, 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.
> 
> I can do it, but will be away on vacation until later next
> week.  If you want to wait, I can commit it.
> 
> Would you like to replace the (*fp->_close)(fp->_cookie)
> with just _close(fp->_file) as you suggest in one of your
> followups?
> 
Thanks, I think you can keep it.