Looking at "int vforked" in signal handler is racy

Looking at "int vforked" in signal handler is racy

[Denys Vlasenko]

struct job *vforkexec(union node *n, char **argv, const char *path, int idx)
{
         struct job *jp;
         int pid;

         jp = makejob(1);

         sigblockall(NULL);
         vforked++;

<<<< Parent can get a signal here.

         pid = vfork();

         if (!pid) {
                 forkchild(jp, n, FORK_FG);
                 sigclearmask();
                 shellexec(argv, path, idx);
                 /* NOTREACHED */
         }
<<<< Parent can get a signal here. The window is in fact not that small:
<<<< in the child, execve() syscall is rather complex (needs to tear down memory
<<<< mappings, which causes TLB invalidation) and takes time.
<<<< all this time parent is blocked and does not execute,
<<<< so it don't yet reach the next line:

         vforked = 0;
	sigclearmask();


Signal handler:

void
onsig(int signo)
{
         if (vforked)
                 return;
^^^^^^^^^^ this assumes we dot signal in the vforked child,
but it may be false! We may be in the parent!


How to solve this.... at any given time,
there are only two processes which share address space via vfork,
all other possibly existing copies of shell processes (such as
the grandparent shell if the parent is a subshell) have a
separate address space.

So, in signal handler, we are in three possible situations:

* we don't have a "vfork sibling" at all (we aren't after vfork)
* we are after vfork and we are parent: need to handle the signal
* we are after vfork and we are child: must not mess up the parent's
   address space, thus do NOT handle the signal
   (or else it can e.g. set intpending = 1 *in the parent too*!)

I see this solution:

vfork_parent_pid = getpid();
have_vfork_sibling = 1;
pid = vfork();
if (!pid) { child ops; NORETURN; }
have_vfork_sibling = 0;

void
onsig(int signo)
{
         if (after_vfork && getpid() != vfork_parent_pid)
                 /* We are vfork child, DO NOT MODIFY ANY VARIABLES! */
                 return;

Re: Looking at "int vforked" in signal handler is racy

[Denys Vlasenko]

> I see this solution:
> 
> vfork_parent_pid = getpid();
> have_vfork_sibling = 1;
> pid = vfork();
> if (!pid) { child ops; NORETURN; }
> have_vfork_sibling = 0;
> 
> void
> onsig(int signo)
> {
>          if (after_vfork && getpid() != vfork_parent_pid)
                ^^^^^^^^^^^s/after_vfork/have_vfork_sibling/

>                  /* We are vfork child, DO NOT MODIFY ANY VARIABLES! */
>                  return;

Re: Looking at "int vforked" in signal handler is racy

[Harald van Dijk]

Hi,

On 09/08/2025 14:29, Denys Vlasenko wrote:
> struct job *vforkexec(union node *n, char **argv, const char *path, int 
> idx)
> {
>          struct job *jp;
>          int pid;
> 
>          jp = makejob(1);
> 
>          sigblockall(NULL);
>          vforked++;
> 
> <<<< Parent can get a signal here.
The sigblockall(NULL) is meant to prevent that from happening. Likewise 
for the next one you point out. Is sigblockall(NULL) insufficient for 
some reason?

Cheers,
Harald van Dijk

Re: Looking at "int vforked" in signal handler is racy

[Denys Vlasenko]

On 8/9/25 15:52, Harald van Dijk wrote:
> Hi,
> 
> On 09/08/2025 14:29, Denys Vlasenko wrote:
>> struct job *vforkexec(union node *n, char **argv, const char *path, int idx)
>> {
>>          struct job *jp;
>>          int pid;
>>
>>          jp = makejob(1);
>>
>>          sigblockall(NULL);
>>          vforked++;
>>
>> <<<< Parent can get a signal here.
> The sigblockall(NULL) is meant to prevent that from happening.

Yep, missed that. Should be ok. I'm mistaken.

However, this method requires three syscalls:
one to mask all signals before vfork,
then two syscalls (one in the parent and one in the child)
to unmask them back.

Whereas the method of recording PID usually needs
just one getpid() syscall.
Should we consider switching to that method?

Re: Looking at "int vforked" in signal handler is racy

[Harald van Dijk]

On 10/08/2025 20:33, Denys Vlasenko wrote:
> On 8/9/25 15:52, Harald van Dijk wrote:
>> Hi,
>>
>> On 09/08/2025 14:29, Denys Vlasenko wrote:
>>> struct job *vforkexec(union node *n, char **argv, const char *path, 
>>> int idx)
>>> {
>>>          struct job *jp;
>>>          int pid;
>>>
>>>          jp = makejob(1);
>>>
>>>          sigblockall(NULL);
>>>          vforked++;
>>>
>>> <<<< Parent can get a signal here.
>> The sigblockall(NULL) is meant to prevent that from happening.
> 
> Yep, missed that. Should be ok. I'm mistaken.
> 
> However, this method requires three syscalls:
> one to mask all signals before vfork,
> then two syscalls (one in the parent and one in the child)
> to unmask them back.
> 
> Whereas the method of recording PID usually needs
> just one getpid() syscall.
> Should we consider switching to that method?

That approach seems solid to me at a very quick glance. I think you 
don't even need separate have_vfork_sibling and vfork_parent_pid 
variables, just make the existing vforked variable a pid, or 0. A mostly 
untested patch using that:

--- a/src/jobs.c
+++ b/src/jobs.c
@@ -991,20 +991,17 @@ struct job *vforkexec(union node *n, char **argv, 
const char *path, int idx)

         jp = makejob(1);

-       sigblockall(NULL);
-       vforked++;
+       vforked = getpid();

         pid = vfork();

         if (!pid) {
                 forkchild(jp, n, FORK_FG);
-               sigclearmask();
                 shellexec(argv, path, idx);
                 /* NOTREACHED */
         }

         vforked = 0;
-       sigclearmask();
         forkparent(jp, n, FORK_FG, pid);

         return jp;
--- a/src/trap.c
+++ b/src/trap.c
@@ -312,7 +312,7 @@ ignoresig(int signo)
  void
  onsig(int signo)
  {
-       if (vforked)
+       if (vforked && getpid() != vforked)
                 return;

         if (signo == SIGCHLD) {

I do wonder if the use of vforked in a signal handler, even in current 
dash, would require the use of volatile to ensure no compiler 
optimisations mess with it, but I think that is not affected by this 
patch. If it is necessary, it is already necessary now, and if it is not 
necessary now, it will not become necessary with this patch.

Cheers,
Harald van Dijk

Re: Looking at "int vforked" in signal handler is racy

[Herbert Xu]

On Sun, Aug 10, 2025 at 11:20:40PM +0100, Harald van Dijk wrote:
>
> That approach seems solid to me at a very quick glance. I think you don't
> even need separate have_vfork_sibling and vfork_parent_pid variables, just
> make the existing vforked variable a pid, or 0. A mostly untested patch
> using that:

Looks good to me.  Could you turn this into a patch please? If
you could run some quick speed tests on this versus the existing
code that would be even better :)

> I do wonder if the use of vforked in a signal handler, even in current dash,
> would require the use of volatile to ensure no compiler optimisations mess
> with it, but I think that is not affected by this patch. If it is necessary,
> it is already necessary now, and if it is not necessary now, it will not
> become necessary with this patch.

Yes vfork should be marked as volatile.  Although being a global
variable means that there shouldn't be any practical difference.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: Looking at "int vforked" in signal handler is racy

[Harald van Dijk]

On 11/08/2025 06:00, Herbert Xu wrote:
> On Sun, Aug 10, 2025 at 11:20:40PM +0100, Harald van Dijk wrote:
>>
>> That approach seems solid to me at a very quick glance. I think you don't
>> even need separate have_vfork_sibling and vfork_parent_pid variables, just
>> make the existing vforked variable a pid, or 0. A mostly untested patch
>> using that:
> 
> Looks good to me.  Could you turn this into a patch please? If
> you could run some quick speed tests on this versus the existing
> code that would be even better :)

It will be a little while before I can do meaningful measurements, but I 
will do so when I can.

Cheers,
Harald van Dijk

Re: Looking at "int vforked" in signal handler is racy

[Harald van Dijk]

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

On 11/08/2025 13:58, Harald van Dijk wrote:
> On 11/08/2025 06:00, Herbert Xu wrote:
>> On Sun, Aug 10, 2025 at 11:20:40PM +0100, Harald van Dijk wrote:
>>>
>>> That approach seems solid to me at a very quick glance. I think you 
>>> don't
>>> even need separate have_vfork_sibling and vfork_parent_pid variables, 
>>> just
>>> make the existing vforked variable a pid, or 0. A mostly untested patch
>>> using that:
>>
>> Looks good to me.  Could you turn this into a patch please? If
>> you could run some quick speed tests on this versus the existing
>> code that would be even better :)
> 
> It will be a little while before I can do meaningful measurements, but I 
> will do so when I can.

The impact on config.status --recheck, measured by running it 100 times 
with the patch, 100 times without, and averaging the run times, reveals 
that the impact is so low it is lost in the noise, and not even clear it 
is beneficial. One attempt showed an average of 2.225s without the 
patch, 2.221s with the patch. Doing the exact same measurements again 
showed an average of 2.221s without the patch, 2.223s with the patch.

The build was using clang 19.1.7 on x86_64, no special compiler flags, 
just -O2.

The patch that I tested is attached.

I welcome anyone doing their own measurements using better benchmarks to 
see if they can determine whether this patch is useful.

Cheers,
Harald van Dijk

[-- Attachment #2: 0001-jobs-avoid-blocking-signals-on-vfork.patch --]
[-- Type: text/x-patch, Size: 1246 bytes --]

From a112e6581540bf7d34bb2f47042e8927bcd95d0d Mon Sep 17 00:00:00 2001
From: Harald van Dijk <harald@gigawatt.nl>
Date: Fri, 22 Aug 2025 02:05:14 +0100
Subject: [PATCH] jobs: avoid blocking signals on vfork().

As pointed out by Denys Vlasenko, we can avoid blocking signals on
vfork() by making the signal handler of a vfork child immediately
return. This saves a syscall.
---
 src/jobs.c | 5 +----
 src/trap.c | 2 +-
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/src/jobs.c b/src/jobs.c
index 51e6fa1..83d9694 100644
--- a/src/jobs.c
+++ b/src/jobs.c
@@ -991,20 +991,17 @@ struct job *vforkexec(union node *n, char **argv, const char *path, int idx)
 
 	jp = makejob(1);
 
-	sigblockall(NULL);
-	vforked++;
+	vforked = getpid();
 
 	pid = vfork();
 
 	if (!pid) {
 		forkchild(jp, n, FORK_FG);
-		sigclearmask();
 		shellexec(argv, path, idx);
 		/* NOTREACHED */
 	}
 
 	vforked = 0;
-	sigclearmask();
 	forkparent(jp, n, FORK_FG, pid);
 
 	return jp;
diff --git a/src/trap.c b/src/trap.c
index aebffa0..23829a5 100644
--- a/src/trap.c
+++ b/src/trap.c
@@ -312,7 +312,7 @@ ignoresig(int signo)
 void
 onsig(int signo)
 {
-	if (vforked)
+	if (vforked && getpid() != vforked)
 		return;
 
 	if (signo == SIGCHLD) {
-- 
2.47.2

Re: Looking at "int vforked" in signal handler is racy

[Herbert Xu]

On Fri, Aug 22, 2025 at 02:32:54AM +0100, Harald van Dijk wrote:
>
> From a112e6581540bf7d34bb2f47042e8927bcd95d0d Mon Sep 17 00:00:00 2001
> From: Harald van Dijk <harald@gigawatt.nl>
> Date: Fri, 22 Aug 2025 02:05:14 +0100
> Subject: [PATCH] jobs: avoid blocking signals on vfork().
> 
> As pointed out by Denys Vlasenko, we can avoid blocking signals on
> vfork() by making the signal handler of a vfork child immediately
> return. This saves a syscall.
> ---
>  src/jobs.c | 5 +----
>  src/trap.c | 2 +-
>  2 files changed, 2 insertions(+), 5 deletions(-)

Patch applied.  Thanks.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt