kill05: fix regression

PATCH In review Cristian Greco 3 participants 5 messages Mar 4, 2011 – Apr 26, 2011 Library code

lore

Share & tools

lore

Cristian Greco

author

2011-03-04 16:10 UTC #0 lore raw

PATCH 1/1 kill05: fix regression

Hi,

this should fix a regression in kill05.

Why did tst_resm/tst_exit get replaced by perror/exit in 84f181fd?

Signed-off-by Cristian Greco <[email protected]>

---
 testcases/kernel/syscalls/kill/kill05.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

(quoted diff) +0 -0

b/testcases/kernel/syscalls/kill/kill05.c index 0193100..ac5dc86 100644

testcases/kernel/syscalls/kill/kill05.c +2 -1

@@ -121,11 +121,12 @@ int main(int ac, char **av)
 		tst_brkm(TBROK, cleanup, "Fork failed");
 	else if (pid == 0)
 		do_master_child(av);
-
+  else {
 	if (waitpid(pid, &status, 0) == -1)
 		tst_resm(TBROK|TERRNO, "waitpid failed");
 	else if (!WIFEXITED(status) || WEXITSTATUS(status) != 0)
 		tst_resm(TFAIL, "child exited abnormally");
+  }
 	cleanup();
 	tst_exit();
 }

Signature · 7 lines

1.7.4.1


Thanks,
--
Cristian Greco
GPG key ID: 0xCF4D32E4

_______________________________________________
Ltp-list mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/ltp-list

Garrett Cooper

2011-03-04 16:29 UTC #1 lore raw

Quoted 5 lines · Cristian Greco

Hi,
this should fix a regression in kill05.
Why did tst_resm/tst_exit get replaced by perror/exit in 84f181fd?

Because it's a child process and child processes should _not_ use
libltp for sanity and to avoid introducing determinism, test bugs, and for cluttering up output (remember: you have to communicate to the user which process is actually doing the work, they're sharing file descriptors potentially, etc).
The proposed fix doesn't make sense though because do_master_child
always exits when the flow is executed correctly (i.e. there aren't any early return statements from do_master_child, etc. Is it not resuming the stack appropriately after it gets signaled? What's the call flow exactly for the failure?
Thanks,
-Garrett

------------------------------------------------------------------------------
What You Don't Know About Data Connectivity CAN Hurt You This paper provides an overview of data connectivity, details its effect on application quality, and explores various alternative solutions. http://p.sf.net/sfu/progress-d2d
_______________________________________________
Ltp-list mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/ltp-list

Garrett Cooper

2011-03-04 16:30 UTC #2 lore raw

Quoted 9 lines · Garrett Cooper

Quoted 5 lines · Cristian Greco
Hi,
this should fix a regression in kill05.
Why did tst_resm/tst_exit get replaced by perror/exit in 84f181fd?
Because it's a child process and child processes should _not_ use libltp for sanity and to avoid introducing determinism, test bugs, and

s/determinism/non-determinism/

Quoted 11 lines

for cluttering up output (remember: you have to communicate to the user which process is actually doing the work, they're sharing file descriptors potentially, etc).
The proposed fix doesn't make sense though because do_master_child always exits when the flow is executed correctly (i.e. there aren't any early return statements from do_master_child, etc. Is it not resuming the stack appropriately after it gets signaled? What's the call flow exactly for the failure?
Thanks,
-Garrett

Cristian Greco

author

2011-03-05 00:05 UTC #3 lore raw

Quoted 4 lines · On Fri, 4 Mar 2011 08:30:14 -0800 Garrett Cooper <[email protected]> wrote:

Quoted 3 lines · Garrett Cooper
Quoted 2 lines · Cristian Greco
Hi,

Quoted 20 lines

Quoted 6 lines
Quoted 3 lines
this should fix a regression in kill05.
Why did tst_resm/tst_exit get replaced by perror/exit in 84f181fd?
Because it's a child process and child processes should _not_ use libltp for sanity and to avoid introducing determinism, test bugs, and
s/determinism/non-determinism/
Quoted 11 lines
for cluttering up output (remember: you have to communicate to the user which process is actually doing the work, they're sharing file descriptors potentially, etc).
The proposed fix doesn't make sense though because do_master_child always exits when the flow is executed correctly (i.e. there aren't any early return statements from do_master_child, etc. Is it not resuming the stack appropriately after it gets signaled? What's the call flow exactly for the failure?
Thanks,
-Garrett

Hi Garrett,

you're right. In fact, the problem is in safe_getpwnam(), which does not correctly reproduce the behavior of the deprecated my_getpwnam(). Moreover, I'm not noticing the problem on my laptop due to a race condition in kill05, while it shows an inconsistent behavior on the box I'm running tests on (mipsel with glibc, for the sake).

Could you please fix safe_getpwnam() with either one of the following patches?

lib/safe_macros.c +7 -2

@@ -98,13 +98,18 @@ struct passwd*
 safe_getpwnam(const char *file, const int lineno, void (*cleanup_fn)(void),
     const char *name)
 {
+       struct passwd *pwd;
        struct passwd *rval;
 
-       rval = getpwnam(name);
-       if (rval == NULL)
+       pwd = getpwnam(name);
+       if (pwd == NULL)
                tst_brkm(TBROK|TERRNO, cleanup_fn, "getpwnam failed at %s:%d",
                    file, lineno);
 
+       rval = (struct passwd *) malloc(sizeof(struct passwd));
+       memset(rval, 0, sizeof(struct passwd));
+       memcpy(rval, pwd, sizeof(struct passwd));
+
        return (rval);
 }

This second patch is a bit more invasive, using getpwnam_r() to correctly retrieve the strings which getpwnam() will store in static buffers (e.g. pw_name in kill05 is 'bin' for both the parent and the child, while their UID is still different).

lib/safe_macros.c +18 -1

@@ -98,9 +98,26 @@ struct passwd*
 safe_getpwnam(const char *file, const int lineno, void (*cleanup_fn)(void),
     const char *name)
 {
+       struct passwd *pwd;
        struct passwd *rval;
+       char *buf;
+       size_t bufsize;
 
-       rval = getpwnam(name);
+       bufsize = sysconf(_SC_GETPW_R_SIZE_MAX);
+       if (bufsize == -1)
+               bufsize = 16384;
+
+       buf = malloc(bufsize);
+       if (buf == NULL)
+               tst_brkm(TBROK|TERRNO, cleanup_fn, "malloc failed at %s:%d",
+                   file, lineno);
+
+       pwd = malloc(sizeof(struct passwd));
+       if (buf == NULL)
+               tst_brkm(TBROK|TERRNO, cleanup_fn, "malloc failed at %s:%d",
+                   file, lineno);
+
+       getpwnam_r(name, pwd, buf, bufsize, &rval);
        if (rval == NULL)
                tst_brkm(TBROK|TERRNO, cleanup_fn, "getpwnam failed at %s:%d",
                    file, lineno);

On the other hand, the race condition in kill05 should be fixed with something like this (sorry, please double-check, I have not so much experience with pthreads):

testcases/kernel/syscalls/kill/kill05.c +24 -9

@@ -77,6 +77,7 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <unistd.h>
+#include <pthread.h>
 
 #include "test.h"
 #include "usctest.h"

@@ -101,6 +102,9 @@ extern int getipckey();
 
 #define TEST_SIG SIGKILL
 
+pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER;
+pthread_cond_t cond = PTHREAD_COND_INITIALIZER;
+
 int main(int ac, char **av)
 {
        char *msg;              /* message returned from parse_opts */

@@ -169,6 +173,11 @@ void do_master_child(char **av)
                        exit(1);
                }
 #else
+               pthread_mutex_lock(&mutex);
+               *flag = 1;
+               pthread_cond_signal(&cond);
+               pthread_mutex_unlock(&mutex);
+
                do_child();
 #endif
        }

@@ -177,10 +186,19 @@ void do_master_child(char **av)
                exit(1);
        }
 
+       pthread_mutex_lock(&mutex);
+       while (*flag == 0)
+               pthread_cond_wait(&cond, &mutex);
+       pthread_mutex_unlock(&mutex);
+
        TEST(kill(pid1, TEST_SIG));
 
        /* signal the child that we're done */
+       pthread_mutex_lock(&mutex);
        *flag = 1;
+       pthread_cond_signal(&cond);
+       pthread_mutex_unlock(&mutex);
+
 
        if (waitpid(pid1, &status, 0) == -1) {
                perror("waitpid failed");

@@ -206,15 +224,12 @@ void do_master_child(char **av)
 
 void do_child()
 {
-       pid_t my_pid;
-
-       my_pid = getpid();
-       while (1) {
-               if (*flag == 1)
-                       exit(0);
-               else
-                       sleep(1);
-       }
+       pthread_mutex_lock(&mutex);
+       while (*flag == 0)
+               pthread_cond_wait(&cond, &mutex);
+       pthread_mutex_unlock(&mutex);
+
+       exit(0);
 }
 
 void setup(void)

Thanks,
--
Cristian Greco
GPG key ID: 0xCF4D32E4

_______________________________________________
Ltp-list mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/ltp-list

Cyril Hrubis

2011-04-26 15:33 UTC #4 lore raw

Hi!

Quoted 8 lines

you're right. In fact, the problem is in safe_getpwnam(), which does not correctly reproduce the behavior of the deprecated my_getpwnam(). Moreover, I'm not noticing the problem on my laptop due to a race condition in kill05, while it shows an inconsistent behavior on the box I'm running tests on (mipsel with glibc, for the sake).
Could you please fix safe_getpwnam() with either one of the following patches?

Technically you are right, but my_getpwnam() behaviour is broken too. As the strings the structure is pointing to are statically allocated too, this copying preserves only uid and gid. So instead of creating crippled interface we will rather fix the test itself.

Thanks the report anyway.

Signature · 13 lines

Cyril Hrubis
[email protected]

------------------------------------------------------------------------------
WhatsUp Gold - Download Free Network Management Software
The most intuitive, comprehensive, and cost-effective network 
management toolset available today.  Delivers lowest initial 
acquisition cost and overall TCO of any competing solution.
http://p.sf.net/sfu/whatsupgold-sd
_______________________________________________
Ltp-list mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/ltp-list