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

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

From a112e6581540bf7d34bb2f47042e8927bcd95d0d Mon Sep 17 00:00:00 2001
From: Harald van Dijk <[email protected]>
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