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

[John Baldwin]

The following reply was made to PR threads/141198; it has been noted by GNATS.

From: John Baldwin <jhb at freebsd.org>
To: freebsd-threads at freebsd.org
Cc: freebsd-gnats-submit at freebsd.org,
 Jeremy Huddleston <jeremyhu at apple.com>
Subject: Re: threads/141198: src/lib/libc/stdio does not properly initialize mutexes
Date: Wed, 6 Jan 2010 16:00:47 -0500

 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?
 
 -- 
 John Baldwin