[PATCH 1/2] pasta: correctly exit when execvp() fails

[PATCH 1/2] pasta: correctly exit when execvp() fails

[Paul Holzinger]

By default clone() will create a child that does not send SIGCHLD when
the child exits. The caller has to specifiy the SIGNAL it should get in
the flag bitmask.
see clone(2) under "The child termination signal"

This fixes the problem where pasta would not exit when the execvp()
call failed, i.e. when the command does not exists.

Signed-off-by: Paul Holzinger <pholzing@redhat.com>
---
 pasta.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/pasta.c b/pasta.c
index 528f02a..3f6477c 100644
--- a/pasta.c
+++ b/pasta.c
@@ -229,7 +229,7 @@ void pasta_start_ns(struct ctx *c, uid_t uid, gid_t gid,
 	pasta_child_pid = do_clone(pasta_spawn_cmd, ns_fn_stack,
 				   sizeof(ns_fn_stack),
 				   CLONE_NEWIPC | CLONE_NEWPID | CLONE_NEWNET |
-				   CLONE_NEWUTS,
+				   CLONE_NEWUTS | SIGCHLD,
 				   (void *)&arg);
 
 	if (pasta_child_pid == -1) {
-- 
2.39.1

[PATCH 2/2] pasta: propagate exit code from child command

[Paul Holzinger]

Exits codes are very useful for scripts, when the pasta child execvp()
call fails with ENOENT that parent should also exit with > 0. In short
the parent should always exit with the code from the child to make it
useful in scripts.

It is easy to test with: `pasta -- bash -c "exit 3"; echo $?`

Signed-off-by: Paul Holzinger <pholzing@redhat.com>
---
 pasta.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/pasta.c b/pasta.c
index 3f6477c..4b18d7e 100644
--- a/pasta.c
+++ b/pasta.c
@@ -64,9 +64,14 @@ void pasta_child_handler(int signal)
 
 	if (pasta_child_pid &&
 	    !waitid(P_PID, pasta_child_pid, &infop, WEXITED | WNOHANG)) {
-		if (infop.si_pid == pasta_child_pid)
-			exit(EXIT_SUCCESS);
-			/* Nothing to do, detached PID namespace going away */
+		if (infop.si_pid == pasta_child_pid) {
+			if (infop.si_code == CLD_EXITED)
+				exit(infop.si_status);
+
+			/* else killed by signal, si_status == SIGNUM in this case */
+			exit(infop.si_status + 128);
+		}
+		/* Nothing to do, detached PID namespace going away */
 	}
 
 	waitid(P_ALL, 0, NULL, WEXITED | WNOHANG);
-- 
2.39.1

Re: [PATCH 2/2] pasta: propagate exit code from child command

[Stefano Brivio]

On Wed,  8 Feb 2023 16:54:41 +0100
Paul Holzinger <pholzing@redhat.com> wrote:

> Exits codes are very useful for scripts, when the pasta child execvp()
> call fails with ENOENT that parent should also exit with > 0. In short
> the parent should always exit with the code from the child to make it
> useful in scripts.
> 
> It is easy to test with: `pasta -- bash -c "exit 3"; echo $?`
> 
> Signed-off-by: Paul Holzinger <pholzing@redhat.com>
> ---
>  pasta.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/pasta.c b/pasta.c
> index 3f6477c..4b18d7e 100644
> --- a/pasta.c
> +++ b/pasta.c
> @@ -64,9 +64,14 @@ void pasta_child_handler(int signal)
>  
>  	if (pasta_child_pid &&
>  	    !waitid(P_PID, pasta_child_pid, &infop, WEXITED | WNOHANG)) {
> -		if (infop.si_pid == pasta_child_pid)
> -			exit(EXIT_SUCCESS);
> -			/* Nothing to do, detached PID namespace going away */
> +		if (infop.si_pid == pasta_child_pid) {
> +			if (infop.si_code == CLD_EXITED)
> +				exit(infop.si_status);
> +
> +			/* else killed by signal, si_status == SIGNUM in this case */

It took me a while to figure out that SIGNUM is not a constant I wasn't
aware of, just the signal number -- I guess you meant 'signum' from
sigaction() or suchlike?

Maybe you could go with:

			/* If killed by a signal, si_status is its number */

> +			exit(infop.si_status + 128);

This is a bit arbitrary -- returning n + 128 is what Bash, zsh, and a
few other shells do, and probably a reasonable convention given that
POSIX says we need to return a value greater than 128. Still I would
mention it:

			/* Arbitrary: return signal number + 128 */

or:

			/* If killed by a signal, si_status is the number.
			 * Follow common shell convention of returning it + 128.
			 */

> +		}
> +		/* Nothing to do, detached PID namespace going away */

This should be moved back into the conditional block (swapping the two
lines above, or moving this comment at the beginning of the block).

If si_pid is not the child, we need to do other stuff (that is, reap
clones).

>  	}
>  
>  	waitid(P_ALL, 0, NULL, WEXITED | WNOHANG);

-- 
Stefano

Re: [PATCH 2/2] pasta: propagate exit code from child command

[Paul Holzinger]

Thanks for the feedback, I will send a v2 with the comments fixed.

On 08/02/2023 19:14, Stefano Brivio wrote:
> On Wed,  8 Feb 2023 16:54:41 +0100
> Paul Holzinger <pholzing@redhat.com> wrote:
>
>> Exits codes are very useful for scripts, when the pasta child execvp()
>> call fails with ENOENT that parent should also exit with > 0. In short
>> the parent should always exit with the code from the child to make it
>> useful in scripts.
>>
>> It is easy to test with: `pasta -- bash -c "exit 3"; echo $?`
>>
>> Signed-off-by: Paul Holzinger <pholzing@redhat.com>
>> ---
>>   pasta.c | 11 ++++++++---
>>   1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/pasta.c b/pasta.c
>> index 3f6477c..4b18d7e 100644
>> --- a/pasta.c
>> +++ b/pasta.c
>> @@ -64,9 +64,14 @@ void pasta_child_handler(int signal)
>>   
>>   	if (pasta_child_pid &&
>>   	    !waitid(P_PID, pasta_child_pid, &infop, WEXITED | WNOHANG)) {
>> -		if (infop.si_pid == pasta_child_pid)
>> -			exit(EXIT_SUCCESS);
>> -			/* Nothing to do, detached PID namespace going away */
>> +		if (infop.si_pid == pasta_child_pid) {
>> +			if (infop.si_code == CLD_EXITED)
>> +				exit(infop.si_status);
>> +
>> +			/* else killed by signal, si_status == SIGNUM in this case */
> It took me a while to figure out that SIGNUM is not a constant I wasn't
> aware of, just the signal number -- I guess you meant 'signum' from
> sigaction() or suchlike?
>
> Maybe you could go with:
>
> 			/* If killed by a signal, si_status is its number */
>
>> +			exit(infop.si_status + 128);
> This is a bit arbitrary -- returning n + 128 is what Bash, zsh, and a
> few other shells do, and probably a reasonable convention given that
> POSIX says we need to return a value greater than 128. Still I would
> mention it:
>
> 			/* Arbitrary: return signal number + 128 */
>
> or:
>
> 			/* If killed by a signal, si_status is the number.
> 			 * Follow common shell convention of returning it + 128.
> 			 */

The second suggestion sounds good to me.

>> +		}
>> +		/* Nothing to do, detached PID namespace going away */
> This should be moved back into the conditional block (swapping the two
> lines above, or moving this comment at the beginning of the block).
Whoops, this was by accident.
>
> If si_pid is not the child, we need to do other stuff (that is, reap
> clones).
>
>>   	}
>>   
>>   	waitid(P_ALL, 0, NULL, WEXITED | WNOHANG);

Re: [PATCH 1/2] pasta: correctly exit when execvp() fails

[Stefano Brivio]

On Wed,  8 Feb 2023 16:54:40 +0100
Paul Holzinger <pholzing@redhat.com> wrote:

> By default clone() will create a child that does not send SIGCHLD when
> the child exits. The caller has to specifiy the SIGNAL it should get in
> the flag bitmask.
> see clone(2) under "The child termination signal"
> 
> This fixes the problem where pasta would not exit when the execvp()
> call failed, i.e. when the command does not exists.
> 
> Signed-off-by: Paul Holzinger <pholzing@redhat.com>

Thanks, this looks good to me, pushing later.

-- 
Stefano

Re: [PATCH 1/2] pasta: correctly exit when execvp() fails

[Stefano Brivio]

On Wed,  8 Feb 2023 16:54:40 +0100
Paul Holzinger <pholzing@redhat.com> wrote:

> By default clone() will create a child that does not send SIGCHLD when
> the child exits. The caller has to specifiy the SIGNAL it should get in
> the flag bitmask.
> see clone(2) under "The child termination signal"
> 
> This fixes the problem where pasta would not exit when the execvp()
> call failed, i.e. when the command does not exists.
> 
> Signed-off-by: Paul Holzinger <pholzing@redhat.com>

Applied.

-- 
Stefano