Re: [PATCH] fclose: avoid double close race when possible

[Bruno Haible]

Hi Eric,

> Is it worth thinking about replacing _every_ standard function on
> mingw that can possibly create an fd in another thread (obviously,
> as an optional module, only needed in multithreaded gnulib clients),
> by grabbing a mutex to thus guarantee atomicity around otherwise
> racy operations like the one in our fclose() wrapper?

I would leave this work to those who need multithread-safe file operations
on mingw. IMO the most important use-case for this is in the web server
area. Do we have a GNU web server that is meant to run on mingw? I don't
think so. And Apache2 uses APR, not gnulib.

> > Calling close(fileno(fp)) prior to fclose(fp) is racy in a 
> multi-threaded application - some other thread could open a new file,
> which is then inadvertently closed by the fclose that we thought
> should fail with EBADF.  For mingw, this is no worse than the race
> already present in close_fd_maybe_socket for calling closesocket()
> prior to _close(), but for all other platforms, we might as well be
> nice and avoid the race.

Yes, good point.

But the patch has two mistakes:
  - It invokes the function unregister_shadow_fd, from your patch
    <http://lists.gnu.org/archive/html/bug-gnulib/2011-04/msg00374.html>
    that didn't make it to 'master'.
  - The comment is wrong: There is still a race if REPLACE_FCHDIR
    is true (which is the case on mingw, Tandem/NSK, BeOS).

Also, about the comments: I find that the race condition in fclose.c is more
severe than the one in sockets.c, because it's more likely for an 'fd' to be
reused than for a SOCKET value to be reused as a HANDLE value.


2011-05-11  Bruno Haible  <address@hidden>

        fclose: Fix possible link error.
        * lib/fclose.c (rpl_fclose): Invoke _gl_unregister_fd, not
        unregister_shadow_fd. Improve comments.
        * lib/sockets.c (close_fd_maybe_socket): Add comments. Reported by
        Eric Blake.

--- lib/fclose.c.orig   Wed May 11 13:11:21 2011
+++ lib/fclose.c        Wed May 11 13:04:26 2011
@@ -46,10 +46,12 @@
       && fflush (fp))
     saved_errno = errno;
 
+  /* fclose() calls close(), but we need to also invoke all hooks that our
+     overridden close() function invokes.  See lib/close.c.  */
 #if WINDOWS_SOCKETS
-  /* There is a minor race where some other thread could open fd
-     between our close and fopen, but it is no worse than the race in
-     close_fd_maybe_socket.  */
+  /* Call the overridden close(), then the original fclose().
+     Note about multithread-safety: There is a race condition where some
+     other thread could open fd between our close and fclose.  */
   if (close (fd) < 0 && saved_errno == 0)
     saved_errno = errno;
 
@@ -62,13 +64,20 @@
     }
 
 #else /* !WINDOWS_SOCKETS */
-  /* No race here.  */
-  result = fclose (fp);
+  /* Call fclose() and invoke all hooks of the overridden close().  */
 
 # if REPLACE_FCHDIR
-  if (result == 0)
-    unregister_shadow_fd (fd);
+  /* Note about multithread-safety: There is a race condition here as well.
+     Some other thread could open fd between our calls to fclose and
+     _gl_unregister_fd.  */
+  result = fclose (fp);
+  if (result >= 0)
+    _gl_unregister_fd (fd);
+# else
+  /* No race condition here.  */
+  result = fclose (fp);
 # endif
+
 #endif /* !WINDOWS_SOCKETS */
 
   return result;
--- lib/sockets.c.orig  Wed May 11 13:11:21 2011
+++ lib/sockets.c       Wed May 11 13:08:52 2011
@@ -37,6 +37,10 @@
                        gl_close_fn primary,
                        int fd)
 {
+  /* Note about multithread-safety: There is a race condition where, between
+     our calls to closesocket() and the primary close(), some other thread
+     could make system calls that allocate precisely the same HANDLE value
+     as sock; then the primary close() would call CloseHandle() on it.  */
   SOCKET sock;
   WSANETWORKEVENTS ev;
 

-- 
In memoriam Paul Bloomquist <http://en.wikipedia.org/wiki/Paul_Bloomquist>