Conformance problem in system()

Conformance problem in system()

[Markus Wichmann]

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

Hi all,

I was testing system() while SIGCHLD is ignored just now, and noticed a
difference between glibc and musl. That in itself is not alarming, but
it prompted me to look this function up in the standard, to see which
behavior was conforming better (or maybe both are).

The program was this:

#include <signal.h>
#include <stdio.h>
#include <stdlib.h>

int main(void)
{
    if (signal(SIGCHLD, SIG_IGN) == SIG_ERR)
        perror("signal");
    int i = system("sleep 1");
    if (i != 0)
        fprintf(stderr, "system() == %d, %m\n", i);
    return i;
}

Glibc returns -1 from system() while musl returns 32512. errno is the
same in both cases, ECHILD. Which is also what waitpid() is returning.

SUSv4 specifies for system()'s return value:

| [...][I]f the termination status for the command language interpreter
| cannot be obtained, system() shall return -1 and set errno to indicate
| the error.

So, it turns out, glibc is conforming to the standard, and musl is not.
If waitpid() fails, then the status cannot be retrieved.

I don't know about other system calls, but I tend to view pointer
arguments to system calls as undefined after a failed call. The kernel
might deposit something in those variables, or it might not. In this
case, it does not, so the status returned is the value status was
initialized to at the start of system(), which is 0x7f00, the value I
observed. I think you were hoping to land under the umbrella of

| If command is not a null pointer, system() shall return the termination
| status of the command language interpreter in the format specified by
| waitpid().

But a failing waitpid() does not fetch the status at all.

I have attached a patch to correct this behavior, for your perusal.

Ciao,
Markus

[-- Attachment #2: 0002-Correctly-handle-failing-waitpid-in-system.patch --]
[-- Type: text/x-diff, Size: 1194 bytes --]

From a2a4e552e617af76f6352bd280a042af621ffcf4 Mon Sep 17 00:00:00 2001
From: Markus Wichmann <nullplan@gmx.net>
Date: Fri, 29 Dec 2017 10:45:58 +0100
Subject: [PATCH 2/2] Correctly handle failing waitpid() in system()

---
 src/process/system.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/process/system.c b/src/process/system.c
index 8cbdda06..a62ff26e 100644
--- a/src/process/system.c
+++ b/src/process/system.c
@@ -14,7 +14,7 @@ int system(const char *cmd)
 	pid_t pid;
 	sigset_t old, reset;
 	struct sigaction sa = { .sa_handler = SIG_IGN }, oldint, oldquit;
-	int status = 0x7f00, ret;
+	int status = 0x7f00, ret, wr = 0;
 	posix_spawnattr_t attr;
 
 	pthread_testcancel();
@@ -37,11 +37,12 @@ int system(const char *cmd)
 		(char *[]){"sh", "-c", (char *)cmd, 0}, __environ);
 	posix_spawnattr_destroy(&attr);
 
-	if (!ret) while (waitpid(pid, &status, 0)<0 && errno == EINTR);
+	if (!ret) while ((wr = waitpid(pid, &status, 0))<0 && errno == EINTR);
 	sigaction(SIGINT, &oldint, NULL);
 	sigaction(SIGQUIT, &oldquit, NULL);
 	sigprocmask(SIG_SETMASK, &old, NULL);
 
 	if (ret) errno = ret;
+        if (wr) status = wr;
 	return status;
 }
-- 
2.14.2

Re: Conformance problem in system()

[Rich Felker]

On Fri, Dec 29, 2017 at 10:48:48AM +0100, Markus Wichmann wrote:
> Hi all,
> 
> I was testing system() while SIGCHLD is ignored just now, and noticed a
> difference between glibc and musl. That in itself is not alarming, but
> it prompted me to look this function up in the standard, to see which
> behavior was conforming better (or maybe both are).
> 
> The program was this:
> 
> #include <signal.h>
> #include <stdio.h>
> #include <stdlib.h>
> 
> int main(void)
> {
>     if (signal(SIGCHLD, SIG_IGN) == SIG_ERR)
>         perror("signal");
>     int i = system("sleep 1");
>     if (i != 0)
>         fprintf(stderr, "system() == %d, %m\n", i);
>     return i;
> }
> 
> Glibc returns -1 from system() while musl returns 32512. errno is the
> same in both cases, ECHILD. Which is also what waitpid() is returning.
> 
> SUSv4 specifies for system()'s return value:
> 
> | [...][I]f the termination status for the command language interpreter
> | cannot be obtained, system() shall return -1 and set errno to indicate
> | the error.
> 
> So, it turns out, glibc is conforming to the standard, and musl is not.
> If waitpid() fails, then the status cannot be retrieved.
> 
> I don't know about other system calls, but I tend to view pointer
> arguments to system calls as undefined after a failed call. The kernel
> might deposit something in those variables, or it might not. In this
> case, it does not, so the status returned is the value status was
> initialized to at the start of system(), which is 0x7f00, the value I
> observed. I think you were hoping to land under the umbrella of
> 
> | If command is not a null pointer, system() shall return the termination
> | status of the command language interpreter in the format specified by
> | waitpid().
> 
> But a failing waitpid() does not fetch the status at all.
> 
> I have attached a patch to correct this behavior, for your perusal.

I think you're right that there's a problem here, but I don't think
the patch correctly or fully fixes it. A simpler version of what
you're doing would be to just initialize status to -1 instead of
0x7f00, since your patch is returning -1 in all cases where waitpid
did not complete successfully. But that ignores the POSIX requirement
to behave as if the interpreter exited with status 127 when it was
possibel to make the child process but the command interpreter could
not be executed.

musl's posix_spawn does not succeed when exec fails in the child;
instead the exec error is returned. This behavior is permitted but not
required by POSIX. I think it would actually be preferable to system
to return -1 and set errno in this case too, but POSIX doesn't seem to
allow that. So I think we actually need to break down error cases for
posix_spawn and simulate the exit(127) result if the error returned is
anything other than an error that could be returned by fork (EAGAIN or
ENOMEM).

Does that sound right?

Rich

Re: Conformance problem in system()

[Markus Wichmann]

On Sat, Dec 30, 2017 at 05:22:04PM -0500, Rich Felker wrote:
> I think you're right that there's a problem here, but I don't think
> the patch correctly or fully fixes it. A simpler version of what
> you're doing would be to just initialize status to -1 instead of
> 0x7f00, since your patch is returning -1 in all cases where waitpid
> did not complete successfully. But that ignores the POSIX requirement
> to behave as if the interpreter exited with status 127 when it was
> possibel to make the child process but the command interpreter could
> not be executed.
> 

Actually, I noticed another problem: waitpid() returns the PID of the
changed child process on success, so the

if (wr) status = wr;

should be

if (wr < 0) status = wr;

The initialization of status would only change something if the kernel
did not write to status on waitpid() failure. Is that guarenteed ABI, or
does this just happen to be the case on current kernels?

> musl's posix_spawn does not succeed when exec fails in the child;
> instead the exec error is returned. This behavior is permitted but not
> required by POSIX. I think it would actually be preferable to system
> to return -1 and set errno in this case too, but POSIX doesn't seem to
> allow that.

Actually, the requirement to return exit status 127 on exec failure
sounds mighty specific to me. As if someone wanted to codify behavior
they needed in their utility. Which means there may be software out
there that depends on this behavior.

There is the possibility of not considering a posix_spawn()ed child
process as "created" unless posix_spawn() itself did return success,
though. But that might run counter to what the POSIX was going for,
here.

> So I think we actually need to break down error cases for
> posix_spawn and simulate the exit(127) result if the error returned is
> anything other than an error that could be returned by fork (EAGAIN or
> ENOMEM).
> 
> Does that sound right?

Sounds correct, but I don't like it. Not least because any syscall may
fail with any errno in addition to the ones documented. But all the
solutions I can think of are even worse. In the end, it looks like POSIX
was codifying the use of fork(), exec(), and exit(127) in the child
here. Any implementation that does something different now has to work
around the mess they've made.

> 
> Rich

Ciao,
Markus

Re: Conformance problem in system()

[Rich Felker]

On Sun, Dec 31, 2017 at 12:23:42AM +0100, Markus Wichmann wrote:
> On Sat, Dec 30, 2017 at 05:22:04PM -0500, Rich Felker wrote:
> > I think you're right that there's a problem here, but I don't think
> > the patch correctly or fully fixes it. A simpler version of what
> > you're doing would be to just initialize status to -1 instead of
> > 0x7f00, since your patch is returning -1 in all cases where waitpid
> > did not complete successfully. But that ignores the POSIX requirement
> > to behave as if the interpreter exited with status 127 when it was
> > possibel to make the child process but the command interpreter could
> > not be executed.
> > 
> 
> Actually, I noticed another problem: waitpid() returns the PID of the
> changed child process on success, so the
> 
> if (wr) status = wr;
> 
> should be
> 
> if (wr < 0) status = wr;
> 
> The initialization of status would only change something if the kernel
> did not write to status on waitpid() failure. Is that guarenteed ABI, or
> does this just happen to be the case on current kernels?
> 
> > musl's posix_spawn does not succeed when exec fails in the child;
> > instead the exec error is returned. This behavior is permitted but not
> > required by POSIX. I think it would actually be preferable to system
> > to return -1 and set errno in this case too, but POSIX doesn't seem to
> > allow that.
> 
> Actually, the requirement to return exit status 127 on exec failure
> sounds mighty specific to me. As if someone wanted to codify behavior
> they needed in their utility. Which means there may be software out
> there that depends on this behavior.
> 
> There is the possibility of not considering a posix_spawn()ed child
> process as "created" unless posix_spawn() itself did return success,
> though. But that might run counter to what the POSIX was going for,
> here.

I think this is an acceptable interpretation for now. So just changing
default initialization of status to -1 should work, right?

Rich