Re: [LTP] [PATCH] kill05: fix regression

[Cristian Greco <cristian@regolo.cc>]

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

On Fri, 4 Mar 2011 08:30:14 -0800
Garrett Cooper <yanegomi@gmail.com> wrote:

> On Fri, Mar 4, 2011 at 8:29 AM, Garrett Cooper <yanegomi@gmail.com> wrote:
> > On Fri, Mar 4, 2011 at 8:09 AM, Cristian Greco <cristian@regolo.cc> wrote:
> >> 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/
> 
> > 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?

--- a/lib/safe_macros.c
+++ b/lib/safe_macros.c
@@ -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).

--- a/lib/safe_macros.c
+++ b/lib/safe_macros.c
@@ -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):

--- a/testcases/kernel/syscalls/kill/kill05.c
+++ b/testcases/kernel/syscalls/kill/kill05.c
@@ -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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 309 bytes --]

------------------------------------------------------------------------------
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

[-- Attachment #3: Type: text/plain, Size: 155 bytes --]

_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list