job-control: set CLOEXEC on pty processes. #5986

Before this change, new processes started with libuv prevented SIGHUP
from reaching pty processes (by keeping the ptmx file descriptor open).

src/nvim/ex_cmds2.c

@@ -2692,14 +2692,7 @@ static FILE *fopen_noinh_readbin(char *filename)
     return NULL;
   }
 
-#ifdef HAVE_FD_CLOEXEC
+  (void)os_set_cloexec(fd_tmp);
-  {
-    int fdflags = fcntl(fd_tmp, F_GETFD);
-    if (fdflags >= 0 && (fdflags & FD_CLOEXEC) == 0) {
-      (void)fcntl(fd_tmp, F_SETFD, fdflags | FD_CLOEXEC);
-    }
-  }
-#endif
 
   return fdopen(fd_tmp, READBIN);
 }

src/nvim/fileio.c

@@ -1742,16 +1742,11 @@ failed:
   }
 # endif
 
-  if (!read_buffer && !read_stdin)
+  if (!read_buffer && !read_stdin) {
-    close(fd);                                  /* errors are ignored */
+    close(fd);  // errors are ignored
-#ifdef HAVE_FD_CLOEXEC
+  } else {
-  else {
+    (void)os_set_cloexec(fd);
-    int fdflags = fcntl(fd, F_GETFD);
-    if (fdflags >= 0 && (fdflags & FD_CLOEXEC) == 0) {
-      (void)fcntl(fd, F_SETFD, fdflags | FD_CLOEXEC);
-    }
   }
-#endif
   xfree(buffer);
 
   if (read_stdin) {

src/nvim/memfile.c

@@ -909,12 +909,7 @@ static bool mf_do_open(memfile_T *mfp, char_u *fname, int flags)
     return false;
   }
 
-#ifdef HAVE_FD_CLOEXEC
+  (void)os_set_cloexec(mfp->mf_fd);
-  int fdflags = fcntl(mfp->mf_fd, F_GETFD);
-  if (fdflags >= 0 && (fdflags & FD_CLOEXEC) == 0) {
-    (void)fcntl(mfp->mf_fd, F_SETFD, fdflags | FD_CLOEXEC);
-  }
-#endif
 #ifdef HAVE_SELINUX
   mch_copy_sec(fname, mfp->mf_fname);
 #endif

src/nvim/memline.c

@@ -439,14 +439,7 @@ void ml_setname(buf_T *buf)
       EMSG(_("E301: Oops, lost the swap file!!!"));
       return;
     }
-#ifdef HAVE_FD_CLOEXEC
+    (void)os_set_cloexec(mfp->mf_fd);
-    {
-      int fdflags = fcntl(mfp->mf_fd, F_GETFD);
-      if (fdflags >= 0 && (fdflags & FD_CLOEXEC) == 0) {
-        (void)fcntl(mfp->mf_fd, F_SETFD, fdflags | FD_CLOEXEC);
-      }
-    }
-#endif
   }
   if (!success)
     EMSG(_("E302: Could not rename swap file"));

src/nvim/os/fs.c

@@ -391,6 +391,35 @@ int os_open(const char* path, int flags, int mode)
   return r;
 }
 
+/// Sets file descriptor `fd` to close-on-exec.
+//
+// @return -1 if failed to set, 0 otherwise.
+int os_set_cloexec(const int fd)
+{
+#ifdef HAVE_FD_CLOEXEC
+  int e;
+  int fdflags = fcntl(fd, F_GETFD);
+  if (fdflags < 0) {
+    e = errno;
+    ELOG("Failed to get flags on descriptor %d: %s", fd, strerror(e));
+    errno = e;
+    return -1;
+  }
+  if ((fdflags & FD_CLOEXEC) == 0
+      && fcntl(fd, F_SETFD, fdflags | FD_CLOEXEC) < 0) {
+    e = errno;
+    ELOG("Failed to set CLOEXEC on descriptor %d: %s", fd, strerror(e));
+    errno = e;
+    return -1;
+  }
+  return 0;
+#endif
+
+  // No FD_CLOEXEC flag. On Windows, the file should have been opened with
+  // O_NOINHERIT anyway.
+  return -1;
+}
+
 /// Close a file
 ///
 /// @return 0 or libuv error code on failure.

src/nvim/os/pty_process_unix.c

@@ -73,6 +73,13 @@ int pty_process_spawn(PtyProcess *ptyproc)
     goto error;
   }
 
+  // Other jobs and providers should not get a copy of this file descriptor.
+  if (os_set_cloexec(master) == -1) {
+    status = -errno;
+    ELOG("Failed to set CLOEXEC on ptmx file descriptor");
+    goto error;
+  }
+
   if (proc->in
       && (status = set_duplicating_descriptor(master, &proc->in->uv.pipe))) {
     goto error;
@@ -215,14 +222,24 @@ static int set_duplicating_descriptor(int fd, uv_pipe_t *pipe)
     ELOG("Failed to dup descriptor %d: %s", fd, strerror(errno));
     return status;
   }
+
+  if (os_set_cloexec(fd_dup) == -1) {
+    status = -errno;
+    ELOG("Failed to set CLOEXEC on duplicate fd");
+    goto error;
+  }
+
   status = uv_pipe_open(pipe, fd_dup);
   if (status) {
     ELOG("Failed to set pipe to descriptor %d: %s",
          fd_dup, uv_strerror(status));
-    close(fd_dup);
+    goto error;
-    return status;
   }
   return status;
+
+error:
+  close(fd_dup);
+  return status;
 }
 
 static void chld_handler(uv_signal_t *handle, int signum)

test/functional/core/job_spec.lua

@@ -1,7 +1,7 @@
 local helpers = require('test.functional.helpers')(after_each)
-local clear, eq, eval, execute, feed, insert, neq, next_msg, nvim,
+local clear, eq, eval, exc_exec, execute, feed, insert, neq, next_msg, nvim,
   nvim_dir, ok, source, write_file, mkdir, rmdir = helpers.clear,
-  helpers.eq, helpers.eval, helpers.execute, helpers.feed,
+  helpers.eq, helpers.eval, helpers.exc_exec, helpers.execute, helpers.feed,
   helpers.insert, helpers.neq, helpers.next_message, helpers.nvim,
   helpers.nvim_dir, helpers.ok, helpers.source,
   helpers.write_file, helpers.mkdir, helpers.rmdir
@@ -622,6 +622,26 @@ describe('jobs', function()
       msg = (msg[2] == 'stdout') and next_msg() or msg  -- Skip stdout, if any.
       eq({'notification', 'exit', {0, 42}}, msg)
     end)
+
+    it('jobstart() does not keep ptmx file descriptor open', function()
+      -- Start another job (using libuv)
+      command('let g:job_opts.pty = 0')
+      local other_jobid = eval("jobstart(['cat', '-'], g:job_opts)")
+      local other_pid = eval('jobpid(' .. other_jobid .. ')')
+
+      -- Other job doesn't block first job from recieving SIGHUP on jobclose()
+      command('call jobclose(j)')
+      -- Have to wait so that the SIGHUP can be processed by tty-test on time.
+      -- Can't wait for the next message in case this test fails, if it fails
+      -- there won't be any more messages, and the test would hang.
+      helpers.sleep(100)
+      local err = exc_exec('call jobpid(j)')
+      eq('Vim(call):E900: Invalid job id', err)
+
+      -- cleanup
+      eq(other_pid, eval('jobpid(' .. other_jobid .. ')'))
+      command('call jobstop(' .. other_jobid .. ')')
+    end)
   end)
 end)