diff mbox series

[v2,3/4] linux-user: fix TARGET_NSIG and _NSIG uses

Message ID 20200204171053.1718013-4-laurent@vivier.eu
State New
Headers show
Series linux-user: fix use of SIGRTMIN | expand

Commit Message

Laurent Vivier Feb. 4, 2020, 5:10 p.m. UTC 
Valid signal numbers are between 1 (SIGHUP) and SIGRTMAX.

System includes define _NSIG to SIGRTMAX + 1, but
QEMU (like kernel) defines TARGET_NSIG to TARGET_SIGRTMAX.

Fix all the checks involving the signal range.

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

Notes:
    v2: replace i, j by target_sig, host_sig

 linux-user/signal.c | 52 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 37 insertions(+), 15 deletions(-)

Comments

Peter Maydell Feb. 11, 2020, 4:47 p.m. UTC | #1 
On Tue, 4 Feb 2020 at 17:11, Laurent Vivier <laurent@vivier.eu> wrote:
>
> Valid signal numbers are between 1 (SIGHUP) and SIGRTMAX.
>
> System includes define _NSIG to SIGRTMAX + 1, but
> QEMU (like kernel) defines TARGET_NSIG to TARGET_SIGRTMAX.
>
> Fix all the checks involving the signal range.
>
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> ---
>
> Notes:
>     v2: replace i, j by target_sig, host_sig
>
>  linux-user/signal.c | 52 ++++++++++++++++++++++++++++++++-------------
>  1 file changed, 37 insertions(+), 15 deletions(-)
>
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index 246315571c09..c1e664f97a7c 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -30,6 +30,15 @@ static struct target_sigaction sigact_table[TARGET_NSIG];

Optional follow-on patch: make sigact_table[] also size
TARGET_NSIG + 1, for consistency with target_to_host_signal_table[],
and remove all the "- 1"s when we index into it.


> @@ -492,10 +514,10 @@ static void signal_table_init(void)
>          if (host_to_target_signal_table[host_sig] == 0) {
>              host_to_target_signal_table[host_sig] = host_sig;
>          }
> -    }
> -    for (host_sig = 1; host_sig < _NSIG; host_sig++) {
>          target_sig = host_to_target_signal_table[host_sig];
> -        target_to_host_signal_table[target_sig] = host_sig;
> +        if (target_sig <= TARGET_NSIG) {
> +            target_to_host_signal_table[target_sig] = host_sig;
> +        }

Why does this hunk apparently delete the for() line ?

Why do we need the if() -- surely there should never be any
entries in host_to_target_signal_table[] that aren't
valid target signal numbers ?

>      }
>  }
>
> @@ -518,7 +540,7 @@ void signal_init(void)
>      act.sa_sigaction = host_signal_handler;
>      for(i = 1; i <= TARGET_NSIG; i++) {
>  #ifdef TARGET_GPROF
> -        if (i == SIGPROF) {
> +        if (i == TARGET_SIGPROF) {
>              continue;
>          }
>  #endif
> --

thanks
-- PMM
Laurent Vivier Feb. 11, 2020, 4:59 p.m. UTC | #2 
Le 11/02/2020 à 17:47, Peter Maydell a écrit :
> On Tue, 4 Feb 2020 at 17:11, Laurent Vivier <laurent@vivier.eu> wrote:
>>
>> Valid signal numbers are between 1 (SIGHUP) and SIGRTMAX.
>>
>> System includes define _NSIG to SIGRTMAX + 1, but
>> QEMU (like kernel) defines TARGET_NSIG to TARGET_SIGRTMAX.
>>
>> Fix all the checks involving the signal range.
>>
>> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
>> ---
>>
>> Notes:
>>     v2: replace i, j by target_sig, host_sig
>>
>>  linux-user/signal.c | 52 ++++++++++++++++++++++++++++++++-------------
>>  1 file changed, 37 insertions(+), 15 deletions(-)
>>
>> diff --git a/linux-user/signal.c b/linux-user/signal.c
>> index 246315571c09..c1e664f97a7c 100644
>> --- a/linux-user/signal.c
>> +++ b/linux-user/signal.c
>> @@ -30,6 +30,15 @@ static struct target_sigaction sigact_table[TARGET_NSIG];
> 
> Optional follow-on patch: make sigact_table[] also size
> TARGET_NSIG + 1, for consistency with target_to_host_signal_table[],
> and remove all the "- 1"s when we index into it.
> 

OK,

>> @@ -492,10 +514,10 @@ static void signal_table_init(void)
>>          if (host_to_target_signal_table[host_sig] == 0) {
>>              host_to_target_signal_table[host_sig] = host_sig;
>>          }
>> -    }
>> -    for (host_sig = 1; host_sig < _NSIG; host_sig++) {
>>          target_sig = host_to_target_signal_table[host_sig];
>> -        target_to_host_signal_table[target_sig] = host_sig;
>> +        if (target_sig <= TARGET_NSIG) {
>> +            target_to_host_signal_table[target_sig] = host_sig;
>> +        }
> 
> Why does this hunk apparently delete the for() line ?

It effectively deletes the for() line because I merge the two "for
(host_sig = 1; host_sig < _NSIG; host_sig++)" loops into one.

> Why do we need the if() -- surely there should never be any
> entries in host_to_target_signal_table[] that aren't
> valid target signal numbers ?
> 

we have above the "host_to_target_signal_table[host_sig] = host_sig;"
and host_sig can be greater than TARGET_NSIG.

Setting like this allows to ignore them later in the target as we can
compare them to TARGET_NSIG. This mapping 1:1 in the default case is the
original behaviour.

Thanks,
Laurent
Peter Maydell Feb. 11, 2020, 5:17 p.m. UTC | #3 
On Tue, 11 Feb 2020 at 16:59, Laurent Vivier <laurent@vivier.eu> wrote:
>
> Le 11/02/2020 à 17:47, Peter Maydell a écrit :
> > On Tue, 4 Feb 2020 at 17:11, Laurent Vivier <laurent@vivier.eu> wrote:
> >>
> >> Valid signal numbers are between 1 (SIGHUP) and SIGRTMAX.
> >>
> >> System includes define _NSIG to SIGRTMAX + 1, but
> >> QEMU (like kernel) defines TARGET_NSIG to TARGET_SIGRTMAX.
> >>
> >> Fix all the checks involving the signal range.
> >>
> >> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> >> ---
> >>
> >> Notes:
> >>     v2: replace i, j by target_sig, host_sig
> >>
> >>  linux-user/signal.c | 52 ++++++++++++++++++++++++++++++++-------------
> >>  1 file changed, 37 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/linux-user/signal.c b/linux-user/signal.c
> >> index 246315571c09..c1e664f97a7c 100644
> >> --- a/linux-user/signal.c
> >> +++ b/linux-user/signal.c
> >> @@ -30,6 +30,15 @@ static struct target_sigaction sigact_table[TARGET_NSIG];
> >
> > Optional follow-on patch: make sigact_table[] also size
> > TARGET_NSIG + 1, for consistency with target_to_host_signal_table[],
> > and remove all the "- 1"s when we index into it.
> >
>
> OK,
>
> >> @@ -492,10 +514,10 @@ static void signal_table_init(void)
> >>          if (host_to_target_signal_table[host_sig] == 0) {
> >>              host_to_target_signal_table[host_sig] = host_sig;
> >>          }
> >> -    }
> >> -    for (host_sig = 1; host_sig < _NSIG; host_sig++) {
> >>          target_sig = host_to_target_signal_table[host_sig];
> >> -        target_to_host_signal_table[target_sig] = host_sig;
> >> +        if (target_sig <= TARGET_NSIG) {
> >> +            target_to_host_signal_table[target_sig] = host_sig;
> >> +        }
> >
> > Why does this hunk apparently delete the for() line ?
>
> It effectively deletes the for() line because I merge the two "for
> (host_sig = 1; host_sig < _NSIG; host_sig++)" loops into one.

Oh, I see, I missed the closing brace being deleted.

> > Why do we need the if() -- surely there should never be any
> > entries in host_to_target_signal_table[] that aren't
> > valid target signal numbers ?
> >
>
> we have above the "host_to_target_signal_table[host_sig] = host_sig;"
> and host_sig can be greater than TARGET_NSIG.
>
> Setting like this allows to ignore them later in the target as we can
> compare them to TARGET_NSIG. This mapping 1:1 in the default case is the
> original behaviour.

I guess so (I was sort of expecting us to do the filter on
"is this valid" when we filled the array, rather than having
to do it every time we used the entries, but this works).

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM
diff mbox series

Patch

diff --git a/linux-user/signal.c b/linux-user/signal.c
index 246315571c09..c1e664f97a7c 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -30,6 +30,15 @@  static struct target_sigaction sigact_table[TARGET_NSIG];
 static void host_signal_handler(int host_signum, siginfo_t *info,
                                 void *puc);
 
+
+/*
+ * System includes define _NSIG as SIGRTMAX + 1,
+ * but qemu (like the kernel) defines TARGET_NSIG as TARGET_SIGRTMAX
+ * and the first signal is SIGHUP defined as 1
+ * Signal number 0 is reserved for use as kill(pid, 0), to test whether
+ * a process exists without sending it a signal.
+ */
+QEMU_BUILD_BUG_ON(__SIGRTMAX + 1 != _NSIG);
 static uint8_t host_to_target_signal_table[_NSIG] = {
     [SIGHUP] = TARGET_SIGHUP,
     [SIGINT] = TARGET_SIGINT,
@@ -67,19 +76,24 @@  static uint8_t host_to_target_signal_table[_NSIG] = {
     [SIGSYS] = TARGET_SIGSYS,
     /* next signals stay the same */
 };
-static uint8_t target_to_host_signal_table[_NSIG];
 
+static uint8_t target_to_host_signal_table[TARGET_NSIG + 1];
+
+/* valid sig is between 1 and _NSIG - 1 */
 int host_to_target_signal(int sig)
 {
-    if (sig < 0 || sig >= _NSIG)
+    if (sig < 1 || sig >= _NSIG) {
         return sig;
+    }
     return host_to_target_signal_table[sig];
 }
 
+/* valid sig is between 1 and TARGET_NSIG */
 int target_to_host_signal(int sig)
 {
-    if (sig < 0 || sig >= _NSIG)
+    if (sig < 1 || sig > TARGET_NSIG) {
         return sig;
+    }
     return target_to_host_signal_table[sig];
 }
 
@@ -100,11 +114,15 @@  static inline int target_sigismember(const target_sigset_t *set, int signum)
 void host_to_target_sigset_internal(target_sigset_t *d,
                                     const sigset_t *s)
 {
-    int i;
+    int host_sig, target_sig;
     target_sigemptyset(d);
-    for (i = 1; i <= TARGET_NSIG; i++) {
-        if (sigismember(s, i)) {
-            target_sigaddset(d, host_to_target_signal(i));
+    for (host_sig = 1; host_sig < _NSIG; host_sig++) {
+        target_sig = host_to_target_signal(host_sig);
+        if (target_sig < 1 || target_sig > TARGET_NSIG) {
+            continue;
+        }
+        if (sigismember(s, host_sig)) {
+            target_sigaddset(d, target_sig);
         }
     }
 }
@@ -122,11 +140,15 @@  void host_to_target_sigset(target_sigset_t *d, const sigset_t *s)
 void target_to_host_sigset_internal(sigset_t *d,
                                     const target_sigset_t *s)
 {
-    int i;
+    int host_sig, target_sig;
     sigemptyset(d);
-    for (i = 1; i <= TARGET_NSIG; i++) {
-        if (target_sigismember(s, i)) {
-            sigaddset(d, target_to_host_signal(i));
+    for (target_sig = 1; target_sig <= TARGET_NSIG; target_sig++) {
+        host_sig = target_to_host_signal(target_sig);
+        if (host_sig < 1 || host_sig >= _NSIG) {
+            continue;
+        }
+        if (target_sigismember(s, target_sig)) {
+            sigaddset(d, host_sig);
         }
     }
 }
@@ -492,10 +514,10 @@  static void signal_table_init(void)
         if (host_to_target_signal_table[host_sig] == 0) {
             host_to_target_signal_table[host_sig] = host_sig;
         }
-    }
-    for (host_sig = 1; host_sig < _NSIG; host_sig++) {
         target_sig = host_to_target_signal_table[host_sig];
-        target_to_host_signal_table[target_sig] = host_sig;
+        if (target_sig <= TARGET_NSIG) {
+            target_to_host_signal_table[target_sig] = host_sig;
+        }
     }
 }
 
@@ -518,7 +540,7 @@  void signal_init(void)
     act.sa_sigaction = host_signal_handler;
     for(i = 1; i <= TARGET_NSIG; i++) {
 #ifdef TARGET_GPROF
-        if (i == SIGPROF) {
+        if (i == TARGET_SIGPROF) {
             continue;
         }
 #endif