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.

19 files changed, 118 insertions, 52 deletions

diff --git a/ChangeLog b/ChangeLog
index a96dbf214a..e802ee2d15 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2013-07-12  Paul Eggert  <eggert@cs.ucla.edu>
+
+	Fix races with threads and file descriptors.
+	* configure.ac (PTY_TTY_NAME_SPRINTF): Use emacs_close, not close.
+
 2013-07-10  Paul Eggert  <eggert@cs.ucla.edu>
 
 	* Makefile.in (removenullpaths): Remove adjacent null paths (Bug#14835).
diff --git a/configure.ac b/configure.ac
index bb140a86b1..6378ef1928 100644
--- a/configure.ac
+++ b/configure.ac
@@ -3931,7 +3931,7 @@ case $opsys in
       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)])
diff --git a/src/ChangeLog b/src/ChangeLog
index 04d5a02467..20c8be63cd 100644
--- a/src/ChangeLog
+++ b/src/ChangeLog
@@ -1,3 +1,28 @@
+2013-07-12  Paul Eggert  <eggert@cs.ucla.edu>
+
+	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  <eggert@cs.ucla.edu>
 
 	* inotify.c (uninitialized): Remove.  All uses replaced by -1.
diff --git a/src/callproc.c b/src/callproc.c
index fc274f3d9c..ac9477bff1 100644
--- a/src/callproc.c
+++ b/src/callproc.c
@@ -1052,7 +1052,7 @@ usage: (call-process-region START END PROGRAM &optional DELETE BUFFER DISPLAY &r
 	report_file_error ("Failed to open temporary file",
 			   Fcons (build_string (tempfile), Qnil));
       else
-	close (fd);
+	emacs_close (fd);
     }
 #else
     errno = 0;
diff --git a/src/dired.c b/src/dired.c
index 7bbfee7e5b..b3348b0aff 100644
--- a/src/dired.c
+++ b/src/dired.c
@@ -95,7 +95,7 @@ open_directory (char const *name, int *fdp)
       d = fdopendir (fd);
       opendir_errno = errno;
       if (! d)
-	close (fd);
+	emacs_close (fd);
     }
 #endif
 
diff --git a/src/emacs.c b/src/emacs.c
index 5babf3af02..2d55cfad3d 100644
--- a/src/emacs.c
+++ b/src/emacs.c
@@ -1010,7 +1010,7 @@ Using an Emacs configured with --with-x-toolkit=lucid does not have this problem
 	  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 @@ Using an Emacs configured with --with-x-toolkit=lucid does not have this problem
 	      exit (1);
 	    }
 
-	  close (daemon_pipe[0]);
+	  emacs_close (daemon_pipe[0]);
 	  exit (0);
 	}
       if (f < 0)
@@ -1080,7 +1080,7 @@ Using an Emacs configured with --with-x-toolkit=lucid does not have this problem
       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 @@ from the parent process and its tty file descriptors.  */)
   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 @@ from the parent process and its tty file descriptors.  */)
      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;
 
diff --git a/src/image.c b/src/image.c
index b3a52688eb..0938a11da7 100644
--- a/src/image.c
+++ b/src/image.c
@@ -2260,7 +2260,7 @@ x_find_image_file (Lisp_Object file)
   else
     {
       file_found = ENCODE_FILE (file_found);
-      close (fd);
+      emacs_close (fd);
     }
 
   return file_found;
diff --git a/src/inotify.c b/src/inotify.c
index 0bbcd44333..f4f850bf18 100644
--- a/src/inotify.c
+++ b/src/inotify.c
@@ -387,7 +387,7 @@ See inotify_rm_watch(2) for more information.
   /* Cleanup if no more files are watched. */
   if (NILP (watch_list))
     {
-      close (inotifyfd);
+      emacs_close (inotifyfd);
       delete_read_fd (inotifyfd);
       inotifyfd = -1;
     }
diff --git a/src/lread.c b/src/lread.c
index ae945d113d..5729cca956 100644
--- a/src/lread.c
+++ b/src/lread.c
@@ -1413,7 +1413,7 @@ directories, make sure the PREDICATE function returns `dir-ok' for them.  */)
   Lisp_Object file;
   int fd = openp (path, filename, suffixes, &file, predicate);
   if (NILP (predicate) && fd > 0)
-    close (fd);
+    emacs_close (fd);
   return file;
 }
 
diff --git a/src/process.c b/src/process.c
index 36ca1cf678..bdab1f8cb8 100644
--- a/src/process.c
+++ b/src/process.c
@@ -3555,14 +3555,14 @@ format; see the description of ADDRESS in `make-network-process'.  */)
       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 @@ FLAGS is the current flags of the interface.  */)
 #endif
   res = Fcons (elt, res);
 
-  close (s);
+  emacs_close (s);
 
   return any ? res : Qnil;
 }
diff --git a/src/sysdep.c b/src/sysdep.c
index 653cf22b07..a1f217b39a 100644
--- a/src/sysdep.c
+++ b/src/sysdep.c
@@ -2201,23 +2201,59 @@ emacs_fopen (char const *file, char const *mode)
   return fd < 0 ? 0 : fdopen (fd, mode);
 }
 
-int
-emacs_close (int fd)
+/* 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)
 {
-  bool did_retry = 0;
-  int rtnval;
+  /* Only the POSIX_CLOSE_RESTART case is emulated.  */
+  eassert (flag == POSIX_CLOSE_RESTART);
 
-  while ((rtnval = close (fd)) == -1
-	 && (errno == EINTR))
-    did_retry = 1;
+  /* Things are tricky if close (fd) returns -1 with errno == EINTR
+     on a system that does not define POSIX_CLOSE_RESTART.
 
-  /* 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;
+     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
 
-  return rtnval;
+/* 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)
+{
+  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.
diff --git a/src/term.c b/src/term.c
index 00a3f6837e..b6878a0abd 100644
--- a/src/term.c
+++ b/src/term.c
@@ -2478,7 +2478,7 @@ term_mouse_moveto (int x, int y)
   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 @@ init_tty (const char *name, const char *terminal_type, bool must_succeed)
                    name);
     if (!isatty (fd))
       {
-        close (fd);
+        emacs_close (fd);
         maybe_fatal (must_succeed, terminal,
                      "Not a tty device: %s",
                      "Not a tty device: %s",
diff --git a/src/termcap.c b/src/termcap.c
index 5f73107746..be05828eea 100644
--- a/src/termcap.c
+++ b/src/termcap.c
@@ -455,7 +455,7 @@ tgetent (char *bp, const char *name)
       /* 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 @@ tgetent (char *bp, const char *name)
       term = tgetst1 (tc_search_point, (char **) 0);
     }
 
-  close (fd);
+  emacs_close (fd);
   xfree (buf.beg);
 
   if (malloc_size)
diff --git a/src/unexaix.c b/src/unexaix.c
index 204f6cf4ad..45b3ca667b 100644
--- a/src/unexaix.c
+++ b/src/unexaix.c
@@ -97,7 +97,7 @@ report_error (const char *file, int fd)
   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 @@ static _Noreturn void ATTRIBUTE_FORMAT_PRINTF (2, 3)
 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 @@ unexec (const char *new_name, const char *a_name)
       || 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 @@ adjust_lnnoptrs (int writedesc, int readdesc, const char *new_name)
             }
 	}
     }
-  close (new);
+  emacs_close (new);
 
   return 0;
 }
diff --git a/src/unexcoff.c b/src/unexcoff.c
index e79821251b..6b2a3336c8 100644
--- a/src/unexcoff.c
+++ b/src/unexcoff.c
@@ -128,7 +128,7 @@ static void
 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 @@ report_error (const char *file, int fd)
 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 @@ adjust_lnnoptrs (int writedesc, int readdesc, const char *new_name)
 	}
     }
 #ifndef MSDOS
-  close (new);
+  emacs_close (new);
 #endif
   return 0;
 }
@@ -541,13 +541,13 @@ unexec (const char *new_name, const char *a_name)
       || 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);
 }
 
diff --git a/src/unexcw.c b/src/unexcw.c
index 0312a7328f..12435a8505 100644
--- a/src/unexcw.c
+++ b/src/unexcw.c
@@ -316,13 +316,13 @@ unexec (const char *outfile, const char *infile)
       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);
 }
diff --git a/src/unexelf.c b/src/unexelf.c
index 28847157e4..e241239328 100644
--- a/src/unexelf.c
+++ b/src/unexelf.c
@@ -1312,13 +1312,13 @@ temacs:
   /* 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)
diff --git a/src/unexhp9k800.c b/src/unexhp9k800.c
index 0f6eb87ee0..bee2517307 100644
--- a/src/unexhp9k800.c
+++ b/src/unexhp9k800.c
@@ -306,6 +306,6 @@ unexec (const char *new_name,      /* name of the new a.out file to be created *
   write_header (new, &hdr, &auxhdr);
 
   /* Close the binary file */
-  close (old);
-  close (new);
+  emacs_close (old);
+  emacs_close (new);
 }
diff --git a/src/unexmacosx.c b/src/unexmacosx.c
index 2bc6de177e..87848b012b 100644
--- a/src/unexmacosx.c
+++ b/src/unexmacosx.c
@@ -1332,7 +1332,7 @@ unexec (const char *outfile, const char *infile)
   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 @@ unexec (const char *outfile, const char *infile)
 
   dump_it ();
 
-  close (outfd);
+  emacs_close (outfd);
 }