From owner-freebsd-threads@FreeBSD.ORG Thu Jan 7 15:29:20 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 6502F106568F; Thu, 7 Jan 2010 15:29:20 +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 34ACA8FC24; Thu, 7 Jan 2010 15:29:20 +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 DA0FB46B49; Thu, 7 Jan 2010 10:29:19 -0500 (EST) Received: from jhbbsd.localnet (smtp.hudson-trading.com [209.249.190.9]) by bigwig.baldwin.cx (Postfix) with ESMTPA id 1553B8A01D; Thu, 7 Jan 2010 10:29:19 -0500 (EST) From: John Baldwin To: Jeremy Huddleston Date: Thu, 7 Jan 2010 09:56:25 -0500 User-Agent: KMail/1.12.1 (FreeBSD/7.2-CBSD-20091103; KDE/4.3.1; amd64; ; ) References: <200912052034.nB5KYfaY000395@www.freebsd.org> <201001061600.47628.jhb@freebsd.org> <56AAD075-8202-40C0-AC75-18A5CDC3B76A@apple.com> In-Reply-To: <56AAD075-8202-40C0-AC75-18A5CDC3B76A@apple.com> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-15" Content-Transfer-Encoding: 7bit Message-Id: <201001070956.25906.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.0.1 (bigwig.baldwin.cx); Thu, 07 Jan 2010 10:29:19 -0500 (EST) X-Virus-Scanned: clamav-milter 0.95.1 at bigwig.baldwin.cx X-Virus-Status: Clean X-Spam-Status: No, score=-2.5 required=4.2 tests=AWL,BAYES_00 autolearn=ham version=3.2.5 X-Spam-Checker-Version: SpamAssassin 3.2.5 (2008-06-10) on bigwig.baldwin.cx Cc: freebsd-gnats-submit@freebsd.org, freebsd-threads@freebsd.org Subject: Re: threads/141198: src/lib/libc/stdio does not properly initialize mutexes 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: Thu, 07 Jan 2010 15:29:20 -0000 On Wednesday 06 January 2010 7:00:14 pm Jeremy Huddleston wrote: > I don't think that is sufficient. > > On Jan 6, 2010, at 16:00, John Baldwin wrote: > > > On Monday 07 December 2009 8:50:37 am John Baldwin wrote: > >> On Saturday 05 December 2009 3:34:41 pm Jeremy Huddleston wrote: > >>> > >>>> Number: 141198 > >>>> Category: threads > >>>> Synopsis: src/lib/libc/stdio does not properly initialize mutexes > >>>> Confidential: no > >>>> Severity: non-critical > >>>> Priority: low > >>>> Responsible: freebsd-threads > >>>> State: open > >>>> Quarter: > >>>> Keywords: > >>>> Date-Required: > >>>> Class: sw-bug > >>>> Submitter-Id: current-users > >>>> Arrival-Date: Sat Dec 05 20:40:01 UTC 2009 > >>>> Closed-Date: > >>>> Last-Modified: > >>>> Originator: Jeremy Huddleston > >>>> Release: 8.0 > >>>> Organization: > >>> Apple > >>>> Environment: > >>> NA > >>>> Description: > >>> libc/stdio assumes PTHREAD_MUTEX_INITIALIZER is NULL (which it is in > >> FreeBSD), but this makes the code not as portable. > >>> > >>> Earlier versions of stdio did properly initialize the lock to > >> PTHREAD_MUTEX_INITIALIZER in INITEXTRA() when it was part of the _extra > >> extension substructure. > >> > >> This is my fault. I suspect it was more an error of omission on my part than > >> assuming the default value of PTHREAD_MUTEX_INITIALIZER. :) Hmm, so I > >> reviewed the change that removed INITEXTRA() and all the places it was used to > >> construct a 'fake' FILE object it was passed to an internal stdio routine that > >> did not use locking. One thing I do see that is an old bug is that the three > >> static FILE structures used for stdin, stdout, and stderr have never had their > >> internal locks properly initialized. Also, it seems __sfp() never initializes > >> fp->_lock at all. Oh, it gets set to NULL via 'empty' in moreglue(). That is > >> also an old bug. I think this will fix those problems: > >> > >> Index: stdio/findfp.c > >> =================================================================== > >> --- stdio/findfp.c (revision 199969) > >> +++ stdio/findfp.c (working copy) > >> @@ -61,6 +61,7 @@ > >> ._read = __sread, \ > >> ._seek = __sseek, \ > >> ._write = __swrite, \ > >> + ._fl_mutex = PTHREAD_MUTEX_INITIALIZER, \ > >> } > >> /* the usual - (stdin + stdout + stderr) */ > >> static FILE usual[FOPEN_MAX - 3]; > >> @@ -96,7 +97,7 @@ > >> int n; > >> { > >> struct glue *g; > >> - static FILE empty; > >> + static FILE empty = { ._fl_mutex = PTHREAD_MUTEX_INITIALIZER }; > >> FILE *p; > >> > >> g = (struct glue *)malloc(sizeof(*g) + ALIGNBYTES + n * sizeof(FILE)); > >> @@ -154,7 +155,7 @@ > >> fp->_ub._size = 0; > >> fp->_lb._base = NULL; /* no line buffer */ > >> fp->_lb._size = 0; > >> -/* fp->_lock = NULL; */ /* once set always set (reused) */ > >> +/* fp->_fl_mutex = NULL; */ /* once set always set (reused) */ > >> fp->_orientation = 0; > >> memset(&fp->_mbstate, 0, sizeof(mbstate_t)); > >> return (fp); > > > > Does this patch address the concerns? > > I'm not sure if that is sufficient. I added it there and as part of INITEXTRA (which we reverted to in darwin) in the following. I can provide you with the full patches if you want, but there is a lot of noise in them for our implementation of the _l variants and whatnot. I think the following might not need to be initialized, but I did it for good measure: > > vasprintf.c.patch:+ INITEXTRA(&f); > vdprintf.c.patch:+ INITEXTRA(&f); > vfprintf.c.patch:+ INITEXTRA(&fake); > vfwprintf.c.patch:+ INITEXTRA(&fake); > vsnprintf.c.patch:+ INITEXTRA(&f); > vsprintf.c.patch:+ INITEXTRA(&f); > vsscanf.c.patch:+ INITEXTRA(&f); > vswprintf.c.patch:+ INITEXTRA(&f); > vswscanf.c.patch:+ INITEXTRA(&f); Ah, ok. In our stdio at least these are all dummy files that are passed to internal stdio routines that never do any locking (e.g. __vfprintf() which does no locking vs vfprintf() which does use the mutex locks). I'm not sure if that is also true for Darwin, but in theory it should be as these file objects are all private stack variables that no other threads can gain a reference to, so no locking is needed. -- John Baldwin