bug#33154: 27.0.50; create_process on Darwin should not invoke setsid() after vfork() [PATCH]

[Paul Eggert <eggert@cs.ucla.edu>]

[-- Attachment #1: Type: text/plain, Size: 816 bytes --]

Filipp Gunbin wrote:
> In your patch, we don't detach from current (Emacs's) controlling
> terminal before doing TIOCSCTTY.

Ah, OK. I see also that vfork won't work on Darwin if pty mode is used, since 
Emacs wants to create a new session and Darwin setsid always fails in a vforked 
child that has not yet execed.

However, your patch introduces another duplicate of the open/TIOCNOTTY/close 
fallback code, making three duplicates in all. How about if we coalesce these 
duplicates into a function and then call that function? Also, I think we can 
call the function from just two places (not three). Furthermore, I think it'd be 
more robust if Emacs does setsid everywhere (with a fallback to 
open/TIOCNOTTY/close everywhere TIOCNOTTY is available), not just Darwin. 
Proposed patch (against master) attached.

[-- Attachment #2: 0001-Dissociate-controlling-tty-better-on-Darwin.patch --]
[-- Type: text/x-patch, Size: 3706 bytes --]

From cff3581f79d5eeb251fb683250bf48da3f68895a Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Tue, 6 Nov 2018 23:30:26 -0800
Subject: [PATCH] Dissociate controlling tty better on Darwin

* src/process.c (dissociate_controlling_tty): New function.
(create_process): Use it to dissociate controlling tty if setsid
fails, which happens on Darwin after a vfork (Bug#33154).
Do this on all platforms, not just on Darwin, as a similar
problem is plausible elsewhere.
* src/callproc.c (call_process): Use the new function here, too,
for consistency and to avoid duplicate code.
---
 src/callproc.c | 14 +-------------
 src/process.c  | 40 ++++++++++++++++++++++------------------
 src/process.h  |  1 +
 3 files changed, 24 insertions(+), 31 deletions(-)

diff --git a/src/callproc.c b/src/callproc.c
index a2cfd2e94d..9f47c79b81 100644
--- a/src/callproc.c
+++ b/src/callproc.c
@@ -643,19 +643,7 @@ call_process (ptrdiff_t nargs, Lisp_Object *args, int filefd,
 #endif
 
       unblock_child_signal (&oldset);
-
-#ifdef DARWIN_OS
-      /* Darwin doesn't let us run setsid after a vfork, so use
-         TIOCNOTTY when necessary. */
-      int j = emacs_open (DEV_TTY, O_RDWR, 0);
-      if (j >= 0)
-        {
-          ioctl (j, TIOCNOTTY, 0);
-          emacs_close (j);
-        }
-#else
-      setsid ();
-#endif
+      dissociate_controlling_tty ();
 
       /* Emacs ignores SIGPIPE, but the child should not.  */
       signal (SIGPIPE, SIG_DFL);
diff --git a/src/process.c b/src/process.c
index 6cda4f27ac..7e78e172d3 100644
--- a/src/process.c
+++ b/src/process.c
@@ -1949,6 +1949,26 @@ close_process_fd (int *fd_addr)
     }
 }
 
+void
+dissociate_controlling_tty (void)
+{
+  if (setsid () < 0)
+    {
+#ifdef TIOCNOTTY
+      /* Needed on Darwin after vfork, since setsid fails in a vforked
+	 child that has not execed.
+	 I wonder: would just ioctl (fd, TIOCNOTTY, 0) work here, for
+	 some fd that the caller already has?  */
+      int ttyfd = emacs_open (DEV_TTY, O_RDWR, 0);
+      if (0 <= ttyfd)
+	{
+	  ioctl (ttyfd, TIOCNOTTY, 0);
+	  emacs_close (ttyfd);
+	}
+#endif
+    }
+}
+
 /* Indexes of file descriptors in open_fds.  */
 enum
   {
@@ -2097,9 +2117,8 @@ create_process (Lisp_Object process, char **new_argv, Lisp_Object current_dir)
     {
       /* Make the pty be the controlling terminal of the process.  */
 #ifdef HAVE_PTYS
-      /* First, disconnect its current controlling terminal.
-	 Do this even if !PTY_FLAG; see Bug#30762.  */
-      setsid ();
+      dissociate_controlling_tty ();
+
       /* Make the pty's terminal the controlling terminal.  */
       if (pty_flag && forkin >= 0)
 	{
@@ -2128,21 +2147,6 @@ create_process (Lisp_Object process, char **new_argv, Lisp_Object current_dir)
 	}
 #endif
 #endif
-#ifdef TIOCNOTTY
-      /* In 4.3BSD, the TIOCSPGRP bug has been fixed, and now you
-	 can do TIOCSPGRP only to the process's controlling tty.  */
-      if (pty_flag)
-	{
-	  /* I wonder: would just ioctl (0, TIOCNOTTY, 0) work here?
-	     I can't test it since I don't have 4.3.  */
-	  int j = emacs_open (DEV_TTY, O_RDWR, 0);
-	  if (j >= 0)
-	    {
-	      ioctl (j, TIOCNOTTY, 0);
-	      emacs_close (j);
-	    }
-	}
-#endif /* TIOCNOTTY */
 
 #if !defined (DONT_REOPEN_PTY)
 /*** There is a suggestion that this ought to be a
diff --git a/src/process.h b/src/process.h
index 3c6dd7b91f..67b783400d 100644
--- a/src/process.h
+++ b/src/process.h
@@ -300,6 +300,7 @@ extern Lisp_Object network_interface_info (Lisp_Object);
 extern Lisp_Object remove_slash_colon (Lisp_Object);
 
 extern void update_processes_for_thread_death (Lisp_Object);
+extern void dissociate_controlling_tty (void);
 
 INLINE_HEADER_END
 
-- 
2.17.1