Date: Thu, 7 Jan 2010 18:20:03 GMT From: John Baldwin <jhb@freebsd.org> To: freebsd-threads@FreeBSD.org Subject: Re: threads/141198: src/lib/libc/stdio does not properly initialize mutexes Message-ID: <201001071820.o07IK3ng039585@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
The following reply was made to PR threads/141198; it has been noted by GNATS.
From: John Baldwin <jhb@freebsd.org>
To: Jeremy Huddleston <jeremyhu@apple.com>
Cc: freebsd-threads@freebsd.org,
freebsd-gnats-submit@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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201001071820.o07IK3ng039585>