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

John Baldwin jhb at freebsd.org
Thu Jan 7 18:20:03 UTC 2010


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

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

 On Thursday 07 January 2010 10:44:54 am Jeremy Huddleston wrote:
 > 
 > On Jan 7, 2010, at 09:56, John Baldwin wrote:
 > 
 > >> 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.
 > 
 > Yeah, we're just being cautious with these changes.  It takes one clock cycle
 > and maintains the old (FBSD 7?) state of initializing the mutex during
 > INITEXTRA in those dumies... just in case something gets added down the line
 > which needs it.
 > 
 > If you're confident that won't be the case for FBSD, then I believe your
 > initial patch is sufficient.
 
 Ok.  I actually think it would be nicer to provide some sort of macro to
 properly initialize the various 'fake' FILE objects.  I will probably look
 at doing that in which case including an initializer for the lock should be
 simple.  I actually don't like the amount of duplicated code to setup fake
 FILE objects as it is.
 
 Here is a larger patch which does this in addition to the earlier patch.
 
 Index: stdio/vsnprintf.c
 ===================================================================
 --- stdio/vsnprintf.c	(revision 201516)
 +++ stdio/vsnprintf.c	(working copy)
 @@ -47,7 +47,7 @@
  	size_t on;
  	int ret;
  	char dummy[2];
 -	FILE f;
 +	FILE f = FAKE_FILE;
  
  	on = n;
  	if (n != 0)
 @@ -61,12 +61,9 @@
  		str = dummy;
  		n = 1;
  	}
 -	f._file = -1;
  	f._flags = __SWR | __SSTR;
  	f._bf._base = f._p = (unsigned char *)str;
  	f._bf._size = f._w = n;
 -	f._orientation = 0;
 -	memset(&f._mbstate, 0, sizeof(mbstate_t));
  	ret = __vfprintf(&f, fmt, ap);
  	if (on > 0)
  		*f._p = '\0';
 Index: stdio/vswprintf.c
 ===================================================================
 --- stdio/vswprintf.c	(revision 201516)
 +++ stdio/vswprintf.c	(working copy)
 @@ -45,7 +45,7 @@
  {
  	static const mbstate_t initial;
  	mbstate_t mbs;
 -	FILE f;
 +	FILE f = FAKE_FILE;
  	char *mbp;
  	int ret, sverrno;
  	size_t nwc;
 @@ -55,7 +55,6 @@
  		return (-1);
  	}
  
 -	f._file = -1;
  	f._flags = __SWR | __SSTR | __SALC;
  	f._bf._base = f._p = (unsigned char *)malloc(128);
  	if (f._bf._base == NULL) {
 @@ -63,8 +62,6 @@
  		return (-1);
  	}
  	f._bf._size = f._w = 127;		/* Leave room for the NUL */
 -	f._orientation = 0;
 -	memset(&f._mbstate, 0, sizeof(mbstate_t));
  	ret = __vfwprintf(&f, fmt, ap);
  	if (ret < 0) {
  		sverrno = errno;
 Index: stdio/vsscanf.c
 ===================================================================
 --- stdio/vsscanf.c	(revision 201516)
 +++ stdio/vsscanf.c	(working copy)
 @@ -55,16 +55,11 @@
  vsscanf(const char * __restrict str, const char * __restrict fmt,
  	__va_list ap)
  {
 -	FILE f;
 +	FILE f = FAKE_FILE;
  
 -	f._file = -1;
  	f._flags = __SRD;
  	f._bf._base = f._p = (unsigned char *)str;
  	f._bf._size = f._r = strlen(str);
  	f._read = eofread;
 -	f._ub._base = NULL;
 -	f._lb._base = NULL;
 -	f._orientation = 0;
 -	memset(&f._mbstate, 0, sizeof(mbstate_t));
  	return (__svfscanf(&f, fmt, ap));
  }
 Index: stdio/snprintf.c
 ===================================================================
 --- stdio/snprintf.c	(revision 201516)
 +++ stdio/snprintf.c	(working copy)
 @@ -48,7 +48,7 @@
  	size_t on;
  	int ret;
  	va_list ap;
 -	FILE f;
 +	FILE f = FAKE_FILE;
  
  	on = n;
  	if (n != 0)
 @@ -56,12 +56,9 @@
  	if (n > INT_MAX)
  		n = INT_MAX;
  	va_start(ap, fmt);
 -	f._file = -1;
  	f._flags = __SWR | __SSTR;
  	f._bf._base = f._p = (unsigned char *)str;
  	f._bf._size = f._w = n;
 -	f._orientation = 0;
 -	memset(&f._mbstate, 0, sizeof(mbstate_t));
  	ret = __vfprintf(&f, fmt, ap);
  	if (on > 0)
  		*f._p = '\0';
 Index: stdio/xprintf.c
 ===================================================================
 --- stdio/xprintf.c	(revision 201516)
 +++ stdio/xprintf.c	(working copy)
 @@ -48,6 +48,7 @@
  #include <wchar.h>
  #include "un-namespace.h"
  
 +#include "local.h"
  #include "printf.h"
  #include "fvwrite.h"
  
 @@ -575,7 +576,7 @@
  __v3printf(FILE *fp, const char *fmt, int pct, va_list ap)
  {
  	int ret;
 -	FILE fake;
 +	FILE fake = FAKE_FILE;
  	unsigned char buf[BUFSIZ];
  
  	/* copy the important variables */
 Index: stdio/vdprintf.c
 ===================================================================
 --- stdio/vdprintf.c	(revision 201516)
 +++ stdio/vdprintf.c	(working copy)
 @@ -39,7 +39,7 @@
  int
  vdprintf(int fd, const char * __restrict fmt, va_list ap)
  {
 -	FILE f;
 +	FILE f = FAKE_FILE;
  	unsigned char buf[BUFSIZ];
  	int ret;
  
 @@ -56,8 +56,6 @@
  	f._write = __swrite;
  	f._bf._base = buf;
  	f._bf._size = sizeof(buf);
 -	f._orientation = 0;
 -	bzero(&f._mbstate, sizeof(f._mbstate));
  
  	if ((ret = __vfprintf(&f, fmt, ap)) < 0)
  		return (ret);
 Index: stdio/vfprintf.c
 ===================================================================
 --- stdio/vfprintf.c	(revision 201516)
 +++ stdio/vfprintf.c	(working copy)
 @@ -169,7 +169,7 @@
  __sbprintf(FILE *fp, const char *fmt, va_list ap)
  {
  	int ret;
 -	FILE fake;
 +	FILE fake = FAKE_FILE;
  	unsigned char buf[BUFSIZ];
  
  	/* XXX This is probably not needed. */
 Index: stdio/local.h
 ===================================================================
 --- stdio/local.h	(revision 201516)
 +++ stdio/local.h	(working copy)
 @@ -110,6 +110,14 @@
  }
  
  /*
 + * Structure initializations for 'fake' FILE objects.
 + */
 +#define	FAKE_FILE {				\
 +	._file = -1,				\
 +	._fl_mutex = PTHREAD_MUTEX_INITIALIZER, \
 +}
 +
 +/*
   * Set the orientation for a stream. If o > 0, the stream has wide-
   * orientation. If o < 0, the stream has byte-orientation.
   */
 Index: stdio/vsprintf.c
 ===================================================================
 --- stdio/vsprintf.c	(revision 201516)
 +++ stdio/vsprintf.c	(working copy)
 @@ -44,14 +44,11 @@
  vsprintf(char * __restrict str, const char * __restrict fmt, __va_list ap)
  {
  	int ret;
 -	FILE f;
 +	FILE f = FAKE_FILE;
  
 -	f._file = -1;
  	f._flags = __SWR | __SSTR;
  	f._bf._base = f._p = (unsigned char *)str;
  	f._bf._size = f._w = INT_MAX;
 -	f._orientation = 0;
 -	memset(&f._mbstate, 0, sizeof(mbstate_t));
  	ret = __vfprintf(&f, fmt, ap);
  	*f._p = 0;
  	return (ret);
 Index: stdio/findfp.c
 ===================================================================
 --- stdio/findfp.c	(revision 201516)
 +++ 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);
 Index: stdio/vasprintf.c
 ===================================================================
 --- stdio/vasprintf.c	(revision 201516)
 +++ stdio/vasprintf.c	(working copy)
 @@ -42,9 +42,8 @@
  	__va_list ap;
  {
  	int ret;
 -	FILE f;
 +	FILE f = FAKE_FILE;
  
 -	f._file = -1;
  	f._flags = __SWR | __SSTR | __SALC;
  	f._bf._base = f._p = (unsigned char *)malloc(128);
  	if (f._bf._base == NULL) {
 @@ -53,8 +52,6 @@
  		return (-1);
  	}
  	f._bf._size = f._w = 127;		/* Leave room for the NUL */
 -	f._orientation = 0;
 -	memset(&f._mbstate, 0, sizeof(mbstate_t));
  	ret = __vfprintf(&f, fmt, ap);
  	if (ret < 0) {
  		free(f._bf._base);
 Index: stdio/vswscanf.c
 ===================================================================
 --- stdio/vswscanf.c	(revision 201516)
 +++ stdio/vswscanf.c	(working copy)
 @@ -62,7 +62,7 @@
  {
  	static const mbstate_t initial;
  	mbstate_t mbs;
 -	FILE f;
 +	FILE f = FAKE_FILE;
  	char *mbstr;
  	size_t mlen;
  	int r;
 @@ -80,15 +80,10 @@
  		free(mbstr);
  		return (EOF);
  	}
 -	f._file = -1;
  	f._flags = __SRD;
  	f._bf._base = f._p = (unsigned char *)mbstr;
  	f._bf._size = f._r = mlen;
  	f._read = eofread;
 -	f._ub._base = NULL;
 -	f._lb._base = NULL;
 -	f._orientation = 0;
 -	memset(&f._mbstate, 0, sizeof(mbstate_t));
  	r = __vfwscanf(&f, fmt, ap);
  	free(mbstr);
  
 
 -- 
 John Baldwin


More information about the freebsd-threads mailing list