[PATCH] pasta: Save errno on signal handler entry, restore on return when needed

[PATCH] pasta: Save errno on signal handler entry, restore on return when needed

[Stefano Brivio]

Ed reported this:

  # Error: pasta failed with exit code 1:
  # Couldn't drop cap 3 from bounding set
  # : No child processes

in a Podman CI run with tests being run in parallel. The error message
itself, by the way, is fixed by commit 1cd773081f12 ("log: Drop
newlines in the middle of the perror()-like messages"), but how can we
possibly get ECHILD as failure code for prctl()?

Well, we don't, but if we exit early enough, pasta_child_handler()
might run before we're even done with isolation steps, and it calls
waitid(), which sets errno. We need to restore it before returning
from the signal handler (if we return after calling functions that
might set it), as signal-safety(7) also implies:

       Fetching and setting the value of errno is async-signal-safe
       provided that the signal handler saves errno on entry and
       restores its value before returning.

Eventually, we'll probably need to switch to signalfd(2) the day we
want to implement multithreading, but this will do for the moment.

Reported-by: Ed Santiago <santiago@redhat.com>
Link: https://github.com/containers/podman/issues/23478
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
 pasta.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/pasta.c b/pasta.c
index 572048d..b4a3d99 100644
--- a/pasta.c
+++ b/pasta.c
@@ -61,6 +61,7 @@ int pasta_child_pid;
  */
 void pasta_child_handler(int signal)
 {
+	int errno_save = errno;
 	siginfo_t infop;
 
 	(void)signal;
@@ -85,6 +86,8 @@ void pasta_child_handler(int signal)
 
 	waitid(P_ALL, 0, NULL, WEXITED | WNOHANG);
 	waitid(P_ALL, 0, NULL, WEXITED | WNOHANG);
+
+	errno = errno_save;
 }
 
 /**
-- 
@@ -61,6 +61,7 @@ int pasta_child_pid;
  */
 void pasta_child_handler(int signal)
 {
+	int errno_save = errno;
 	siginfo_t infop;
 
 	(void)signal;
@@ -85,6 +86,8 @@ void pasta_child_handler(int signal)
 
 	waitid(P_ALL, 0, NULL, WEXITED | WNOHANG);
 	waitid(P_ALL, 0, NULL, WEXITED | WNOHANG);
+
+	errno = errno_save;
 }
 
 /**
-- 
2.43.0

Re: [PATCH] pasta: Save errno on signal handler entry, restore on return when needed

[Paul Holzinger]

On 02/08/2024 15:34, Stefano Brivio wrote:
> Ed reported this:
>
>    # Error: pasta failed with exit code 1:
>    # Couldn't drop cap 3 from bounding set
>    # : No child processes
>
> in a Podman CI run with tests being run in parallel. The error message
> itself, by the way, is fixed by commit 1cd773081f12 ("log: Drop
> newlines in the middle of the perror()-like messages"), but how can we
> possibly get ECHILD as failure code for prctl()?
>
> Well, we don't, but if we exit early enough, pasta_child_handler()
> might run before we're even done with isolation steps, and it calls
> waitid(), which sets errno. We need to restore it before returning
> from the signal handler (if we return after calling functions that
> might set it), as signal-safety(7) also implies:
>
>         Fetching and setting the value of errno is async-signal-safe
>         provided that the signal handler saves errno on entry and
>         restores its value before returning.
>
> Eventually, we'll probably need to switch to signalfd(2) the day we
> want to implement multithreading, but this will do for the moment.
>
> Reported-by: Ed Santiago <santiago@redhat.com>
> Link: https://github.com/containers/podman/issues/23478
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Reviewed-by: Paul Holzinger <pholzing@redhat.com>

Re: [PATCH] pasta: Save errno on signal handler entry, restore on return when needed

[David Gibson]

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

On Fri, Aug 02, 2024 at 03:34:48PM +0200, Stefano Brivio wrote:
> Ed reported this:
> 
>   # Error: pasta failed with exit code 1:
>   # Couldn't drop cap 3 from bounding set
>   # : No child processes
> 
> in a Podman CI run with tests being run in parallel. The error message
> itself, by the way, is fixed by commit 1cd773081f12 ("log: Drop
> newlines in the middle of the perror()-like messages"), but how can we
> possibly get ECHILD as failure code for prctl()?
> 
> Well, we don't, but if we exit early enough, pasta_child_handler()
> might run before we're even done with isolation steps, and it calls
> waitid(), which sets errno. We need to restore it before returning
> from the signal handler (if we return after calling functions that
> might set it), as signal-safety(7) also implies:
> 
>        Fetching and setting the value of errno is async-signal-safe
>        provided that the signal handler saves errno on entry and
>        restores its value before returning.
> 
> Eventually, we'll probably need to switch to signalfd(2) the day we
> want to implement multithreading, but this will do for the moment.
> 
> Reported-by: Ed Santiago <santiago@redhat.com>
> Link: https://github.com/containers/podman/issues/23478
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

Ouch.  errno, another of those awful APIs we're stuck with :/

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  pasta.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/pasta.c b/pasta.c
> index 572048d..b4a3d99 100644
> --- a/pasta.c
> +++ b/pasta.c
> @@ -61,6 +61,7 @@ int pasta_child_pid;
>   */
>  void pasta_child_handler(int signal)
>  {
> +	int errno_save = errno;
>  	siginfo_t infop;
>  
>  	(void)signal;
> @@ -85,6 +86,8 @@ void pasta_child_handler(int signal)
>  
>  	waitid(P_ALL, 0, NULL, WEXITED | WNOHANG);
>  	waitid(P_ALL, 0, NULL, WEXITED | WNOHANG);
> +
> +	errno = errno_save;
>  }
>  
>  /**

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]