threads/141198: src/lib/libc/stdio does not properly initialize mutexes

[Jeremy Huddleston]

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);