Hey,
In stdio, which lock are you supposed to take first? The global
sfp_mutex or the per-FILE lock?
In __sfp() we hold sfp_mutex while iterating through the pool (unsure
what else to call it) of FILEs. No two threads can modify the pool at
the same time:
111 _MUTEX_LOCK(&__sfp_mutex);
112 for (g = &__sglue; g != NULL; g = g->next) {
113 for (fp = g->iobs, n = g->niobs; --n >= 0; fp++)
114 if (fp->_flags == 0)
115 goto found;
116 }
117
118 /* release lock while mallocing */
119 _MUTEX_UNLOCK(&__sfp_mutex);
120 if ((g = moreglue(NDYNAMIC)) == NULL)
121 return (NULL);
122 _MUTEX_LOCK(&__sfp_mutex);
123 lastglue->next = g;
124 lastglue = g;
125 fp = g->iobs;
126 found:
127 fp->_flags = 1; /* reserve this slot; caller sets real
flags */
128 _MUTEX_UNLOCK(&__sfp_mutex);
Note that we set _flags to 1 to reserve it for the current thread
before leaving sfp_mutex. Note also that we don't take the per-FILE
lock before reading each FILE's _flags.
Then look at fclose(3):
39 int
40 fclose(FILE *fp)
41 {
42 int r;
43
44 if (fp->_flags == 0) { /* not open! */
45 errno = EBADF;
46 return (EOF);
47 }
48 FLOCKFILE(fp);
49 WCIO_FREE(fp);
50 r = fp->_flags & __SWR ? __sflush(fp) : 0;
51 if (fp->_close != NULL && (*fp->_close)(fp->_cookie) < 0)
52 r = EOF;
53 if (fp->_flags & __SMBF)
54 free((char *)fp->_bf._base);
55 if (HASUB(fp))
56 FREEUB(fp);
57 if (HASLB(fp))
58 FREELB(fp);
59 fp->_r = fp->_w = 0; /* Mess up if reaccessed. */
61 fp->_flags = 0; /* Release this FILE for reuse. */
63 FUNLOCKFILE(fp);
64 return (r);
65 }
66 DEF_STRONG(fclose);
We check if _flags is zero without any lock. I'm unsure if this is
safe.
However, we then clean up under the FILE's lock and set _flags to zero
without sfp_mutex.
... that can't be right.
So, what to do? My immediate thought was to export sfp_mutex and
enter it before writing _flags (diff attached). But then the global
sfp_mutex is "higher" in the locking hierarchy than the per-FILE lock.
That doesn't seem quite right to me.
We also modify _flags all over stdio without sfp_mutex, so the rule is
inconsistent.
Another possibility is to take the per-FILE lock when examining each
FILE's _flags during __sfp(). That would be costlier, but then the
hierarchy would be reversed.
Thoughts?
Index: findfp.c
===================================================================
RCS file: /cvs/src/lib/libc/stdio/findfp.c,v
retrieving revision 1.19
diff -u -p -r1.19 findfp.c
--- findfp.c 5 Apr 2016 04:29:21 -0000 1.19
+++ findfp.c 26 Nov 2020 00:17:16 -0000
@@ -42,6 +42,7 @@
#include "thread_private.h"
int __sdidinit;
+void *__sfp_mutex;
#define NDYNAMIC 10 /* add ten more whenever necessary */
@@ -56,7 +57,6 @@ static FILE usual[FOPEN_MAX - 3];
static struct __sfileext usualext[FOPEN_MAX - 3];
static struct glue uglue = { 0, FOPEN_MAX - 3, usual };
static struct glue *lastglue = &uglue;
-static void *sfp_mutex;
static struct __sfileext __sFext[3];
FILE __sF[3] = {
@@ -108,7 +108,7 @@ __sfp(void)
if (!__sdidinit)
__sinit();
- _MUTEX_LOCK(&sfp_mutex);
+ _MUTEX_LOCK(&__sfp_mutex);
for (g = &__sglue; g != NULL; g = g->next) {
for (fp = g->iobs, n = g->niobs; --n >= 0; fp++)
if (fp->_flags == 0)
@@ -116,16 +116,16 @@ __sfp(void)
}
/* release lock while mallocing */
- _MUTEX_UNLOCK(&sfp_mutex);
+ _MUTEX_UNLOCK(&__sfp_mutex);
if ((g = moreglue(NDYNAMIC)) == NULL)
return (NULL);
- _MUTEX_LOCK(&sfp_mutex);
+ _MUTEX_LOCK(&__sfp_mutex);
lastglue->next = g;
lastglue = g;
fp = g->iobs;
found:
fp->_flags = 1; /* reserve this slot; caller sets real flags */
- _MUTEX_UNLOCK(&sfp_mutex);
+ _MUTEX_UNLOCK(&__sfp_mutex);
fp->_p = NULL; /* no current pointer */
fp->_w = 0; /* nothing to read or write */
fp->_r = 0;
Index: fclose.c
===================================================================
RCS file: /cvs/src/lib/libc/stdio/fclose.c,v
retrieving revision 1.10
diff -u -p -r1.10 fclose.c
--- fclose.c 31 Aug 2015 02:53:57 -0000 1.10
+++ fclose.c 26 Nov 2020 00:17:16 -0000
@@ -57,7 +57,9 @@ fclose(FILE *fp)
if (HASLB(fp))
FREELB(fp);
fp->_r = fp->_w = 0; /* Mess up if reaccessed. */
+ _MUTEX_LOCK(&__sfp_mutex);
fp->_flags = 0; /* Release this FILE for reuse. */
+ _MUTEX_UNLOCK(&__sfp_mutex);
FUNLOCKFILE(fp);
return (r);
}
Index: local.h
===================================================================
RCS file: /cvs/src/lib/libc/stdio/local.h,v
retrieving revision 1.25
diff -u -p -r1.25 local.h
--- local.h 23 May 2016 00:21:48 -0000 1.25
+++ local.h 26 Nov 2020 00:17:16 -0000
@@ -66,6 +66,7 @@ int __vfwprintf(FILE * __restrict, const
int __vfwscanf(FILE * __restrict, const wchar_t * __restrict, __va_list);
extern int __sdidinit;
+extern void *__sfp_mutex;
__END_HIDDEN_DECLS
/*
