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

[John Baldwin]

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