[PATCH] linux-user/signal: Map exit signals in SIGCHLD siginfo_t

[PATCH] linux-user/signal: Map exit signals in SIGCHLD siginfo_t

[Matthias Schiffer]

When converting a siginfo_t from waitid(), the interpretation of si_status
depends on the value of si_code: For CLD_EXITED, it is an exit code and
should be copied verbatim. For other codes, it is a signal number
(possibly with additional high bits from ptrace) that should be mapped.

This code was previously changed in commit 1c3dfb506ea3
("linux-user/signal: Decode waitid si_code"), but the fix was
incomplete.

Tested with the following test program:

    #include <stdio.h>
    #include <stdlib.h>
    #include <unistd.h>
    #include <sys/wait.h>

    int main() {
    	pid_t pid = fork();
    	if (pid == 0) {
    		exit(12);
    	} else {
    		siginfo_t siginfo = {};
    		waitid(P_PID, pid, &siginfo, WEXITED);
    		printf("Code: %d, status: %d\n", (int)siginfo.si_code, (int)siginfo.si_status);
    	}

    	pid = fork();
    	if (pid == 0) {
    		raise(SIGUSR2);
    	} else {
    		siginfo_t siginfo = {};
    		waitid(P_PID, pid, &siginfo, WEXITED);
    		printf("Code: %d, status: %d\n", (int)siginfo.si_code, (int)siginfo.si_status);
    	}
    }

Output with an x86_64 host and mips64el target before 1c3dfb506ea3
(incorrect: exit code 12 is translated like a signal):

    Code: 1, status: 17
    Code: 2, status: 17

After 1c3dfb506ea3 (incorrect: signal number is not translated):

    Code: 1, status: 12
    Code: 2, status: 12

With this patch:

    Code: 1, status: 12
    Code: 2, status: 17

Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
---
 linux-user/signal.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/linux-user/signal.c b/linux-user/signal.c
index 14d8fdfde152..8e3af98ec0a7 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -403,7 +403,12 @@ static inline void host_to_target_siginfo_noswap(target_siginfo_t *tinfo,
         case TARGET_SIGCHLD:
             tinfo->_sifields._sigchld._pid = info->si_pid;
             tinfo->_sifields._sigchld._uid = info->si_uid;
-            tinfo->_sifields._sigchld._status = info->si_status;
+            if (si_code == CLD_EXITED)
+                tinfo->_sifields._sigchld._status = info->si_status;
+            else
+                tinfo->_sifields._sigchld._status
+                    = host_to_target_signal(info->si_status & 0x7f)
+                        | (info->si_status & ~0x7f);
             tinfo->_sifields._sigchld._utime = info->si_utime;
             tinfo->_sifields._sigchld._stime = info->si_stime;
             si_type = QEMU_SI_CHLD;
-- 
2.33.1

Re: [PATCH] linux-user/signal: Map exit signals in SIGCHLD siginfo_t

[Matthias Schiffer]

[-- Attachment #1.1: Type: text/plain, Size: 2820 bytes --]

On 23/10/2021 21:59, Matthias Schiffer wrote:
> When converting a siginfo_t from waitid(), the interpretation of si_status
> depends on the value of si_code: For CLD_EXITED, it is an exit code and
> should be copied verbatim. For other codes, it is a signal number
> (possibly with additional high bits from ptrace) that should be mapped.
> 
> This code was previously changed in commit 1c3dfb506ea3
> ("linux-user/signal: Decode waitid si_code"), but the fix was
> incomplete.

ping

> 
> Tested with the following test program:
> 
>      #include <stdio.h>
>      #include <stdlib.h>
>      #include <unistd.h>
>      #include <sys/wait.h>
> 
>      int main() {
>      	pid_t pid = fork();
>      	if (pid == 0) {
>      		exit(12);
>      	} else {
>      		siginfo_t siginfo = {};
>      		waitid(P_PID, pid, &siginfo, WEXITED);
>      		printf("Code: %d, status: %d\n", (int)siginfo.si_code, (int)siginfo.si_status);
>      	}
> 
>      	pid = fork();
>      	if (pid == 0) {
>      		raise(SIGUSR2);
>      	} else {
>      		siginfo_t siginfo = {};
>      		waitid(P_PID, pid, &siginfo, WEXITED);
>      		printf("Code: %d, status: %d\n", (int)siginfo.si_code, (int)siginfo.si_status);
>      	}
>      }
> 
> Output with an x86_64 host and mips64el target before 1c3dfb506ea3
> (incorrect: exit code 12 is translated like a signal):
> 
>      Code: 1, status: 17
>      Code: 2, status: 17
> 
> After 1c3dfb506ea3 (incorrect: signal number is not translated):
> 
>      Code: 1, status: 12
>      Code: 2, status: 12
> 
> With this patch:
> 
>      Code: 1, status: 12
>      Code: 2, status: 17
> 
> Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
> ---
>   linux-user/signal.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index 14d8fdfde152..8e3af98ec0a7 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -403,7 +403,12 @@ static inline void host_to_target_siginfo_noswap(target_siginfo_t *tinfo,
>           case TARGET_SIGCHLD:
>               tinfo->_sifields._sigchld._pid = info->si_pid;
>               tinfo->_sifields._sigchld._uid = info->si_uid;
> -            tinfo->_sifields._sigchld._status = info->si_status;
> +            if (si_code == CLD_EXITED)
> +                tinfo->_sifields._sigchld._status = info->si_status;
> +            else
> +                tinfo->_sifields._sigchld._status
> +                    = host_to_target_signal(info->si_status & 0x7f)
> +                        | (info->si_status & ~0x7f);
>               tinfo->_sifields._sigchld._utime = info->si_utime;
>               tinfo->_sifields._sigchld._stime = info->si_stime;
>               si_type = QEMU_SI_CHLD;


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH] linux-user/signal: Map exit signals in SIGCHLD siginfo_t

[Laurent Vivier]

CC'ing Alistair and Andreas that were involved in original fix 1c3dfb506ea3 ("linux-user/signal: 
Decode waitid si_code")

Thanks,
Laurent

Le 23/10/2021 à 21:59, Matthias Schiffer a écrit :
> When converting a siginfo_t from waitid(), the interpretation of si_status
> depends on the value of si_code: For CLD_EXITED, it is an exit code and
> should be copied verbatim. For other codes, it is a signal number
> (possibly with additional high bits from ptrace) that should be mapped.
> 
> This code was previously changed in commit 1c3dfb506ea3
> ("linux-user/signal: Decode waitid si_code"), but the fix was
> incomplete.
> 
> Tested with the following test program:
> 
>      #include <stdio.h>
>      #include <stdlib.h>
>      #include <unistd.h>
>      #include <sys/wait.h>
> 
>      int main() {
>      	pid_t pid = fork();
>      	if (pid == 0) {
>      		exit(12);
>      	} else {
>      		siginfo_t siginfo = {};
>      		waitid(P_PID, pid, &siginfo, WEXITED);
>      		printf("Code: %d, status: %d\n", (int)siginfo.si_code, (int)siginfo.si_status);
>      	}
> 
>      	pid = fork();
>      	if (pid == 0) {
>      		raise(SIGUSR2);
>      	} else {
>      		siginfo_t siginfo = {};
>      		waitid(P_PID, pid, &siginfo, WEXITED);
>      		printf("Code: %d, status: %d\n", (int)siginfo.si_code, (int)siginfo.si_status);
>      	}
>      }
> 
> Output with an x86_64 host and mips64el target before 1c3dfb506ea3
> (incorrect: exit code 12 is translated like a signal):
> 
>      Code: 1, status: 17
>      Code: 2, status: 17
> 
> After 1c3dfb506ea3 (incorrect: signal number is not translated):
> 
>      Code: 1, status: 12
>      Code: 2, status: 12
> 
> With this patch:
> 
>      Code: 1, status: 12
>      Code: 2, status: 17
> 
> Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
> ---
>   linux-user/signal.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index 14d8fdfde152..8e3af98ec0a7 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -403,7 +403,12 @@ static inline void host_to_target_siginfo_noswap(target_siginfo_t *tinfo,
>           case TARGET_SIGCHLD:
>               tinfo->_sifields._sigchld._pid = info->si_pid;
>               tinfo->_sifields._sigchld._uid = info->si_uid;
> -            tinfo->_sifields._sigchld._status = info->si_status;
> +            if (si_code == CLD_EXITED)
> +                tinfo->_sifields._sigchld._status = info->si_status;
> +            else
> +                tinfo->_sifields._sigchld._status
> +                    = host_to_target_signal(info->si_status & 0x7f)
> +                        | (info->si_status & ~0x7f);
>               tinfo->_sifields._sigchld._utime = info->si_utime;
>               tinfo->_sifields._sigchld._stime = info->si_stime;
>               si_type = QEMU_SI_CHLD;
> 

Re: [PATCH] linux-user/signal: Map exit signals in SIGCHLD siginfo_t

[Andreas K. Huettel]

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

Well, the original fix 1c3dfb506ea3 did clearly improve things for me, but it wasn't
complete yet. At some point I gave up on finding a minimal reproducer for my remaining 
problems (futex-related hangs in a complex python+bash app).

So, this *may* be the missing piece.

Will test, but that takes a few days.

Andreas

Am Sonntag, 19. Dezember 2021, 16:55:16 CET schrieb Laurent Vivier:
> CC'ing Alistair and Andreas that were involved in original fix 1c3dfb506ea3 ("linux-user/signal: 
> Decode waitid si_code")
> 
> Thanks,
> Laurent
> 
> Le 23/10/2021 à 21:59, Matthias Schiffer a écrit :
> > When converting a siginfo_t from waitid(), the interpretation of si_status
> > depends on the value of si_code: For CLD_EXITED, it is an exit code and
> > should be copied verbatim. For other codes, it is a signal number
> > (possibly with additional high bits from ptrace) that should be mapped.
> > 
> > This code was previously changed in commit 1c3dfb506ea3
> > ("linux-user/signal: Decode waitid si_code"), but the fix was
> > incomplete.
> > 
> > Tested with the following test program:
> > 
> >      #include <stdio.h>
> >      #include <stdlib.h>
> >      #include <unistd.h>
> >      #include <sys/wait.h>
> > 
> >      int main() {
> >      	pid_t pid = fork();
> >      	if (pid == 0) {
> >      		exit(12);
> >      	} else {
> >      		siginfo_t siginfo = {};
> >      		waitid(P_PID, pid, &siginfo, WEXITED);
> >      		printf("Code: %d, status: %d\n", (int)siginfo.si_code, (int)siginfo.si_status);
> >      	}
> > 
> >      	pid = fork();
> >      	if (pid == 0) {
> >      		raise(SIGUSR2);
> >      	} else {
> >      		siginfo_t siginfo = {};
> >      		waitid(P_PID, pid, &siginfo, WEXITED);
> >      		printf("Code: %d, status: %d\n", (int)siginfo.si_code, (int)siginfo.si_status);
> >      	}
> >      }
> > 
> > Output with an x86_64 host and mips64el target before 1c3dfb506ea3
> > (incorrect: exit code 12 is translated like a signal):
> > 
> >      Code: 1, status: 17
> >      Code: 2, status: 17
> > 
> > After 1c3dfb506ea3 (incorrect: signal number is not translated):
> > 
> >      Code: 1, status: 12
> >      Code: 2, status: 12
> > 
> > With this patch:
> > 
> >      Code: 1, status: 12
> >      Code: 2, status: 17
> > 
> > Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
> > ---
> >   linux-user/signal.c | 7 ++++++-
> >   1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/linux-user/signal.c b/linux-user/signal.c
> > index 14d8fdfde152..8e3af98ec0a7 100644
> > --- a/linux-user/signal.c
> > +++ b/linux-user/signal.c
> > @@ -403,7 +403,12 @@ static inline void host_to_target_siginfo_noswap(target_siginfo_t *tinfo,
> >           case TARGET_SIGCHLD:
> >               tinfo->_sifields._sigchld._pid = info->si_pid;
> >               tinfo->_sifields._sigchld._uid = info->si_uid;
> > -            tinfo->_sifields._sigchld._status = info->si_status;
> > +            if (si_code == CLD_EXITED)
> > +                tinfo->_sifields._sigchld._status = info->si_status;
> > +            else
> > +                tinfo->_sifields._sigchld._status
> > +                    = host_to_target_signal(info->si_status & 0x7f)
> > +                        | (info->si_status & ~0x7f);
> >               tinfo->_sifields._sigchld._utime = info->si_utime;
> >               tinfo->_sifields._sigchld._stime = info->si_stime;
> >               si_type = QEMU_SI_CHLD;
> > 
> 
> 
> 


-- 
Andreas K. Hüttel
dilfridge@gentoo.org
Gentoo Linux developer
(council, toolchain, base-system, perl, libreoffice)

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 981 bytes --]

Re: [PATCH] linux-user/signal: Map exit signals in SIGCHLD siginfo_t

[Andreas K. Huettel]

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

Am Sonntag, 19. Dezember 2021, 00:32:09 CET schrieb Matthias Schiffer:
> On 23/10/2021 21:59, Matthias Schiffer wrote:
> > When converting a siginfo_t from waitid(), the interpretation of si_status
> > depends on the value of si_code: For CLD_EXITED, it is an exit code and
> > should be copied verbatim. For other codes, it is a signal number
> > (possibly with additional high bits from ptrace) that should be mapped.
> > 
> > This code was previously changed in commit 1c3dfb506ea3
> > ("linux-user/signal: Decode waitid si_code"), but the fix was
> > incomplete.
> 
> ping
> 

Sorry I can't say anything about this. 

The hangs that I experience seem to be unrelated to the patch (no improvement, but also no worsening).

-- 
Andreas K. Hüttel
dilfridge@gentoo.org
Gentoo Linux developer 
(council, qa, toolchain, base-system, perl, libreoffice)

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] linux-user/signal: Map exit signals in SIGCHLD siginfo_t

[Laurent Vivier]

Le 23/10/2021 à 21:59, Matthias Schiffer a écrit :
> When converting a siginfo_t from waitid(), the interpretation of si_status
> depends on the value of si_code: For CLD_EXITED, it is an exit code and
> should be copied verbatim. For other codes, it is a signal number
> (possibly with additional high bits from ptrace) that should be mapped.
> 
> This code was previously changed in commit 1c3dfb506ea3
> ("linux-user/signal: Decode waitid si_code"), but the fix was
> incomplete.
> 
> Tested with the following test program:
> 
>      #include <stdio.h>
>      #include <stdlib.h>
>      #include <unistd.h>
>      #include <sys/wait.h>
> 
>      int main() {
>      	pid_t pid = fork();
>      	if (pid == 0) {
>      		exit(12);
>      	} else {
>      		siginfo_t siginfo = {};
>      		waitid(P_PID, pid, &siginfo, WEXITED);
>      		printf("Code: %d, status: %d\n", (int)siginfo.si_code, (int)siginfo.si_status);
>      	}
> 
>      	pid = fork();
>      	if (pid == 0) {
>      		raise(SIGUSR2);
>      	} else {
>      		siginfo_t siginfo = {};
>      		waitid(P_PID, pid, &siginfo, WEXITED);
>      		printf("Code: %d, status: %d\n", (int)siginfo.si_code, (int)siginfo.si_status);
>      	}
>      }
> 
> Output with an x86_64 host and mips64el target before 1c3dfb506ea3
> (incorrect: exit code 12 is translated like a signal):
> 
>      Code: 1, status: 17
>      Code: 2, status: 17
> 
> After 1c3dfb506ea3 (incorrect: signal number is not translated):
> 
>      Code: 1, status: 12
>      Code: 2, status: 12
> 
> With this patch:
> 
>      Code: 1, status: 12
>      Code: 2, status: 17
> 
> Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
> ---
>   linux-user/signal.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index 14d8fdfde152..8e3af98ec0a7 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -403,7 +403,12 @@ static inline void host_to_target_siginfo_noswap(target_siginfo_t *tinfo,
>           case TARGET_SIGCHLD:
>               tinfo->_sifields._sigchld._pid = info->si_pid;
>               tinfo->_sifields._sigchld._uid = info->si_uid;
> -            tinfo->_sifields._sigchld._status = info->si_status;
> +            if (si_code == CLD_EXITED)
> +                tinfo->_sifields._sigchld._status = info->si_status;
> +            else
> +                tinfo->_sifields._sigchld._status
> +                    = host_to_target_signal(info->si_status & 0x7f)
> +                        | (info->si_status & ~0x7f);
>               tinfo->_sifields._sigchld._utime = info->si_utime;
>               tinfo->_sifields._sigchld._stime = info->si_stime;
>               si_type = QEMU_SI_CHLD;

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Re: [PATCH] linux-user/signal: Map exit signals in SIGCHLD siginfo_t

[Laurent Vivier]

Le 23/10/2021 à 21:59, Matthias Schiffer a écrit :
> When converting a siginfo_t from waitid(), the interpretation of si_status
> depends on the value of si_code: For CLD_EXITED, it is an exit code and
> should be copied verbatim. For other codes, it is a signal number
> (possibly with additional high bits from ptrace) that should be mapped.
> 
> This code was previously changed in commit 1c3dfb506ea3
> ("linux-user/signal: Decode waitid si_code"), but the fix was
> incomplete.
> 
> Tested with the following test program:
> 
>      #include <stdio.h>
>      #include <stdlib.h>
>      #include <unistd.h>
>      #include <sys/wait.h>
> 
>      int main() {
>      	pid_t pid = fork();
>      	if (pid == 0) {
>      		exit(12);
>      	} else {
>      		siginfo_t siginfo = {};
>      		waitid(P_PID, pid, &siginfo, WEXITED);
>      		printf("Code: %d, status: %d\n", (int)siginfo.si_code, (int)siginfo.si_status);
>      	}
> 
>      	pid = fork();
>      	if (pid == 0) {
>      		raise(SIGUSR2);
>      	} else {
>      		siginfo_t siginfo = {};
>      		waitid(P_PID, pid, &siginfo, WEXITED);
>      		printf("Code: %d, status: %d\n", (int)siginfo.si_code, (int)siginfo.si_status);
>      	}
>      }
> 
> Output with an x86_64 host and mips64el target before 1c3dfb506ea3
> (incorrect: exit code 12 is translated like a signal):
> 
>      Code: 1, status: 17
>      Code: 2, status: 17
> 
> After 1c3dfb506ea3 (incorrect: signal number is not translated):
> 
>      Code: 1, status: 12
>      Code: 2, status: 12
> 
> With this patch:
> 
>      Code: 1, status: 12
>      Code: 2, status: 17
> 
> Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
> ---
>   linux-user/signal.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index 14d8fdfde152..8e3af98ec0a7 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -403,7 +403,12 @@ static inline void host_to_target_siginfo_noswap(target_siginfo_t *tinfo,
>           case TARGET_SIGCHLD:
>               tinfo->_sifields._sigchld._pid = info->si_pid;
>               tinfo->_sifields._sigchld._uid = info->si_uid;
> -            tinfo->_sifields._sigchld._status = info->si_status;
> +            if (si_code == CLD_EXITED)
> +                tinfo->_sifields._sigchld._status = info->si_status;
> +            else
> +                tinfo->_sifields._sigchld._status
> +                    = host_to_target_signal(info->si_status & 0x7f)
> +                        | (info->si_status & ~0x7f);
>               tinfo->_sifields._sigchld._utime = info->si_utime;
>               tinfo->_sifields._sigchld._stime = info->si_stime;
>               si_type = QEMU_SI_CHLD;

Applied to my linux-user-for-7.0 branch.

Thanks,
Laurent