Re: __fwalk mutex problem

[Jeff Johnston <jjohnstn at redhat dot com>]

Hans,

There is a known problem with the locking stuff in stdio.  I had already 
fixed fwalk.c to not
lock and unlock the files (i.e. it was up to the called function to do 
so if needed).  That is already checked
in the repository.

Now, there is also a deadlock caused by not enforcing the ordering of 
locks for reading/fclose.  What is happening is fread gets an fp-lock.  
fclose comes along in another thread and gets the sfp-lock, then tries 
for the fp-lock but has to wait for the read to finish.  The read can 
possibly try and get the sfp-lock and is blocked because fclose has it 
but can't finish.  Deadlock.  The read must try the sfp-lock before 
locking fp.

I had created a patch for another user to try out, but they didn't 
report any success.  If you would like to try it out, see the attached 
patch.  What it does is ensure that we always lock the sfp lock before 
any file locks.  If you find any problems, feel free to let me know.

BTW: the lflush function calls fflush which has file locks in it already.

-- Jeff J.

Hans-Erik Floryd wrote:
> Hello,
>
> The __fwalk and __fwalk_reent functions lock the file mutex before 
> calling the walk function. They unlock the mutex when the function 
> returns.
>
>   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);
>         }
>
> When you call __fwalk(ptr, fclose), as _cleanup_r does, there is a 
> problem because fclose will release the mutex. When the function 
> returns  __fwalk will call _funlockfile on a file that is closed.
>
> With our mutex implementation that gives an error, because a released 
> mutex is flagged as invalid.
>
> Is it necessary for __fwalk to lock the file before calling the walk 
> function? When __fwalk is called with fclose and fflush it seems 
> unnecessary, because both of those functions lock the mutex internally.
>
> In __srefill_r there is a call to _fwalk with lflush as the walk 
> function. It calls fflush after inspecting fp->_flags, is the lock 
> needed in this case? If so lflush could be modified to lock the file 
> internally.
>
> (There is also __fp_lock_all and __fp_unlock_all to consider. I am not 
> sure how these are used though, couldn't find a reference anywhere).
>
> Cheers,
>  Hans-Erik Floryd

Index: fgetc.c
===================================================================
RCS file: /cvs/src/src/newlib/libc/stdio/fgetc.c,v
retrieving revision 1.6
diff -u -p -r1.6 fgetc.c
--- fgetc.c	26 Sep 2006 21:22:19 -0000	1.6
+++ fgetc.c	27 Mar 2009 18:53:52 -0000
@@ -93,9 +93,11 @@ _DEFUN(fgetc, (fp),
 #if !defined(PREFER_SIZE_OVER_SPEED) && !defined(__OPTIMIZE_SIZE__)
   int result;
   CHECK_INIT(_REENT, fp);
+  __sfp_lock_acquire ();
   _flockfile (fp);
   result = __sgetc_r (_REENT, fp);
   _funlockfile (fp);
+  __sfp_lock_release ();
   return result;
 #else
   return _fgetc_r (_REENT, fp);
Index: fgets.c
===================================================================
RCS file: /cvs/src/src/newlib/libc/stdio/fgets.c,v
retrieving revision 1.7
diff -u -p -r1.7 fgets.c
--- fgets.c	26 Sep 2006 21:22:19 -0000	1.7
+++ fgets.c	27 Mar 2009 18:53:52 -0000
@@ -98,6 +98,7 @@ _DEFUN(_fgets_r, (ptr, buf, n, fp),
 
   CHECK_INIT(ptr, fp);
 
+  __sfp_lock_acquire ();
   _flockfile (fp);
 #ifdef __SCLE
   if (fp->_flags & __SCLE)
@@ -113,10 +114,12 @@ _DEFUN(_fgets_r, (ptr, buf, n, fp),
       if (c == EOF && s == buf)
         {
           _funlockfile (fp);
+	  __sfp_lock_release ();
           return NULL;
         }
       *s = 0;
       _funlockfile (fp);
+      __sfp_lock_release ();
       return buf;
     }
 #endif
@@ -135,6 +138,7 @@ _DEFUN(_fgets_r, (ptr, buf, n, fp),
 	      if (s == buf)
                 {
                   _funlockfile (fp);
+		  __sfp_lock_release ();
                   return 0;
                 }
 	      break;
@@ -160,6 +164,7 @@ _DEFUN(_fgets_r, (ptr, buf, n, fp),
 	  _CAST_VOID memcpy ((_PTR) s, (_PTR) p, len);
 	  s[len] = 0;
           _funlockfile (fp);
+	  __sfp_lock_release ();
 	  return (buf);
 	}
       fp->_r -= len;
@@ -170,6 +175,7 @@ _DEFUN(_fgets_r, (ptr, buf, n, fp),
   while ((n -= len) != 0);
   *s = 0;
   _funlockfile (fp);
+  __sfp_lock_release ();
   return buf;
 }
 
Index: fgetwc.c
===================================================================
RCS file: /cvs/src/src/newlib/libc/stdio/fgetwc.c,v
retrieving revision 1.2
diff -u -p -r1.2 fgetwc.c
--- fgetwc.c	11 Mar 2009 11:53:22 -0000	1.2
+++ fgetwc.c	27 Mar 2009 18:53:52 -0000
@@ -164,10 +164,12 @@ _DEFUN(_fgetwc_r, (ptr, fp),
 {
   wint_t r;
 
+  __sfp_lock_acquire ();
   _flockfile (fp);
   ORIENT(fp, 1);
   r = __fgetwc (ptr, fp);
   _funlockfile (fp);
+  __sfp_lock_release ();
   return r;
 }
 
Index: fgetws.c
===================================================================
RCS file: /cvs/src/src/newlib/libc/stdio/fgetws.c,v
retrieving revision 1.2
diff -u -p -r1.2 fgetws.c
--- fgetws.c	11 Mar 2009 11:53:22 -0000	1.2
+++ fgetws.c	27 Mar 2009 18:53:52 -0000
@@ -93,6 +93,7 @@ _DEFUN(_fgetws_r, (ptr, ws, n, fp),
   const char *src;
   unsigned char *nl;
 
+  __sfp_lock_acquire ();
   _flockfile (fp);
   ORIENT (fp, 1);
 
@@ -143,10 +144,12 @@ _DEFUN(_fgetws_r, (ptr, ws, n, fp),
     goto error;
   *wsp++ = L'\0';
   _funlockfile (fp);
+  __sfp_lock_release ();
   return ws;
 
 error:
   _funlockfile (fp);
+  __sfp_lock_release ();
   return NULL;
 }
 
Index: fread.c
===================================================================
RCS file: /cvs/src/src/newlib/libc/stdio/fread.c,v
retrieving revision 1.18
diff -u -p -r1.18 fread.c
--- fread.c	11 Mar 2009 11:53:22 -0000	1.18
+++ fread.c	27 Mar 2009 18:53:52 -0000
@@ -146,6 +146,7 @@ _DEFUN(_fread_r, (ptr, buf, size, count,
 
   CHECK_INIT(ptr, fp);
 
+  __sfp_lock_acquire ();
   _flockfile (fp);
   ORIENT (fp, -1);
   if (fp->_r < 0)
@@ -196,10 +197,12 @@ _DEFUN(_fread_r, (ptr, buf, size, count,
               if (fp->_flags & __SCLE)
 	        {
 	          _funlockfile (fp);
+		  __sfp_lock_release ();
 	          return crlf_r (ptr, fp, buf, total-resid, 1) / size;
 	        }
 #endif
 	      _funlockfile (fp);
+	      __sfp_lock_release ();
 	      return (total - resid) / size;
 	    }
 	}
@@ -221,10 +224,12 @@ _DEFUN(_fread_r, (ptr, buf, size, count,
 	      if (fp->_flags & __SCLE)
 		{
 		  _funlockfile (fp);
+		  __sfp_lock_release ();
 		  return crlf_r (ptr, fp, buf, total-resid, 1) / size;
 		}
 #endif
 	      _funlockfile (fp);
+	      __sfp_lock_release ();
 	      return (total - resid) / size;
 	    }
 	}
@@ -238,10 +243,12 @@ _DEFUN(_fread_r, (ptr, buf, size, count,
   if (fp->_flags & __SCLE)
     {
       _funlockfile (fp);
+      __sfp_lock_release ();
       return crlf_r(ptr, fp, buf, total, 0) / size;
     }
 #endif
   _funlockfile (fp);
+  __sfp_lock_release ();
   return count;
 }
 
Index: fseek.c
===================================================================
RCS file: /cvs/src/src/newlib/libc/stdio/fseek.c,v
retrieving revision 1.21
diff -u -p -r1.21 fseek.c
--- fseek.c	12 Dec 2008 15:45:19 -0000	1.21
+++ fseek.c	27 Mar 2009 18:53:52 -0000
@@ -138,6 +138,7 @@ _DEFUN(_fseek_r, (ptr, fp, offset, whenc
 
   CHECK_INIT (ptr, fp);
 
+  __sfp_lock_acquire ();
   _flockfile (fp);
 
   /* If we've been doing some writing, and we're in append mode
@@ -155,6 +156,7 @@ _DEFUN(_fseek_r, (ptr, fp, offset, whenc
     {
       ptr->_errno = ESPIPE;	/* ??? */
       _funlockfile (fp);
+      __sfp_lock_release ();
       return EOF;
     }
 
@@ -180,6 +182,7 @@ _DEFUN(_fseek_r, (ptr, fp, offset, whenc
 	  if (curoff == -1L)
 	    {
 	      _funlockfile (fp);
+	      __sfp_lock_release ();
 	      return EOF;
 	    }
 	}
@@ -205,6 +208,7 @@ _DEFUN(_fseek_r, (ptr, fp, offset, whenc
     default:
       ptr->_errno = EINVAL;
       _funlockfile (fp);
+      __sfp_lock_release ();
       return (EOF);
     }
 
@@ -263,6 +267,8 @@ _DEFUN(_fseek_r, (ptr, fp, offset, whenc
   if ((long)target != target)
     {
       ptr->_errno = EOVERFLOW;
+      _funlockfile (fp);
+      __sfp_lock_release ();
       return EOF;
     }
 
@@ -319,6 +325,7 @@ _DEFUN(_fseek_r, (ptr, fp, offset, whenc
       fp->_flags &= ~__SEOF;
       memset (&fp->_mbstate, 0, sizeof (_mbstate_t));
       _funlockfile (fp);
+      __sfp_lock_release ();
       return 0;
     }
 
@@ -349,6 +356,7 @@ _DEFUN(_fseek_r, (ptr, fp, offset, whenc
     }
   memset (&fp->_mbstate, 0, sizeof (_mbstate_t));
   _funlockfile (fp);
+  __sfp_lock_release ();
   return 0;
 
   /*
@@ -361,6 +369,7 @@ dumb:
       || seekfn (ptr, fp->_cookie, offset, whence) == POS_ERR)
     {
       _funlockfile (fp);
+      __sfp_lock_release ();
       return EOF;
     }
   /* success: clear EOF indicator and discard ungetc() data */
@@ -379,6 +388,7 @@ dumb:
   fp->_flags &= ~__SNPT;
   memset (&fp->_mbstate, 0, sizeof (_mbstate_t));
   _funlockfile (fp);
+  __sfp_lock_release ();
   return 0;
 }
 
Index: getc.c
===================================================================
RCS file: /cvs/src/src/newlib/libc/stdio/getc.c,v
retrieving revision 1.6
diff -u -p -r1.6 getc.c
--- getc.c	26 Sep 2006 21:22:19 -0000	1.6
+++ getc.c	27 Mar 2009 18:53:52 -0000
@@ -92,9 +92,11 @@ _DEFUN(_getc_r, (ptr, fp),
 {
   int result;
   CHECK_INIT (ptr, fp);
+  __sfp_lock_acquire ();
   _flockfile (fp);
   result = __sgetc_r (ptr, fp);
   _funlockfile (fp);
+  __sfp_lock_release ();
   return result;
 }
 
@@ -106,9 +108,11 @@ _DEFUN(getc, (fp),
 {
   int result;
   CHECK_INIT (_REENT, fp);
+  __sfp_lock_acquire ();
   _flockfile (fp);
   result = __sgetc_r (_REENT, fp);
   _funlockfile (fp);
+  __sfp_lock_release ();
   return result;
 }
 
Index: getdelim.c
===================================================================
RCS file: /cvs/src/src/newlib/libc/stdio/getdelim.c,v
retrieving revision 1.6
diff -u -p -r1.6 getdelim.c
--- getdelim.c	26 Sep 2006 21:22:19 -0000	1.6
+++ getdelim.c	27 Mar 2009 18:53:52 -0000
@@ -81,6 +81,7 @@ _DEFUN(__getdelim, (bufptr, n, delim, fp
 
   CHECK_INIT (_REENT, fp);
 
+  __sfp_lock_acquire ();
   _flockfile (fp);
 
   numbytes = *n;
@@ -130,6 +131,7 @@ _DEFUN(__getdelim, (bufptr, n, delim, fp
     }
 
   _funlockfile (fp);
+  __sfp_lock_release ();
 
   /* if no input data, return failure */
   if (ptr == buf)
Index: gets.c
===================================================================
RCS file: /cvs/src/src/newlib/libc/stdio/gets.c,v
retrieving revision 1.3
diff -u -p -r1.3 gets.c
--- gets.c	25 Nov 2008 09:33:43 -0000	1.3
+++ gets.c	27 Mar 2009 18:53:52 -0000
@@ -79,12 +79,14 @@ _DEFUN(_gets_r, (ptr, buf),
   register int c;
   register char *s = buf;
 
+  __sfp_lock_acquire ();
   _flockfile (stdin);
   while ((c = __sgetc_r (ptr, stdin)) != '\n')
     if (c == EOF)
       if (s == buf)
 	{
 	  _funlockfile (stdin);
+	  __sfp_lock_release ();
 	  return NULL;
 	}
       else
@@ -93,6 +95,7 @@ _DEFUN(_gets_r, (ptr, buf),
       *s++ = c;
   *s = 0;
   _funlockfile (stdin);
+  __sfp_lock_release ();
   return buf;
 }