GNU bug report logs - #33154
27.0.50; create_process on Darwin should not invoke setsid() after vfork() [PATCH]

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 33154 in the body.
You can then email your comments to 33154 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Filipp Gunbin <fgunbin <at> fastmail.fm>
To: bug-gnu-emacs <at> gnu.org
Subject: 27.0.50;
 create_process on Darwin should not invoke setsid() after vfork()
 [PATCH]
Date: Thu, 25 Oct 2018 22:30:12 +0300

This resulted from analysis of bug 33050.  To avoid repetition, here's
the message with explanation:
http://lists.gnu.org/archive/html/bug-gnu-emacs/2018-10/msg00763.html

Suggested patch is below.

Thanks.

In GNU Emacs 27.0.50 (build 12, x86_64-apple-darwin17.7.0, NS appkit-1561.60 Version 10.13.6 (Build 17G65))
 of 2018-10-25 built on fgunbin.playteam.ru
Repository revision: f1f1687fcd8d48cd519c0f2977bcecbf394a7f01
System Description:  Mac OS X 10.13.6


diff --git a/src/process.c b/src/process.c
index 6cda4f27ac..1f8810927d 100644
--- a/src/process.c
+++ b/src/process.c
@@ -2066,21 +2066,7 @@ create_process (Lisp_Object process, char **new_argv, Lisp_Object current_dir)
   int volatile forkerr_volatile = forkerr;
   struct Lisp_Process *p_volatile = p;
 
-#ifdef DARWIN_OS
-  /* Darwin doesn't let us run setsid after a vfork, so use fork when
-     necessary.  Also, reset SIGCHLD handling after a vfork, as
-     apparently macOS can mistakenly deliver SIGCHLD to the child.  */
-  if (pty_flag)
-    pid = fork ();
-  else
-    {
-      pid = vfork ();
-      if (pid == 0)
-	signal (SIGCHLD, SIG_DFL);
-    }
-#else
   pid = vfork ();
-#endif
 
   current_dir = current_dir_volatile;
   lisp_pty_name = lisp_pty_name_volatile;
@@ -2091,15 +2077,35 @@ create_process (Lisp_Object process, char **new_argv, Lisp_Object current_dir)
   p = p_volatile;
 
   pty_flag = p->pty_flag;
-
   if (pid == 0)
 #endif /* not WINDOWSNT */
     {
+#ifdef DARWIN_OS
+      /* Work around a macOS bug, where SIGCHLD is apparently
+	 delivered to a vforked child instead of to its parent.  See:
+	 https://lists.gnu.org/r/emacs-devel/2017-05/msg00342.html
+      */
+      signal (SIGCHLD, SIG_DFL);
+#endif
+
       /* 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.  */
+#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
       /* Make the pty's terminal the controlling terminal.  */
       if (pty_flag && forkin >= 0)
 	{

Message #8 received at 33154 <at> debbugs.gnu.org (full text, mbox):

From: Alan Third <alan <at> idiocy.org>
To: Filipp Gunbin <fgunbin <at> fastmail.fm>
Cc: 33154 <at> debbugs.gnu.org
Subject: Re: bug#33154: 27.0.50; create_process on Darwin should not invoke
 setsid() after vfork() [PATCH]
Date: Fri, 26 Oct 2018 12:12:27 +0100

On Thu, Oct 25, 2018 at 10:30:12PM +0300, Filipp Gunbin wrote:
> This resulted from analysis of bug 33050.  To avoid repetition, here's
> the message with explanation:
> http://lists.gnu.org/archive/html/bug-gnu-emacs/2018-10/msg00763.html

Sorry, I didn’t notice the other emails before now.

This looks fine to me, but I’m no expert on this stuff.

Just one point, though:

>        /* 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.  */
> +#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

The main block of this code looks identical to the BSD specific code
just underneath (#ifdef TIOCNOTTY), is there a reason we can’t reuse
that?

-- 
Alan Third

Message #11 received at 33154 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Alan Third <alan <at> idiocy.org>
Cc: 33154 <at> debbugs.gnu.org, Filipp Gunbin <fgunbin <at> fastmail.fm>
Subject: Re: 27.0.50; create_process on Darwin should not invoke setsid()
 after vfork() [PATCH]
Date: Mon, 5 Nov 2018 09:28:27 -0800

[Message part 1 (text/plain, inline)]

> The main block of this code looks identical to the BSD specific code
> just underneath (#ifdef TIOCNOTTY), is there a reason we can’t reuse
> that?

I don't see why not. Proposed patch attached; please give it a try as I 
don't use macOS.

[0001-Let-macOS-vfork-to-create-processes.patch (text/x-patch, attachment)]

Message #14 received at 33154 <at> debbugs.gnu.org (full text, mbox):

From: Filipp Gunbin <fgunbin <at> fastmail.fm>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: Alan Third <alan <at> idiocy.org>, 33154 <at> debbugs.gnu.org
Subject: Re: 27.0.50;
 create_process on Darwin should not invoke setsid() after vfork()
 [PATCH]
Date: Tue, 06 Nov 2018 16:46:06 +0300

On 05/11/2018 09:28 -0800, Paul Eggert wrote:

>> The main block of this code looks identical to the BSD specific code
>> just underneath (#ifdef TIOCNOTTY), is there a reason we can’t reuse
>> that?
>
> I don't see why not. Proposed patch attached; please give it a try as
> I don't use macOS.

Paul, thanks for the patch.

I'm seeing some problems with my fix - specifically, with sudo run as
async shell command:

"sudo: no tty present and no askpass program specified"

and with tramp when trying to connect to anything via ssh:

Permission denied, please try again.
Permission denied, please try again.
Received disconnect from <my-ip> port 22:2: Too many authentication failures
Disconnected from <my-ip> port 22

I didn't have time to look into it.  When I handle it, I'll try your
patch also and will write back.

Filipp

Message #17 received at 33154 <at> debbugs.gnu.org (full text, mbox):

From: Filipp Gunbin <fgunbin <at> fastmail.fm>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: Alan Third <alan <at> idiocy.org>, 33154 <at> debbugs.gnu.org
Subject: Re: 27.0.50;
 create_process on Darwin should not invoke setsid() after vfork()
 [PATCH]
Date: Wed, 07 Nov 2018 04:23:34 +0300

Paul, I think this won't work:

man tty (4):
     TIOCSCTTY void
                 Make the terminal the controlling terminal for the process
                 (the process must not currently have a controlling terminal).

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

I think what Alan meant as "reuse" was to extract the code into some
function.

Filipp

Message #20 received at 33154 <at> debbugs.gnu.org (full text, mbox):

From: Filipp Gunbin <fgunbin <at> fastmail.fm>
To: 33154 <at> debbugs.gnu.org
Subject: Re: bug#33154: 27.0.50;
 create_process on Darwin should not invoke setsid() after vfork()
 [PATCH]
Date: Wed, 07 Nov 2018 04:35:24 +0300

Well, my initial patch fixed the pipe case, but broke the pty case.
That became visible after make bootstrap.

This new patch seems to work.  It attempts to handle all cases:

- When not on Darwin, just do vfork() and setsid()

- When on Darwin and pipe mode, do the same as call_process(): vfork(),
  then detach from the current controlling terminal via TIOCNOTTY

- When on Darwin and pty mode, do fork() and later setsid().  We should
  create new session to be able to use just allocated tty.

I'll use it for some time, then report the result.

Filipp


diff --git a/src/process.c b/src/process.c
index 6cda4f27ac..9125936eb9 100644
--- a/src/process.c
+++ b/src/process.c
@@ -2099,7 +2099,23 @@ create_process (Lisp_Object process, char **new_argv, Lisp_Object current_dir)
 #ifdef HAVE_PTYS
       /* First, disconnect its current controlling terminal.
 	 Do this even if !PTY_FLAG; see Bug#30762.  */
+#ifdef DARWIN_OS
+      /* Darwin doesn't let us run setsid after a vfork, so use
+	 TIOCNOTTY when necessary. */
+      if (pty_flag)
+	setsid ();
+      else
+	{
+	  int j = emacs_open (DEV_TTY, O_RDWR, 0);
+	  if (j >= 0)
+	    {
+	      ioctl (j, TIOCNOTTY, 0);
+	      emacs_close (j);
+	    }
+	}
+#else
       setsid ();
+#endif
       /* Make the pty's terminal the controlling terminal.  */
       if (pty_flag && forkin >= 0)
 	{

Message #23 received at 33154 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Filipp Gunbin <fgunbin <at> fastmail.fm>
Cc: Alan Third <alan <at> idiocy.org>, 33154 <at> debbugs.gnu.org
Subject: Re: 27.0.50; create_process on Darwin should not invoke setsid()
 after vfork() [PATCH]
Date: Tue, 6 Nov 2018 23:41:46 -0800

[Message part 1 (text/plain, inline)]

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.

[0001-Dissociate-controlling-tty-better-on-Darwin.patch (text/x-patch, attachment)]

Message #26 received at 33154 <at> debbugs.gnu.org (full text, mbox):

From: Filipp Gunbin <fgunbin <at> fastmail.fm>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: Alan Third <alan <at> idiocy.org>, 33154 <at> debbugs.gnu.org
Subject: Re: 27.0.50;
 create_process on Darwin should not invoke setsid() after vfork()
 [PATCH]
Date: Wed, 07 Nov 2018 11:53:23 +0300

Yes, dissociate_controlling_tty looks nice.  I thought about checking
what setsid() returns too, but thought I'd do this after some testing.

> Also, I think we can call the function from just two places (not
> three).

Are you sure we can remove that 3rd place?  It dates back to initial
revision from 1992.  And I can't tell why it's there and what it does.

Filipp

Message #29 received at 33154 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Filipp Gunbin <fgunbin <at> fastmail.fm>
Cc: Alan Third <alan <at> idiocy.org>, 33154 <at> debbugs.gnu.org
Subject: Re: 27.0.50; create_process on Darwin should not invoke setsid()
 after vfork() [PATCH]
Date: Wed, 7 Nov 2018 07:40:36 -0800

Filipp Gunbin wrote:
> Are you sure we can remove that 3rd place?  It dates back to initial
> revision from 1992.  And I can't tell why it's there and what it does.

It's there to dissociate the controlling tty. And it's not removed, it's just 
moved into the previous call to dissociate_controlling_tty (when setsid fails).

It is a little disconcerting to change code this old. But we needn't worry about 
how it would run on 4.3BSD, only on current platforms. On most current platforms 
setsid suffices because POSIX says it should; on Darwin (and perhaps a few other 
BSD-derived systems) Emacs can fall back on TIOCNOTTY when setsid fails; and the 
proposed code does this more systematically than the current master does.

Message #32 received at 33154 <at> debbugs.gnu.org (full text, mbox):

From: Alan Third <alan <at> idiocy.org>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 33154 <at> debbugs.gnu.org, Filipp Gunbin <fgunbin <at> fastmail.fm>
Subject: Re: 27.0.50; create_process on Darwin should not invoke setsid()
 after vfork() [PATCH]
Date: Fri, 9 Nov 2018 00:07:41 +0000

On Tue, Nov 06, 2018 at 11:41:46PM -0800, Paul Eggert wrote:
> 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.

I only have two tests I know of to try here and they both pass with
this patch:

1. M‐x shell RET bg REST

doesn’t report that there’s no job control.

2. (benchmark 1 '(call-process "/usr/bin/true" nil nil nil))

Returns times in the order of 3ms, which is what we’d expect to see.

I’m not even sure if they’re really relevant, tbh. Is there anything
else I should try?
-- 
Alan Third

Message #35 received at 33154 <at> debbugs.gnu.org (full text, mbox):

From: Filipp Gunbin <fgunbin <at> fastmail.fm>
To: Alan Third <alan <at> idiocy.org>
Cc: Paul Eggert <eggert <at> cs.ucla.edu>, 33154 <at> debbugs.gnu.org
Subject: Re: 27.0.50;
 create_process on Darwin should not invoke setsid() after vfork()
 [PATCH]
Date: Fri, 09 Nov 2018 13:29:55 +0300

On 09/11/2018 00:07 +0000, Alan Third wrote:

> On Tue, Nov 06, 2018 at 11:41:46PM -0800, Paul Eggert wrote:
>> 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.
>
> I only have two tests I know of to try here and they both pass with
> this patch:
>
> 1. M‐x shell RET bg REST
>
> doesn’t report that there’s no job control.
>
> 2. (benchmark 1 '(call-process "/usr/bin/true" nil nil nil))
>
> Returns times in the order of 3ms, which is what we’d expect to see.
>
> I’m not even sure if they’re really relevant, tbh. Is there anything
> else I should try?

I have one failing case: "M-x shell-command sudo ls -la" reports "sudo:
no tty present and no askpass program specified".  I don't know what to
do about it yet, but we can defer this for later.

Filipp

Message #38 received at 33154 <at> debbugs.gnu.org (full text, mbox):

From: Andreas Schwab <schwab <at> linux-m68k.org>
To: Filipp Gunbin <fgunbin <at> fastmail.fm>
Cc: Alan Third <alan <at> idiocy.org>, Paul Eggert <eggert <at> cs.ucla.edu>,
 33154 <at> debbugs.gnu.org
Subject: Re: bug#33154: 27.0.50;
 create_process on Darwin should not invoke setsid() after vfork()
 [PATCH]
Date: Fri, 09 Nov 2018 12:16:39 +0100

On Nov 09 2018, Filipp Gunbin <fgunbin <at> fastmail.fm> wrote:

> I have one failing case: "M-x shell-command sudo ls -la" reports "sudo:
> no tty present and no askpass program specified".

That's not a bug.  If shell-command runs the command synchronously it
doesn't allocate a tty.

Andreas.

-- 
Andreas Schwab, schwab <at> linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Message #41 received at 33154 <at> debbugs.gnu.org (full text, mbox):

From: Filipp Gunbin <fgunbin <at> fastmail.fm>
To: Andreas Schwab <schwab <at> linux-m68k.org>
Cc: Alan Third <alan <at> idiocy.org>, Paul Eggert <eggert <at> cs.ucla.edu>,
 33154 <at> debbugs.gnu.org
Subject: Re: bug#33154: 27.0.50;
 create_process on Darwin should not invoke setsid() after vfork()
 [PATCH]
Date: Sat, 10 Nov 2018 18:24:39 +0300

On 09/11/2018 12:16 +0100, Andreas Schwab wrote:

> On Nov 09 2018, Filipp Gunbin <fgunbin <at> fastmail.fm> wrote:
>
>> I have one failing case: "M-x shell-command sudo ls -la" reports "sudo:
>> no tty present and no askpass program specified".
>
> That's not a bug.  If shell-command runs the command synchronously it
> doesn't allocate a tty.

Are there any reasons for not allocating a pty in this case?  A
synchronous program may be interactive.

Message #46 received at 33154-done <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Alan Third <alan <at> idiocy.org>
Cc: Filipp Gunbin <fgunbin <at> fastmail.fm>, 33154-done <at> debbugs.gnu.org
Subject: Re: 27.0.50; create_process on Darwin should not invoke setsid()
 after vfork() [PATCH]
Date: Sat, 10 Nov 2018 09:05:24 -0800

Alan Third wrote:

> I only have two tests I know of to try here and they both pass

Thanks for checking. I installed the patch on the master branch and am marking 
this bug as done.

Message #49 received at 33154 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Filipp Gunbin <fgunbin <at> fastmail.fm>, Andreas Schwab <schwab <at> linux-m68k.org>
Cc: Alan Third <alan <at> idiocy.org>, 33154 <at> debbugs.gnu.org
Subject: Re: bug#33154: 27.0.50; create_process on Darwin should not invoke
 setsid() after vfork() [PATCH]
Date: Sat, 10 Nov 2018 09:09:14 -0800

Filipp Gunbin wrote:
> On 09/11/2018 12:16 +0100, Andreas Schwab wrote:
> 
>> On Nov 09 2018, Filipp Gunbin <fgunbin <at> fastmail.fm> wrote:
>>
>>> I have one failing case: "M-x shell-command sudo ls -la" reports "sudo:
>>> no tty present and no askpass program specified".
>>
>> That's not a bug.  If shell-command runs the command synchronously it
>> doesn't allocate a tty.
> 
> Are there any reasons for not allocating a pty in this case?  A
> synchronous program may be interactive.

How would the interaction work, though? I can see potential problems with 
existing Lisp code that runs synchronous commands if we allocate ptys for them.

At any rate, that case has always failed the same way on Ubuntu 18.04 etc., so 
if this is a problem it's not limited to macOS and someone should file a new bug 
report for it.

Message #52 received at 33154 <at> debbugs.gnu.org (full text, mbox):

From: Filipp Gunbin <fgunbin <at> fastmail.fm>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: Alan Third <alan <at> idiocy.org>, 33154 <at> debbugs.gnu.org,
 Andreas Schwab <schwab <at> linux-m68k.org>
Subject: Re: bug#33154: 27.0.50;
 create_process on Darwin should not invoke setsid() after vfork()
 [PATCH]
Date: Sun, 11 Nov 2018 20:14:33 +0300

On 10/11/2018 09:09 -0800, Paul Eggert wrote:

> Filipp Gunbin wrote:
>> On 09/11/2018 12:16 +0100, Andreas Schwab wrote:
>>
>>> On Nov 09 2018, Filipp Gunbin <fgunbin <at> fastmail.fm> wrote:
>>>
>>>> I have one failing case: "M-x shell-command sudo ls -la" reports "sudo:
>>>> no tty present and no askpass program specified".
>>>
>>> That's not a bug.  If shell-command runs the command synchronously it
>>> doesn't allocate a tty.
>>
>> Are there any reasons for not allocating a pty in this case?  A
>> synchronous program may be interactive.
>
> How would the interaction work, though? I can see potential problems with
> existing Lisp code that runs synchronous commands if we allocate ptys
> for them.

Yes, I see, there's no way to interact.  Without interaction, there's no
sense in allocating pty.

> At any rate, that case has always failed the same way on Ubuntu 18.04 etc., so
> if this is a problem it's not limited to macOS and someone should file a new bug
> report for it.

No need for that.

Thanks.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.