From owner-freebsd-threads@FreeBSD.ORG Thu Jan 7 00:19:52 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 43CDE1065732; Thu, 7 Jan 2010 00:19:52 +0000 (UTC) (envelope-from jeremyhu@apple.com) Received: from mail-out3.apple.com (mail-out3.apple.com [17.254.13.22]) by mx1.freebsd.org (Postfix) with ESMTP id 25C8F8FC20; Thu, 7 Jan 2010 00:19:52 +0000 (UTC) Received: from relay16.apple.com (relay16.apple.com [17.128.113.55]) by mail-out3.apple.com (Postfix) with ESMTP id 8EDAA7F3B677; Wed, 6 Jan 2010 16:00:15 -0800 (PST) X-AuditID: 11807137-b7bd4ae000000f0d-e7-4b45240e6aaf Received: from [17.151.90.43] (Unknown_Domain [17.151.90.43]) by relay16.apple.com (Apple SCV relay) with SMTP id 81.DF.03853.E04254B4; Wed, 6 Jan 2010 16:00:15 -0800 (PST) Mime-Version: 1.0 (Apple Message framework v1077) Content-Type: multipart/signed; boundary=Apple-Mail-17-1047373119; protocol="application/pkcs7-signature"; micalg=sha1 From: Jeremy Huddleston In-Reply-To: <201001061600.47628.jhb@freebsd.org> Date: Wed, 6 Jan 2010 19:00:14 -0500 Message-Id: <56AAD075-8202-40C0-AC75-18A5CDC3B76A@apple.com> References: <200912052034.nB5KYfaY000395@www.freebsd.org> <200912070850.37112.jhb@freebsd.org> <201001061600.47628.jhb@freebsd.org> To: John Baldwin X-Mailer: Apple Mail (2.1077) X-Brightmail-Tracker: AAAAAQAAAZE= X-Content-Filtered-By: Mailman/MimeDel 2.1.5 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 00:19:52 -0000 --Apple-Mail-17-1047373119 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=us-ascii 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: >>>=20 >>>> 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: =20 >>>> Keywords: =20 >>>> 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=20= >> FreeBSD), but this makes the code not as portable. >>>=20 >>> Earlier versions of stdio did properly initialize the lock to=20 >> PTHREAD_MUTEX_INITIALIZER in INITEXTRA() when it was part of the = _extra=20 >> extension substructure. >>=20 >> This is my fault. I suspect it was more an error of omission on my = part than=20 >> assuming the default value of PTHREAD_MUTEX_INITIALIZER. :) Hmm, so = I=20 >> reviewed the change that removed INITEXTRA() and all the places it = was used to=20 >> construct a 'fake' FILE object it was passed to an internal stdio = routine that=20 >> did not use locking. One thing I do see that is an old bug is that = the three=20 >> static FILE structures used for stdin, stdout, and stderr have never = had their=20 >> internal locks properly initialized. Also, it seems __sfp() never = initializes=20 >> fp->_lock at all. Oh, it gets set to NULL via 'empty' in moreglue(). = That is=20 >> also an old bug. I think this will fix those problems: >>=20 >> Index: stdio/findfp.c >> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >> --- stdio/findfp.c (revision 199969) >> +++ stdio/findfp.c (working copy) >> @@ -61,6 +61,7 @@ >> ._read =3D __sread, \ >> ._seek =3D __sseek, \ >> ._write =3D __swrite, \ >> + ._fl_mutex =3D 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 =3D { ._fl_mutex =3D PTHREAD_MUTEX_INITIALIZER = }; >> FILE *p; >>=20 >> g =3D (struct glue *)malloc(sizeof(*g) + ALIGNBYTES + n * = sizeof(FILE)); >> @@ -154,7 +155,7 @@ >> fp->_ub._size =3D 0; >> fp->_lb._base =3D NULL; /* no line buffer */ >> fp->_lb._size =3D 0; >> -/* fp->_lock =3D NULL; */ /* once set always set (reused) */ >> +/* fp->_fl_mutex =3D NULL; */ /* once set always set (reused) */ >> fp->_orientation =3D 0; >> memset(&fp->_mbstate, 0, sizeof(mbstate_t)); >> return (fp); >=20 > 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); --Apple-Mail-17-1047373119--