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

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