diff mbox

[TRIVIAL,v2] Bad zero comparison for sas_ss_flags on powerpc

Message ID CAFKAgTcgMqVpopxTw8+8N005OSOoNM33L2a8DLJvn7Tpa-BGvQ@mail.gmail.com
State New
Headers show

Commit Message

Alex Barcelo Feb. 10, 2012, 9:55 a.m. UTC 
This is v2 of the patch "sas_ss_flags bug for powerpc", which had a
horrible name and no description.

All architectures work the same way, and all check for sas_ss_flags ==
0. The powerpc lines are wrong, and do the check the other way round
(it's a qemu internal check, which is done wrong only for this
architecture, it's more a typo than a bug). It's NOT ppc specific,
it's POSIX standard (sigaltstack) and qemu internal.

I have a test source that I will send in a follow-up (it's longer than
I would have wished, I'm sure that a better test case can be written
if needed)

Signed-off-by: Alex Barcelo <abarcelo@ac.upc.edu>
---
 linux-user/signal.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

    }
--
1.7.5.4

Comments

Alex Barcelo Feb. 10, 2012, 9:57 a.m. UTC | #1 
// Test source and desired /real output:

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

void handler(int sig)
{
   unsigned int a;
   // to prevent uninitialized stack, normally a = 0
   if ( a>10 ) a = 0;
   a = a + 1;
   printf ("new value: %d\n" , a );
   if (a > 7) _exit(a);
   return;
}

int main()
{
   int ret;
   char * stackA = malloc(SIGSTKSZ);
   char * stackB = malloc(SIGSTKSZ);
   stack_t ssA = {
       .ss_size = SIGSTKSZ,
       .ss_sp = stackA,
   };
   stack_t ssB = {
       .ss_size = SIGSTKSZ,
       .ss_sp = stackB,
   };
   struct sigaction sa = {
       .sa_handler = handler,
       .sa_flags = SA_ONSTACK
   };

   // no error checking, only debug output
   ret = sigfillset(&sa.sa_mask);
   printf ( "Sigfillset: %d\n" , ret );
   ret = sigaction(SIGUSR1, &sa, 0);
   printf ( "Sigaction: %d\n" , ret );

   while (1) {
       printf ("On stack A -- " );
       ret = sigaltstack(&ssA, 0);
       printf ( "sigaltstack return: %d -- " , ret );
       kill(0, SIGUSR1);
       sleep(1);
       printf ("                                    -- " );
       kill(0, SIGUSR1);
       sleep(1);

       printf ("On stack B -- " );
       ret = sigaltstack(&ssB, 0);
       printf ( "sigaltstack return: %d -- " , ret );
       kill(0, SIGUSR1);
       sleep(1);
   }
}

/* Desired output:
Sigfillset: 0
Sigaction: 0
On stack A -- sigaltstack return: 0 -- new value: 1
                                   -- new value: 2
On stack B -- sigaltstack return: 0 -- new value: 1
On stack A -- sigaltstack return: 0 -- new value: 3
                                   -- new value: 4
On stack B -- sigaltstack return: 0 -- new value: 2
On stack A -- sigaltstack return: 0 -- new value: 5
                                   -- new value: 6
On stack B -- sigaltstack return: 0 -- new value: 3
On stack A -- sigaltstack return: 0 -- new value: 7
                                   -- new value: 8

Output for ppc without patch:
Sigfillset: 0
Sigaction: 0
On stack A -- sigaltstack return: 0 -- new value: 1
                                   -- new value: 2
On stack B -- sigaltstack return: 0 -- new value: 3 // WRONG!!
On stack A -- sigaltstack return: 0 -- new value: 4
                                   -- new value: 5 // WRONG AGAIN!
...
*/
Alex Barcelo Feb. 15, 2012, 6:55 a.m. UTC | #2 
ping? is this ok? Or it is not trivial at all, and better do a normal
patch? And write it better?

I have no inconvenience on doing so, but I didn't want to duplicate patches
in the list.
El 10/02/2012 10:57, "Alex Barcelo" <abarcelo@ac.upc.edu> escribió:

> // Test source and desired /real output:
>
> #include <signal.h>
> #include <unistd.h>
> #include <stdio.h>
> #include <stdlib.h>
>
> void handler(int sig)
> {
>   unsigned int a;
>   // to prevent uninitialized stack, normally a = 0
>   if ( a>10 ) a = 0;
>   a = a + 1;
>   printf ("new value: %d\n" , a );
>   if (a > 7) _exit(a);
>   return;
> }
>
> int main()
> {
>   int ret;
>   char * stackA = malloc(SIGSTKSZ);
>   char * stackB = malloc(SIGSTKSZ);
>   stack_t ssA = {
>       .ss_size = SIGSTKSZ,
>       .ss_sp = stackA,
>   };
>   stack_t ssB = {
>       .ss_size = SIGSTKSZ,
>       .ss_sp = stackB,
>   };
>   struct sigaction sa = {
>       .sa_handler = handler,
>       .sa_flags = SA_ONSTACK
>   };
>
>   // no error checking, only debug output
>   ret = sigfillset(&sa.sa_mask);
>   printf ( "Sigfillset: %d\n" , ret );
>   ret = sigaction(SIGUSR1, &sa, 0);
>   printf ( "Sigaction: %d\n" , ret );
>
>   while (1) {
>       printf ("On stack A -- " );
>       ret = sigaltstack(&ssA, 0);
>       printf ( "sigaltstack return: %d -- " , ret );
>       kill(0, SIGUSR1);
>       sleep(1);
>       printf ("                                    -- " );
>       kill(0, SIGUSR1);
>       sleep(1);
>
>       printf ("On stack B -- " );
>       ret = sigaltstack(&ssB, 0);
>       printf ( "sigaltstack return: %d -- " , ret );
>       kill(0, SIGUSR1);
>       sleep(1);
>   }
> }
>
> /* Desired output:
> Sigfillset: 0
> Sigaction: 0
> On stack A -- sigaltstack return: 0 -- new value: 1
>                                   -- new value: 2
> On stack B -- sigaltstack return: 0 -- new value: 1
> On stack A -- sigaltstack return: 0 -- new value: 3
>                                   -- new value: 4
> On stack B -- sigaltstack return: 0 -- new value: 2
> On stack A -- sigaltstack return: 0 -- new value: 5
>                                   -- new value: 6
> On stack B -- sigaltstack return: 0 -- new value: 3
> On stack A -- sigaltstack return: 0 -- new value: 7
>                                   -- new value: 8
>
> Output for ppc without patch:
> Sigfillset: 0
> Sigaction: 0
> On stack A -- sigaltstack return: 0 -- new value: 1
>                                   -- new value: 2
> On stack B -- sigaltstack return: 0 -- new value: 3 // WRONG!!
> On stack A -- sigaltstack return: 0 -- new value: 4
>                                   -- new value: 5 // WRONG AGAIN!
> ...
> */
>
Alexander Graf Feb. 15, 2012, 8:35 a.m. UTC | #3 
On 15.02.2012, at 07:55, Alex Barcelo wrote:

> ping? is this ok? Or it is not trivial at all, and better do a normal patch? And write it better?

The patch is fine, but it's not trivial. It's a ppc patch, hence it should've gotten CC'ed to qemu-ppc :). Also, whatever mailer you used to send the patch line wrapped it:

agraf@lychee:/home/agraf/release/qemu> git pw am 140538
git coERROR: patch seems to be corrupt (line wrapped?)
#40: FILE: linux-user/signal.c:4114:
target_sigaction *ka,

total: 1 errors, 0 warnings, 9 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

I'll fix it up and apply it to ppc-next.


Thanks!

Alex
Alex Barcelo Feb. 15, 2012, 1 p.m. UTC | #4 
On Wed, Feb 15, 2012 at 09:35, Alexander Graf <agraf@suse.de> wrote:
> On 15.02.2012, at 07:55, Alex Barcelo wrote:
>> ping? is this ok? Or it is not trivial at all, and better do a normal patch? And write it better?
>
> The patch is fine, but it's not trivial. It's a ppc patch, hence it should've gotten CC'ed
> to qemu-ppc :). Also, whatever mailer you used to send the patch line wrapped it:
> ...
>
> I'll fix it up and apply it to ppc-next.

Sorry for the inconvenience, and thanks a lot!
diff mbox

Patch

diff --git a/linux-user/signal.c b/linux-user/signal.c
index 79a39dc..26e0530 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -4115,7 +4115,7 @@  static target_ulong get_sigframe(struct
target_sigaction *ka,
    oldsp = env->gpr[1];

    if ((ka->sa_flags & TARGET_SA_ONSTACK) &&
-        (sas_ss_flags(oldsp))) {
+        (sas_ss_flags(oldsp)) == 0) {
        oldsp = (target_sigaltstack_used.ss_sp
                 + target_sigaltstack_used.ss_size);