From bacba3c26522ef297662bace31947d3e4f47c87a Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Thu, 11 Jul 2013 19:03:47 -0700 Subject: [PATCH] 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. --- ChangeLog | 5 ++++ configure.ac | 2 +- src/ChangeLog | 25 +++++++++++++++++++ src/callproc.c | 2 +- src/dired.c | 2 +- src/emacs.c | 10 ++++---- src/image.c | 2 +- src/inotify.c | 2 +- src/lread.c | 2 +- src/process.c | 6 ++--- src/sysdep.c | 62 +++++++++++++++++++++++++++++++++++++---------- src/term.c | 4 +-- src/termcap.c | 4 +-- src/unexaix.c | 12 ++++----- src/unexcoff.c | 12 ++++----- src/unexcw.c | 4 +-- src/unexelf.c | 6 ++--- src/unexhp9k800.c | 4 +-- src/unexmacosx.c | 4 +-- 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 + + Fix races with threads and file descriptors. + * configure.ac (PTY_TTY_NAME_SPRINTF): Use emacs_close, not close. + 2013-07-10 Paul Eggert * 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 + + 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 * 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 + . */ + +#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); } -- 2.20.1