Re: kern/56979: fork(2) fails to be signal safe

[Taylor R Campbell <riastradh%NetBSD.org@localhost>]

The following reply was made to PR lib/56979; it has been noted by GNATS.

From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
To: Tom Lane <tgl%sss.pgh.pa.us@localhost>
Cc: gnats-bugs%NetBSD.org@localhost
Subject: Re: kern/56979: fork(2) fails to be signal safe
Date: Thu, 25 Aug 2022 22:54:52 +0000

 This is a multi-part message in MIME format.
 --=_3LM3Qr5snjR6jwWdKR2eeC80vJ4rOpBA
 
 Can you try the attached patch series?  Do you have a quick reproducer
 for the problem?  Ideally we could make an automatic test for this if
 there's a cheap reproducer to trigger the race.
 
 --=_3LM3Qr5snjR6jwWdKR2eeC80vJ4rOpBA
 Content-Type: text/plain; charset="ISO-8859-1"; name="forksignalsafety"
 Content-Transfer-Encoding: quoted-printable
 Content-Disposition: attachment; filename="forksignalsafety.patch"
 
 From 14e63dd753fad250b3b82701b02a654dd6913a32 Mon Sep 17 00:00:00 2001
 From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
 Date: Thu, 25 Aug 2022 22:49:00 +0000
 Subject: [PATCH 1/2] pthread_atfork(3): Block signals during the call to
  pthread_atfork.
 
 This doesn't affect the calls to the atfork handlers -- it only
 protects access to the lists of handlers from interruption by a
 signal, in case the signal handler calls fork(2).
 ---
  lib/libc/gen/pthread_atfork.c | 23 +++++++++++++++--------
  1 file changed, 15 insertions(+), 8 deletions(-)
 
 diff --git a/lib/libc/gen/pthread_atfork.c b/lib/libc/gen/pthread_atfork.c
 index 9d72261eb6cf..22f2371f44b2 100644
 --- a/lib/libc/gen/pthread_atfork.c
 +++ b/lib/libc/gen/pthread_atfork.c
 @@ -101,15 +101,20 @@ pthread_atfork(void (*prepare)(void), void (*parent)(=
 void),
      void (*child)(void))
  {
  	struct atfork_callback *newprepare, *newparent, *newchild;
 +	sigset_t mask, omask;
 +	int error;
 =20
  	newprepare =3D newparent =3D newchild =3D NULL;
 =20
 +	sigfillset(&mask);
 +	thr_sigsetmask(SIG_SETMASK, &mask, &omask);
 +
  	mutex_lock(&atfork_lock);
  	if (prepare !=3D NULL) {
  		newprepare =3D af_alloc();
  		if (newprepare =3D=3D NULL) {
 -			mutex_unlock(&atfork_lock);
 -			return ENOMEM;
 +			error =3D ENOMEM;
 +			goto out;
  		}
  		newprepare->fn =3D prepare;
  	}
 @@ -119,8 +124,8 @@ pthread_atfork(void (*prepare)(void), void (*parent)(vo=
 id),
  		if (newparent =3D=3D NULL) {
  			if (newprepare !=3D NULL)
  				af_free(newprepare);
 -			mutex_unlock(&atfork_lock);
 -			return ENOMEM;
 +			error =3D ENOMEM;
 +			goto out;
  		}
  		newparent->fn =3D parent;
  	}
 @@ -132,8 +137,8 @@ pthread_atfork(void (*prepare)(void), void (*parent)(vo=
 id),
  				af_free(newprepare);
  			if (newparent !=3D NULL)
  				af_free(newparent);
 -			mutex_unlock(&atfork_lock);
 -			return ENOMEM;
 +			error =3D ENOMEM;
 +			goto out;
  		}
  		newchild->fn =3D child;
  	}
 @@ -150,9 +155,11 @@ pthread_atfork(void (*prepare)(void), void (*parent)(v=
 oid),
  		SIMPLEQ_INSERT_TAIL(&parentq, newparent, next);
  	if (child)
  		SIMPLEQ_INSERT_TAIL(&childq, newchild, next);
 -	mutex_unlock(&atfork_lock);
 +	error =3D 0;
 =20
 -	return 0;
 +out:	mutex_unlock(&atfork_lock);
 +	thr_sigsetmask(SIG_SETMASK, &omask, NULL);
 +	return error;
  }
 =20
  pid_t
 
 From f21ba3adae5fddd6a991b1ba40642db1465ecc99 Mon Sep 17 00:00:00 2001
 From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
 Date: Thu, 25 Aug 2022 22:50:00 +0000
 Subject: [PATCH 2/2] ld.elf_so(8): Make fork take a shared, not exclusive,
  lock.
 
 We only need to ensure that there are no concurrent modifications to
 the rtld data structures in flight, since the threads that began
 those modifications will not exist in the child and will therefore be
 unable to complete them in the child.
 
 A shared lock suffices to ensure there are no such concurrent
 modifications in flight; an exclusive lock is not necessary, and can
 cause deadlock if fork is executed from a signal handler, which is
 explicitly allowed by POSIX (and our own sigaction(2) man page) which
 marks fork as async-signal-safe.
 
 PR lib/56979
 ---
  libexec/ld.elf_so/rtld.c | 5 ++---
  1 file changed, 2 insertions(+), 3 deletions(-)
 
 diff --git a/libexec/ld.elf_so/rtld.c b/libexec/ld.elf_so/rtld.c
 index 5ea978e2e61d..bb37b7f9f232 100644
 --- a/libexec/ld.elf_so/rtld.c
 +++ b/libexec/ld.elf_so/rtld.c
 @@ -1539,14 +1539,13 @@ pid_t __fork(void);
  __dso_public pid_t
  __locked_fork(int *my_errno)
  {
 -	sigset_t mask;
  	pid_t result;
 =20
 -	_rtld_exclusive_enter(&mask);
 +	_rtld_shared_enter();
  	result =3D __fork();
  	if (result =3D=3D -1)
  		*my_errno =3D errno;
 -	_rtld_exclusive_exit(&mask);
 +	_rtld_shared_exit();
 =20
  	return result;
  }
 
 --=_3LM3Qr5snjR6jwWdKR2eeC80vJ4rOpBA--