Date: Wed, 6 Jan 2010 19:00:14 -0500 From: Jeremy Huddleston <jeremyhu@apple.com> To: John Baldwin <jhb@freebsd.org> Cc: freebsd-gnats-submit@freebsd.org, freebsd-threads@freebsd.org Subject: Re: threads/141198: src/lib/libc/stdio does not properly initialize mutexes Message-ID: <56AAD075-8202-40C0-AC75-18A5CDC3B76A@apple.com> In-Reply-To: <201001061600.47628.jhb@freebsd.org> References: <200912052034.nB5KYfaY000395@www.freebsd.org> <200912070850.37112.jhb@freebsd.org> <201001061600.47628.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--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--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?56AAD075-8202-40C0-AC75-18A5CDC3B76A>