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 <br...@clisp.org>
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>
