This is the mail archive of the newlib@sources.redhat.com mailing list for the newlib project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [RFA]: race safe fwalk


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;
 }
 

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]