This is the mail archive of the
newlib@sources.redhat.com
mailing list for the newlib project.
Re: [RFA]: race safe fwalk
- From: Thomas Pfaff <thomas dot pfaff at gmx dot net>
- To: Jeff Johnston <jjohnstn at redhat dot com>
- Cc: newlib at sources dot redhat dot com
- Date: Mon, 22 Mar 2004 22:47:13 +0100
- Subject: Re: [RFA]: race safe fwalk
- References: <404CD69B.1080803@gmx.net> <404E4309.1090109@redhat.com>
Jeff Johnston wrote:
Thomas Pfaff wrote:
This time with attachment.
There is a possible race between fwalk and fopen:
When a thread make a call to fopen the FILE * _flags will be set to 1
in findfp to mark it used and later it will be changed to the real
FILE flag.
When another thread calls fwalk during that time fwalk will treat the
FILE as already opened and calls the callback functions with the yet
unopened and only partially initialized FILE *.
This can be avoided by checking for fp->_flags != 0 && fp->_flags !=
1. Since _flags is signed short i did not check for _flags > 1. The
flag should be set as the last step in an open call.
I do not think that 1 is a valid _flag for an open file. Correct me if
am wrong.
Unfortunately, 1 is also line-buffered: __SLBF.
What if instead, we use the _file field to check for a valid file. It
gets set to -1 by __sfp. Now, if we set _file inside the __sfp_lock and
then had fwalk() use the __sfp lock as well and check for _file != -1,
plus have the open routines set the _file field last, this should work
equally as well. Comments?
I do not think that this can be done that easy anymore. The whole file
list stuff must be better protected.
Attached is a new patch. I am sorry for the delay.
2004-03-22 Thomas Pfaff <tpfaff@gmx.net>
* libc/stdio/fclose.c (fclose): Protect file pointer list when
releasing a file.
* libc/stdio/fcloseall.c (_fcloseall_r): Close all files via
fwalk.
* libc/stdio/fdopen.c (_fdopen_r): Add calls to
_flockfile/_funlockfile.
* libc/stdio/findfp.c: Move __sfp_lock. Change __sfp_lock type
to recursive.
Change __lock_acquire/__lock_release calls for __sfp_lock to
__sfp_lock_acquire/__sfp_lock_release throughout.
(std): Make sure that file lock is only initialized once.
(__sfp): Move _file initialization. Initialize file lock.
(__sfp_lock_acquire): New function.
(__sfp_lock_release): Ditto.
(__fp_lock_all): Remove __sfp_lock_acquire call.
(__fp_unlock_all): Remove __sfp_lock_release call.
* libc/stdio/fopen.c (_fopen_r): Protect file pointer list.
Add calls to _flockfile/_funlockfile. Remove
__lock_init_recursive call.
* libc/stdio/freopen.c (_freopen_r): Protect file pointer list.
* libc/stdio/fwalk.c (__fwalk): New static function.
(_fwalk): Protect file pointer list. Use __fwalk to walk through
file pointers.
* libc/stdio/local.h: Add defines for
__sfp_lock_acquire/__sfp_lock_release when
single threaded. Add function prototypes otherwise.
* libc/stdio64/fdopen64.c (_fdopen64_r): Add calls to
_flockfile/_funlockfile.
* libc/stdio/fopen64.c (_fopen64_r): Protect file pointer list.
Add calls to _flockfile/_funlockfile. Remove
__lock_init_recursive call.
* libc/stdio/freopen64.c (_freopen64_r): Protect file pointer
list.
Index: libc/stdio/fclose.c
===================================================================
RCS file: /cvs/src/src/newlib/libc/stdio/fclose.c,v
retrieving revision 1.5
diff -u -p -r1.5 fclose.c
--- libc/stdio/fclose.c 19 Jan 2004 21:30:34 -0000 1.5
+++ libc/stdio/fclose.c 22 Mar 2004 21:30:43 -0000
@@ -65,6 +65,8 @@ _DEFUN (fclose, (fp),
if (fp == NULL)
return (0); /* on NULL */
+ __sfp_lock_acquire ();
+
_flockfile(fp);
CHECK_INIT (fp);
@@ -72,6 +74,7 @@ _DEFUN (fclose, (fp),
if (fp->_flags == 0) /* not open! */
{
_funlockfile(fp);
+ __sfp_lock_release ();
return (0);
}
r = fp->_flags & __SWR ? fflush (fp) : 0;
@@ -83,11 +86,13 @@ _DEFUN (fclose, (fp),
FREEUB (fp);
if (HASLB (fp))
FREELB (fp);
+ fp->_flags = 0; /* release this FILE for reuse */
_funlockfile(fp);
#ifndef __SINGLE_THREAD__
__lock_close_recursive (*(_LOCK_RECURSIVE_T *)&fp->_lock);
#endif
- fp->_flags = 0; /* release this FILE for reuse */
+
+ __sfp_lock_release ();
return (r);
}
Index: libc/stdio/fcloseall.c
===================================================================
RCS file: /cvs/src/src/newlib/libc/stdio/fcloseall.c,v
retrieving revision 1.2
diff -u -p -r1.2 fcloseall.c
--- libc/stdio/fcloseall.c 22 Aug 2003 18:52:25 -0000 1.2
+++ libc/stdio/fcloseall.c 22 Mar 2004 21:30:43 -0000
@@ -66,15 +66,7 @@ int
_fcloseall_r (ptr)
struct _reent *ptr;
{
- register FILE *fp;
- register int n, ret = 0;
- register struct _glue *g;
-
- for (g = &ptr->__sglue; g != NULL; g = g->_next)
- for (fp = g->_iobs, n = g->_niobs; --n >= 0; fp++)
- if (fp->_flags != 0)
- ret |= fclose (fp);
- return ret;
+ return _fwalk (ptr, fclose);
}
#ifndef _REENT_ONLY
Index: libc/stdio/fdopen.c
===================================================================
RCS file: /cvs/src/src/newlib/libc/stdio/fdopen.c,v
retrieving revision 1.6
diff -u -p -r1.6 fdopen.c
--- libc/stdio/fdopen.c 19 Sep 2002 21:28:52 -0000 1.6
+++ libc/stdio/fdopen.c 22 Mar 2004 21:30:43 -0000
@@ -76,6 +76,9 @@ _DEFUN (_fdopen_r, (ptr, fd, mode),
if ((fp = __sfp (ptr)) == 0)
return 0;
+
+ _flockfile(fp);
+
fp->_flags = flags;
/*
* If opened for appending, but underlying descriptor
@@ -111,6 +114,7 @@ _DEFUN (_fdopen_r, (ptr, fd, mode),
fp->_flags |= __SCLE;
#endif
+ _funlockfile(fp);
return fp;
}
Index: libc/stdio/findfp.c
===================================================================
RCS file: /cvs/src/src/newlib/libc/stdio/findfp.c,v
retrieving revision 1.11
diff -u -p -r1.11 findfp.c
--- libc/stdio/findfp.c 9 Mar 2004 21:27:37 -0000 1.11
+++ libc/stdio/findfp.c 22 Mar 2004 21:30:44 -0000
@@ -25,10 +25,6 @@
#include <sys/lock.h>
#include "local.h"
-#ifndef __SINGLE_THREAD__
-__LOCK_INIT(static, __sfp_lock);
-#endif
-
static void
std (ptr, flags, file, data)
FILE *ptr;
@@ -49,8 +45,12 @@ std (ptr, flags, file, data)
ptr->_write = __swrite;
ptr->_seek = __sseek;
ptr->_close = __sclose;
-#ifndef __SINGLE_THREAD__
+#if !defined(__SINGLE_THREAD__) && !defined(_REENT_SMALL)
__lock_init_recursive (*(_LOCK_RECURSIVE_T *)&ptr->_lock);
+ /*
+ * #else
+ * lock is already initialized in __sfp
+ */
#endif
#ifdef __SCLE
@@ -90,9 +90,7 @@ __sfp (d)
int n;
struct _glue *g;
-#ifndef __SINGLE_THREAD__
- __lock_acquire(__sfp_lock);
-#endif
+ __sfp_lock_acquire ();
if (!_GLOBAL_REENT->__sdidinit)
__sinit (_GLOBAL_REENT);
@@ -105,24 +103,24 @@ __sfp (d)
(g->_next = __sfmoreglue (d, NDYNAMIC)) == NULL)
break;
}
-#ifndef __SINGLE_THREAD__
- __lock_release(__sfp_lock);
-#endif
+ __sfp_lock_release ();
d->_errno = ENOMEM;
return NULL;
found:
+ fp->_file = -1; /* no file */
fp->_flags = 1; /* reserve this slot; caller sets real flags */
#ifndef __SINGLE_THREAD__
- __lock_release(__sfp_lock);
+ __lock_init_recursive (*(_LOCK_RECURSIVE_T *)&fp->_lock);
#endif
+ __sfp_lock_release ();
+
fp->_p = NULL; /* no current pointer */
fp->_w = 0; /* nothing to read or write */
fp->_r = 0;
fp->_bf._base = NULL; /* no buffer */
fp->_bf._size = 0;
fp->_lbfsize = 0; /* not line buffered */
- fp->_file = -1; /* no file */
/* fp->_cookie = <any>; */ /* caller sets cookie, _read/_write etc */
fp->_ub._base = NULL; /* no ungetc buffer */
fp->_ub._size = 0;
@@ -197,6 +195,20 @@ __sinit (s)
#ifndef __SINGLE_THREAD__
+__LOCK_INIT_RECURSIVE(static, __sfp_lock);
+
+void
+__sfp_lock_acquire ()
+{
+ __lock_acquire(__sfp_lock);
+}
+
+void
+__sfp_lock_release ()
+{
+ __lock_release(__sfp_lock);
+}
+
/* Walkable file locking routine. */
static int
__fp_lock (ptr)
@@ -220,8 +232,6 @@ __fp_unlock (ptr)
void
__fp_lock_all ()
{
- __lock_acquire(__sfp_lock);
-
(void) _fwalk (_REENT, __fp_lock);
}
@@ -229,7 +239,5 @@ void
__fp_unlock_all ()
{
(void) _fwalk (_REENT, __fp_unlock);
-
- __lock_release(__sfp_lock);
}
#endif
Index: libc/stdio/fopen.c
===================================================================
RCS file: /cvs/src/src/newlib/libc/stdio/fopen.c,v
retrieving revision 1.5
diff -u -p -r1.5 fopen.c
--- libc/stdio/fopen.c 22 Aug 2003 18:52:25 -0000 1.5
+++ libc/stdio/fopen.c 22 Mar 2004 21:30:44 -0000
@@ -138,10 +138,17 @@ _DEFUN (_fopen_r, (ptr, file, mode),
if ((f = _open_r (ptr, file, oflags, 0666)) < 0)
{
+ __sfp_lock_acquire ();
fp->_flags = 0; /* release */
+#ifndef __SINGLE_THREAD__
+ __lock_close_recursive (*(_LOCK_RECURSIVE_T *)&fp->_lock);
+#endif
+ __sfp_lock_release ();
return NULL;
}
+ _flockfile(fp);
+
fp->_file = f;
fp->_flags = flags;
fp->_cookie = (_PTR) fp;
@@ -158,10 +165,7 @@ _DEFUN (_fopen_r, (ptr, file, mode),
fp->_flags |= __SCLE;
#endif
-#ifndef __SINGLE_THREAD__
- __lock_init_recursive (*(_LOCK_RECURSIVE_T *)&fp->_lock);
-#endif
-
+ _funlockfile(fp);
return fp;
}
Index: libc/stdio/freopen.c
===================================================================
RCS file: /cvs/src/src/newlib/libc/stdio/freopen.c,v
retrieving revision 1.7
diff -u -p -r1.7 freopen.c
--- libc/stdio/freopen.c 19 Jan 2004 21:30:34 -0000 1.7
+++ libc/stdio/freopen.c 22 Mar 2004 21:30:44 -0000
@@ -87,14 +87,17 @@ _DEFUN (_freopen_r, (ptr, file, mode, fp
register int f;
int flags, oflags, e;
+ __sfp_lock_acquire ();
+
_flockfile(fp);
CHECK_INIT (fp);
if ((flags = __sflags (ptr, mode, &oflags)) == 0)
{
- (void) fclose (fp);
_funlockfile(fp);
+ (void) fclose (fp);
+ __sfp_lock_release ();
return NULL;
}
@@ -148,12 +151,13 @@ _DEFUN (_freopen_r, (ptr, file, mode, fp
if (f < 0)
{ /* did not get it after all */
+ fp->_flags = 0; /* set it free */
ptr->_errno = e; /* restore in case _close clobbered */
_funlockfile(fp);
#ifndef __SINGLE_THREAD__
__lock_close_recursive (*(_LOCK_RECURSIVE_T *)&fp->_lock);
#endif
- fp->_flags = 0; /* set it free */
+ __sfp_lock_release ();
return NULL;
}
@@ -171,6 +175,7 @@ _DEFUN (_freopen_r, (ptr, file, mode, fp
#endif
_funlockfile(fp);
+ __sfp_lock_release ();
return fp;
}
Index: libc/stdio/fwalk.c
===================================================================
RCS file: /cvs/src/src/newlib/libc/stdio/fwalk.c,v
retrieving revision 1.3
diff -u -p -r1.3 fwalk.c
--- libc/stdio/fwalk.c 31 Jan 2004 00:39:07 -0000 1.3
+++ libc/stdio/fwalk.c 22 Mar 2004 21:30:44 -0000
@@ -26,8 +26,8 @@ static char sccsid[] = "%W% (Berkeley) %
#include <errno.h>
#include "local.h"
-int
-_fwalk (ptr, function)
+static int
+__fwalk (ptr, function)
struct _reent *ptr;
register int (*function) ();
{
@@ -35,20 +35,36 @@ _fwalk (ptr, function)
register int n, ret = 0;
register struct _glue *g;
+ for (g = &ptr->__sglue; g != NULL; g = g->_next)
+ for (fp = g->_iobs, n = g->_niobs; --n >= 0; fp++)
+ if (fp->_flags != 0)
+ {
+ _flockfile (fp);
+ if (fp->_flags != 0 && fp->_file != -1)
+ ret |= (*function) (fp);
+ _funlockfile (fp);
+ }
+
+ return ret;
+}
+
+int
+_fwalk (ptr, function)
+ struct _reent *ptr;
+ register int (*function) ();
+{
+ register int ret = 0;
+
+ __sfp_lock_acquire ();
+
/* Must traverse given list for std streams. */
if (ptr != _GLOBAL_REENT)
- {
- for (g = &ptr->__sglue; g != NULL; g = g->_next)
- for (fp = g->_iobs, n = g->_niobs; --n >= 0; fp++)
- if (fp->_flags != 0)
- ret |= (*function) (fp);
- }
+ ret |= __fwalk (ptr, function);
/* Must traverse global list for all other streams. */
- for (g = &_GLOBAL_REENT->__sglue; g != NULL; g = g->_next)
- for (fp = g->_iobs, n = g->_niobs; --n >= 0; fp++)
- if (fp->_flags != 0)
- ret |= (*function) (fp);
+ ret |= __fwalk (_GLOBAL_REENT, function);
+
+ __sfp_lock_release ();
return ret;
}
Index: libc/stdio/local.h
===================================================================
RCS file: /cvs/src/src/newlib/libc/stdio/local.h,v
retrieving revision 1.8
diff -u -p -r1.8 local.h
--- libc/stdio/local.h 22 Aug 2003 18:52:25 -0000 1.8
+++ libc/stdio/local.h 22 Mar 2004 21:30:45 -0000
@@ -87,3 +87,11 @@ char *_EXFUN(_llicvt,(char *, long long,
#define CVT_BUF_SIZE 128
#define NDYNAMIC 4 /* add four more whenever necessary */
+
+#ifdef __SINGLE_THREAD__
+#define __sfp_lock_acquire()
+#define __sfp_lock_release()
+#else
+void _EXFUN(__sfp_lock_acquire,(void));
+void _EXFUN(__sfp_lock_release,(void));
+#endif
Index: libc/stdio64/fdopen64.c
===================================================================
RCS file: /cvs/src/src/newlib/libc/stdio64/fdopen64.c,v
retrieving revision 1.2
diff -u -p -r1.2 fdopen64.c
--- libc/stdio64/fdopen64.c 25 Jul 2003 16:19:55 -0000 1.2
+++ libc/stdio64/fdopen64.c 22 Mar 2004 21:30:45 -0000
@@ -63,6 +63,9 @@ _DEFUN (_fdopen64_r, (ptr, fd, mode),
if ((fp = __sfp (ptr)) == 0)
return 0;
+
+ _flockfile(fp);
+
fp->_flags = flags;
/*
* If opened for appending, but underlying descriptor
@@ -99,12 +102,9 @@ _DEFUN (_fdopen64_r, (ptr, fd, mode),
fp->_flags |= __SCLE;
#endif
-#ifndef __SINGLE_THREAD__
- __lock_init_recursive (*(_LOCK_RECURSIVE_T *)&fp->_lock);
-#endif
-
fp->_flags |= __SL64;
+ _funlockfile(fp);
return fp;
}
Index: libc/stdio64/fopen64.c
===================================================================
RCS file: /cvs/src/src/newlib/libc/stdio64/fopen64.c,v
retrieving revision 1.4
diff -u -p -r1.4 fopen64.c
--- libc/stdio64/fopen64.c 22 Aug 2003 18:52:25 -0000 1.4
+++ libc/stdio64/fopen64.c 22 Mar 2004 21:30:45 -0000
@@ -91,10 +91,17 @@ _DEFUN (_fopen64_r, (ptr, file, mode),
if ((f = _open64_r (ptr, file, oflags, 0666)) < 0)
{
+ __sfp_lock_acquire ();
fp->_flags = 0; /* release */
+#ifndef __SINGLE_THREAD__
+ __lock_close_recursive (*(_LOCK_RECURSIVE_T *)&fp->_lock);
+#endif
+ __sfp_lock_release ();
return NULL;
}
+ _flockfile(fp);
+
fp->_file = f;
fp->_flags = flags;
fp->_cookie = (_PTR) fp;
@@ -114,10 +121,7 @@ _DEFUN (_fopen64_r, (ptr, file, mode),
fp->_flags |= __SL64;
-#ifndef __SINGLE_THREAD__
- __lock_init_recursive (*(_LOCK_RECURSIVE_T *)&fp->_lock);
-#endif
-
+ _funlockfile(fp);
return fp;
}
Index: libc/stdio64/freopen64.c
===================================================================
RCS file: /cvs/src/src/newlib/libc/stdio64/freopen64.c,v
retrieving revision 1.3
diff -u -p -r1.3 freopen64.c
--- libc/stdio64/freopen64.c 22 Aug 2003 18:52:25 -0000 1.3
+++ libc/stdio64/freopen64.c 22 Mar 2004 21:30:45 -0000
@@ -88,14 +88,17 @@ _DEFUN (_freopen64_r, (ptr, file, mode,
register int f;
int flags, oflags, e;
+ __sfp_lock_acquire ();
+
_flockfile(fp);
CHECK_INIT (fp);
if ((flags = __sflags (ptr, mode, &oflags)) == 0)
{
- (void) fclose (fp);
_funlockfile(fp);
+ (void) fclose (fp);
+ __sfp_lock_release ();
return NULL;
}
@@ -152,6 +155,10 @@ _DEFUN (_freopen64_r, (ptr, file, mode,
fp->_flags = 0; /* set it free */
ptr->_errno = e; /* restore in case _close clobbered */
_funlockfile(fp);
+#ifndef __SINGLE_THREAD__
+ __lock_close_recursive (*(_LOCK_RECURSIVE_T *)&fp->_lock);
+#endif
+ __sfp_lock_release ();
return NULL;
}
@@ -172,6 +179,7 @@ _DEFUN (_freopen64_r, (ptr, file, mode,
fp->_flags |= __SL64;
_funlockfile(fp);
+ __sfp_lock_release ();
return fp;
}