[Emacs-diffs] trunk r113384: Fix races with threads and file descriptors.

[Paul Eggert]

------------------------------------------------------------
revno: 113384
revision-id: address@hidden
parent: address@hidden
committer: Paul Eggert <address@hidden>
branch nick: trunk
timestamp: Thu 2013-07-11 19:03:47 -0700
message:
  Fix races with threads and file descriptors.
  
  * configure.ac (PTY_TTY_NAME_SPRINTF): Use emacs_close, not close.
  * src/callproc.c (Fcall_process_region):
  * src/dired.c (open_directory):
  * src/emacs.c (main, Fdaemon_initialized):
  * src/image.c (x_find_image_file):
  * src/inotify.c (Finotify_rm_watch):
  * src/lread.c (Flocate_file_internal):
  * src/process.c (Fnetwork_interface_list, Fnetwork_interface_info):
  * src/term.c (term_mouse_moveto, init_tty):
  * src/termcap.c (tgetent):
  * src/unexaix.c, src/unexcoff.c (report_error, report_error_1, 
adjust_lnnoptrs)
  * src/unexaix.c, src/unexcoff.c, src/unexcw.c, src/unexelf.c (unexec):
  * src/unexhp9k800.c, src/unexmacosx.c (unexec):
  * src/callproc.c (Fcall_process_region):
  Use emacs_close, not close.
  * src/sysdep.c (POSIX_CLOSE_RESTART, posix_close) [!POSIX_CLOSE_RESTART]:
  New macro and function, which emulates the POSIX_CLOSE_RESTART macro
  and posix_close function on current platforms (which all lack them).
  (emacs_close): Use it.  This should fix the races on GNU/Linux and
  on AIX and on future platforms that support POSIX_CLOSE_RESTART,
  and it should avoid closing random victim file descriptors on
  other platforms.
modified:
  ChangeLog                      changelog-20091113204419-o5vbwnq5f7feedwu-1538
  configure.ac                   
configure.in-20091113204419-o5vbwnq5f7feedwu-783
  src/ChangeLog                  changelog-20091113204419-o5vbwnq5f7feedwu-1438
  src/callproc.c                 callproc.c-20091113204419-o5vbwnq5f7feedwu-248
  src/dired.c                    dired.c-20091113204419-o5vbwnq5f7feedwu-171
  src/emacs.c                    emacs.c-20091113204419-o5vbwnq5f7feedwu-241
  src/image.c                    image.c-20091113204419-o5vbwnq5f7feedwu-2969
  src/inotify.c                  inotify.c-20121210111500-2cd7np2oq1r0lmp9-1
  src/lread.c                    lread.c-20091113204419-o5vbwnq5f7feedwu-266
  src/process.c                  process.c-20091113204419-o5vbwnq5f7feedwu-462
  src/sysdep.c                   sysdep.c-20091113204419-o5vbwnq5f7feedwu-448
  src/term.c                     term.c-20091113204419-o5vbwnq5f7feedwu-220
  src/termcap.c                  termcap.c-20091113204419-o5vbwnq5f7feedwu-775
  src/unexaix.c                  unexaix.c-20091113204419-o5vbwnq5f7feedwu-147
  src/unexcoff.c                 unexec.c-20091113204419-o5vbwnq5f7feedwu-184
  src/unexcw.c                   unexcw.c-20091113204419-o5vbwnq5f7feedwu-3010
  src/unexelf.c                  unexelf.c-20091113204419-o5vbwnq5f7feedwu-426
  src/unexhp9k800.c              
unexhp9k800.c-20091113204419-o5vbwnq5f7feedwu-4481
  src/unexmacosx.c               
unexmacosx.c-20091113204419-o5vbwnq5f7feedwu-2413

=== modified file 'ChangeLog'
--- a/ChangeLog 2013-07-11 17:18:48 +0000
+++ b/ChangeLog 2013-07-12 02:03:47 +0000
@@ -1,3 +1,8 @@
+2013-07-12  Paul Eggert  <address@hidden>
+
+       Fix races with threads and file descriptors.
+       * configure.ac (PTY_TTY_NAME_SPRINTF): Use emacs_close, not close.
+
 2013-07-10  Paul Eggert  <address@hidden>
 
        * Makefile.in (removenullpaths): Remove adjacent null paths (Bug#14835).

=== modified file 'configure.ac'
--- a/configure.ac      2013-07-09 18:06:25 +0000
+++ b/configure.ac      2013-07-12 02:03:47 +0000
@@ -3931,7 +3931,7 @@
       AC_DEFINE(PTY_ITERATION, [int i; for (i = 0; i < 1; i++)])
       dnl Note that grantpt and unlockpt may fork.  We must block SIGCHLD
       dnl to prevent sigchld_handler from intercepting the child's death.
-      AC_DEFINE(PTY_TTY_NAME_SPRINTF, [{ char *ptyname = 0; sigset_t blocked; 
sigemptyset (&blocked); sigaddset (&blocked, SIGCHLD); pthread_sigmask 
(SIG_BLOCK, &blocked, 0); if (grantpt (fd) != -1 && unlockpt (fd) != -1) 
ptyname = ptsname(fd); pthread_sigmask (SIG_UNBLOCK, &blocked, 0); if 
(!ptyname) { close (fd); return -1; } snprintf (pty_name, sizeof pty_name, 
"%s", ptyname); }])
+      AC_DEFINE(PTY_TTY_NAME_SPRINTF, [{ char *ptyname = 0; sigset_t blocked; 
sigemptyset (&blocked); sigaddset (&blocked, SIGCHLD); pthread_sigmask 
(SIG_BLOCK, &blocked, 0); if (grantpt (fd) != -1 && unlockpt (fd) != -1) 
ptyname = ptsname(fd); pthread_sigmask (SIG_UNBLOCK, &blocked, 0); if 
(!ptyname) { emacs_close (fd); return -1; } snprintf (pty_name, sizeof 
pty_name, "%s", ptyname); }])
       dnl if HAVE_POSIX_OPENPT
       if test "x$ac_cv_func_posix_openpt" = xyes; then
         AC_DEFINE(PTY_OPEN, [fd = posix_openpt (O_RDWR | O_CLOEXEC | 
O_NOCTTY)])

=== modified file 'src/ChangeLog'
--- a/src/ChangeLog     2013-07-11 17:28:58 +0000
+++ b/src/ChangeLog     2013-07-12 02:03:47 +0000
@@ -1,3 +1,28 @@
+2013-07-12  Paul Eggert  <address@hidden>
+
+       Fix races with threads and file descriptors.
+       * callproc.c (Fcall_process_region):
+       * dired.c (open_directory):
+       * emacs.c (main, Fdaemon_initialized):
+       * image.c (x_find_image_file):
+       * inotify.c (Finotify_rm_watch):
+       * lread.c (Flocate_file_internal):
+       * process.c (Fnetwork_interface_list, Fnetwork_interface_info):
+       * term.c (term_mouse_moveto, init_tty):
+       * termcap.c (tgetent):
+       * unexaix.c, unexcoff.c (report_error, report_error_1, adjust_lnnoptrs)
+       * unexaix.c, unexcoff.c, unexcw.c, unexelf.c (unexec):
+       * unexhp9k800.c, unexmacosx.c (unexec):
+       * callproc.c (Fcall_process_region):
+       Use emacs_close, not close.
+       * sysdep.c (POSIX_CLOSE_RESTART, posix_close) [!POSIX_CLOSE_RESTART]:
+       New macro and function, which emulates the POSIX_CLOSE_RESTART macro
+       and posix_close function on current platforms (which all lack them).
+       (emacs_close): Use it.  This should fix the races on GNU/Linux and
+       on AIX and on future platforms that support POSIX_CLOSE_RESTART,
+       and it should avoid closing random victim file descriptors on
+       other platforms.
+
 2013-07-11  Paul Eggert  <address@hidden>
 
        * inotify.c (uninitialized): Remove.  All uses replaced by -1.

=== modified file 'src/callproc.c'
--- a/src/callproc.c    2013-07-09 07:04:48 +0000
+++ b/src/callproc.c    2013-07-12 02:03:47 +0000
@@ -1052,7 +1052,7 @@
        report_file_error ("Failed to open temporary file",
                           Fcons (build_string (tempfile), Qnil));
       else
-       close (fd);
+       emacs_close (fd);
     }
 #else
     errno = 0;

=== modified file 'src/dired.c'
--- a/src/dired.c       2013-04-02 01:54:56 +0000
+++ b/src/dired.c       2013-07-12 02:03:47 +0000
@@ -95,7 +95,7 @@
       d = fdopendir (fd);
       opendir_errno = errno;
       if (! d)
-       close (fd);
+       emacs_close (fd);
     }
 #endif
 

=== modified file 'src/emacs.c'
--- a/src/emacs.c       2013-07-10 23:23:57 +0000
+++ b/src/emacs.c       2013-07-12 02:03:47 +0000
@@ -1010,7 +1010,7 @@
          char buf[1];
 
          /* Close unused writing end of the pipe.  */
-         close (daemon_pipe[1]);
+         emacs_close (daemon_pipe[1]);
 
          /* Just wait for the child to close its end of the pipe.  */
          do
@@ -1030,7 +1030,7 @@
              exit (1);
            }
 
-         close (daemon_pipe[0]);
+         emacs_close (daemon_pipe[0]);
          exit (0);
        }
       if (f < 0)
@@ -1080,7 +1080,7 @@
       if (dname_arg)
                daemon_name = xstrdup (dname_arg);
       /* Close unused reading end of the pipe.  */
-      close (daemon_pipe[0]);
+      emacs_close (daemon_pipe[0]);
 
       setsid ();
 #else /* DOS_NT */
@@ -2254,7 +2254,7 @@
   err |= dup2 (nfd, 0) < 0;
   err |= dup2 (nfd, 1) < 0;
   err |= dup2 (nfd, 2) < 0;
-  err |= close (nfd) != 0;
+  err |= emacs_close (nfd) != 0;
 
   /* Closing the pipe will notify the parent that it can exit.
      FIXME: In case some other process inherited the pipe, closing it here
@@ -2264,7 +2264,7 @@
      call-process to make sure the pipe is never inherited by
      subprocesses.  */
   err |= write (daemon_pipe[1], "\n", 1) < 0;
-  err |= close (daemon_pipe[1]) != 0;
+  err |= emacs_close (daemon_pipe[1]) != 0;
   /* Set it to an invalid value so we know we've already run this function.  */
   daemon_pipe[1] = -1;
 

=== modified file 'src/image.c'
--- a/src/image.c       2013-07-09 05:04:45 +0000
+++ b/src/image.c       2013-07-12 02:03:47 +0000
@@ -2260,7 +2260,7 @@
   else
     {
       file_found = ENCODE_FILE (file_found);
-      close (fd);
+      emacs_close (fd);
     }
 
   return file_found;

=== modified file 'src/inotify.c'
--- a/src/inotify.c     2013-07-11 17:28:58 +0000
+++ b/src/inotify.c     2013-07-12 02:03:47 +0000
@@ -387,7 +387,7 @@
   /* Cleanup if no more files are watched. */
   if (NILP (watch_list))
     {
-      close (inotifyfd);
+      emacs_close (inotifyfd);
       delete_read_fd (inotifyfd);
       inotifyfd = -1;
     }

=== modified file 'src/lread.c'
--- a/src/lread.c       2013-07-10 23:23:57 +0000
+++ b/src/lread.c       2013-07-12 02:03:47 +0000
@@ -1413,7 +1413,7 @@
   Lisp_Object file;
   int fd = openp (path, filename, suffixes, &file, predicate);
   if (NILP (predicate) && fd > 0)
-    close (fd);
+    emacs_close (fd);
   return file;
 }
 

=== modified file 'src/process.c'
--- a/src/process.c     2013-07-09 07:04:48 +0000
+++ b/src/process.c     2013-07-12 02:03:47 +0000
@@ -3555,14 +3555,14 @@
       ifconf.ifc_len = buf_size;
       if (ioctl (s, SIOCGIFCONF, &ifconf))
        {
-         close (s);
+         emacs_close (s);
          xfree (buf);
          return Qnil;
        }
     }
   while (ifconf.ifc_len == buf_size);
 
-  close (s);
+  emacs_close (s);
 
   res = Qnil;
   ifreq = ifconf.ifc_req;
@@ -3819,7 +3819,7 @@
 #endif
   res = Fcons (elt, res);
 
-  close (s);
+  emacs_close (s);
 
   return any ? res : Qnil;
 }

=== modified file 'src/sysdep.c'
--- a/src/sysdep.c      2013-07-11 02:17:47 +0000
+++ b/src/sysdep.c      2013-07-12 02:03:47 +0000
@@ -2201,23 +2201,59 @@
   return fd < 0 ? 0 : fdopen (fd, mode);
 }
 
+/* Approximate posix_close and POSIX_CLOSE_RESTART well enough for Emacs.
+   For the background behind this mess, please see Austin Group defect 529
+   <http://austingroupbugs.net/view.php?id=529>.  */
+
+#ifndef POSIX_CLOSE_RESTART
+# define POSIX_CLOSE_RESTART 1
+static int
+posix_close (int fd, int flag)
+{
+  /* Only the POSIX_CLOSE_RESTART case is emulated.  */
+  eassert (flag == POSIX_CLOSE_RESTART);
+
+  /* Things are tricky if close (fd) returns -1 with errno == EINTR
+     on a system that does not define POSIX_CLOSE_RESTART.
+
+     In this case, in some systems (e.g., GNU/Linux, AIX) FD is
+     closed, and retrying the close could inadvertently close a file
+     descriptor allocated by some other thread.  In other systems
+     (e.g., HP/UX) FD is not closed.  And in still other systems
+     (e.g., OS X, Solaris), maybe FD is closed, maybe not, and in a
+     multithreaded program there can be no way to tell.
+
+     So, in this case, pretend that the close succeeded.  This works
+     well on systems like GNU/Linux that close FD.  Although it may
+     leak a file descriptor on other systems, the leak is unlikely and
+     it's better to leak than to close a random victim.  */
+  return close (fd) == 0 || errno == EINTR ? 0 : -1;
+}
+#endif
+
+/* Close FD, retrying if interrupted.  If successful, return 0;
+   otherwise, return -1 and set errno to a non-EINTR value.  Consider
+   an EINPROGRESS error to be successful, as that's merely a signal
+   arriving.  FD is always closed when this function returns, even
+   when it returns -1.
+
+   Do not call this function if FD might already be closed, as that
+   might close an innocent victim opened by some other thread.  */
+
 int
 emacs_close (int fd)
 {
-  bool did_retry = 0;
-  int rtnval;
-
-  while ((rtnval = close (fd)) == -1
-        && (errno == EINTR))
-    did_retry = 1;
-
-  /* If close is interrupted SunOS 4.1 may or may not have closed the
-     file descriptor.  If it did the second close will fail with
-     errno = EBADF.  That means we have succeeded.  */
-  if (rtnval == -1 && did_retry && errno == EBADF)
-    return 0;
-
-  return rtnval;
+  while (1)
+    {
+      int r = posix_close (fd, POSIX_CLOSE_RESTART);
+      if (r == 0)
+       return r;
+      if (!POSIX_CLOSE_RESTART || errno != EINTR)
+       {
+         eassert (errno != EBADF);
+         return errno == EINPROGRESS ? 0 : r;
+       }
+    }
 }
 
 /* Maximum number of bytes to read or write in a single system call.

=== modified file 'src/term.c'
--- a/src/term.c        2013-07-11 02:17:47 +0000
+++ b/src/term.c        2013-07-12 02:03:47 +0000
@@ -2478,7 +2478,7 @@
   name = (const char *) ttyname (0);
   fd = emacs_open (name, O_WRONLY, 0);
      SOME_FUNCTION (x, y, fd);
-  close (fd);
+  emacs_close (fd);
   last_mouse_x = x;
   last_mouse_y = y;  */
 }
@@ -3012,7 +3012,7 @@
                    name);
     if (!isatty (fd))
       {
-        close (fd);
+        emacs_close (fd);
         maybe_fatal (must_succeed, terminal,
                      "Not a tty device: %s",
                      "Not a tty device: %s",

=== modified file 'src/termcap.c'
--- a/src/termcap.c     2013-07-11 02:17:47 +0000
+++ b/src/termcap.c     2013-07-12 02:03:47 +0000
@@ -455,7 +455,7 @@
       /* Scan the file, reading it via buf, till find start of main entry.  */
       if (scan_file (term, fd, &buf) == 0)
        {
-         close (fd);
+         emacs_close (fd);
          xfree (buf.beg);
          if (malloc_size)
            xfree (bp);
@@ -493,7 +493,7 @@
       term = tgetst1 (tc_search_point, (char **) 0);
     }
 
-  close (fd);
+  emacs_close (fd);
   xfree (buf.beg);
 
   if (malloc_size)

=== modified file 'src/unexaix.c'
--- a/src/unexaix.c     2013-07-06 02:40:50 +0000
+++ b/src/unexaix.c     2013-07-12 02:03:47 +0000
@@ -97,7 +97,7 @@
   if (fd)
     {
       int failed_errno = errno;
-      close (fd);
+      emacs_close (fd);
       errno = failed_errno;
     }
   report_file_error ("Cannot unexec", Fcons (build_string (file), Qnil));
@@ -111,7 +111,7 @@
 report_error_1 (int fd, const char *msg, ...)
 {
   va_list ap;
-  close (fd);
+  emacs_close (fd);
   va_start (ap, msg);
   verror (msg, ap);
   va_end (ap);
@@ -148,13 +148,13 @@
       || adjust_lnnoptrs (new, a_out, new_name) < 0
       || unrelocate_symbols (new, a_out, a_name, new_name) < 0)
     {
-      close (new);
+      emacs_close (new);
       return;
     }
 
-  close (new);
+  emacs_close (new);
   if (a_out >= 0)
-    close (a_out);
+    emacs_close (a_out);
   mark_x (new_name);
 }
 
@@ -534,7 +534,7 @@
             }
        }
     }
-  close (new);
+  emacs_close (new);
 
   return 0;
 }

=== modified file 'src/unexcoff.c'
--- a/src/unexcoff.c    2013-07-06 02:40:50 +0000
+++ b/src/unexcoff.c    2013-07-12 02:03:47 +0000
@@ -128,7 +128,7 @@
 report_error (const char *file, int fd)
 {
   if (fd)
-    close (fd);
+    emacs_close (fd);
   report_file_error ("Cannot unexec", Fcons (build_string (file), Qnil));
 }
 
@@ -139,7 +139,7 @@
 static void
 report_error_1 (int fd, const char *msg, int a1, int a2)
 {
-  close (fd);
+  emacs_close (fd);
   error (msg, a1, a2);
 }
 
@@ -511,7 +511,7 @@
        }
     }
 #ifndef MSDOS
-  close (new);
+  emacs_close (new);
 #endif
   return 0;
 }
@@ -541,13 +541,13 @@
       || adjust_lnnoptrs (new, a_out, new_name) < 0
       )
     {
-      close (new);
+      emacs_close (new);
       return;
     }
 
-  close (new);
+  emacs_close (new);
   if (a_out >= 0)
-    close (a_out);
+    emacs_close (a_out);
   mark_x (new_name);
 }
 

=== modified file 'src/unexcw.c'
--- a/src/unexcw.c      2013-07-06 02:40:50 +0000
+++ b/src/unexcw.c      2013-07-12 02:03:47 +0000
@@ -316,13 +316,13 @@
       ret2 = write (fd_out, buffer, ret);
       assert (ret2 == ret);
     }
-  ret = close (fd_in);
+  ret = emacs_close (fd_in);
   assert (ret == 0);
 
   bss_sbrk_did_unexec = 1;
   fixup_executable (fd_out);
   bss_sbrk_did_unexec = 0;
 
-  ret = close (fd_out);
+  ret = emacs_close (fd_out);
   assert (ret == 0);
 }

=== modified file 'src/unexelf.c'
--- a/src/unexelf.c     2013-07-06 02:40:50 +0000
+++ b/src/unexelf.c     2013-07-12 02:03:47 +0000
@@ -1312,13 +1312,13 @@
   /* Close the files and make the new file executable.  */
 
 #if MAP_ANON == 0
-  close (mmap_fd);
+  emacs_close (mmap_fd);
 #endif
 
-  if (close (old_file) != 0)
+  if (emacs_close (old_file) != 0)
     fatal ("Can't close (%s): %s", old_name, strerror (errno));
 
-  if (close (new_file) != 0)
+  if (emacs_close (new_file) != 0)
     fatal ("Can't close (%s): %s", new_name, strerror (errno));
 
   if (stat (new_name, &stat_buf) != 0)

=== modified file 'src/unexhp9k800.c'
--- a/src/unexhp9k800.c 2013-07-06 02:40:50 +0000
+++ b/src/unexhp9k800.c 2013-07-12 02:03:47 +0000
@@ -306,6 +306,6 @@
   write_header (new, &hdr, &auxhdr);
 
   /* Close the binary file */
-  close (old);
-  close (new);
+  emacs_close (old);
+  emacs_close (new);
 }

=== modified file 'src/unexmacosx.c'
--- a/src/unexmacosx.c  2013-07-06 02:40:50 +0000
+++ b/src/unexmacosx.c  2013-07-12 02:03:47 +0000
@@ -1332,7 +1332,7 @@
   outfd = emacs_open (outfile, O_WRONLY | O_TRUNC | O_CREAT, 0755);
   if (outfd < 0)
     {
-      close (infd);
+      emacs_close (infd);
       unexec_error ("cannot open output file `%s'", outfile);
     }
 
@@ -1346,7 +1346,7 @@
 
   dump_it ();
 
-  close (outfd);
+  emacs_close (outfd);
 }