threads/141198: src/lib/libc/stdio does not properly initialize mutexes
Jeremy Huddleston
>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.
>How-To-Repeat:
>Fix:
>Release-Note:
>Audit-Trail:
>Unformatted:
John Baldwin
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);
--
John Baldwin
John Baldwin
John Baldwin
Does this patch address the concerns?
--
John Baldwin
John Baldwin
From: John Baldwin <j...@freebsd.org>
To: freebsd...@freebsd.org
Jeremy Huddleston
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);
John Baldwin
From: John Baldwin <j...@freebsd.org>
To: Jeremy Huddleston <jere...@apple.com>
Cc: freebsd...@freebsd.org,
freebsd-gn...@freebsd.org
Subject: Re: threads/141198: src/lib/libc/stdio does not properly initialize mutexes
Date: Thu, 7 Jan 2010 09:56:25 -0500
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
John Baldwin
Ah, ok. In our stdio at least these are all dummy files that are passed to
Jeremy Huddleston
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.
John Baldwin
From: John Baldwin <j...@freebsd.org>
To: Jeremy Huddleston <jere...@apple.com>
Cc: freebsd...@freebsd.org,
freebsd-gn...@freebsd.org
Subject: Re: threads/141198: src/lib/libc/stdio does not properly initialize mutexes
Date: Thu, 7 Jan 2010 13:15:25 -0500
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
John Baldwin
>
Ok. I actually think it would be nicer to provide some sort of macro to
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);