Re: [LTP] [PATCH v3 0/2] mount03: Convert to new API

[Petr Vorel <pvorel@suse.cz>]

Hi Xu,

...
> > diff --git testcases/kernel/syscalls/mount/mount03.c testcases/kernel/syscalls/mount/mount03.c
> > index 74b018d78..9c58783d7 100644
> > --- testcases/kernel/syscalls/mount/mount03.c
> > +++ testcases/kernel/syscalls/mount/mount03.c
> > @@ -15,7 +15,6 @@
> >   #include <sys/types.h>
> >   #include <sys/wait.h>
> >   #include <pwd.h>
> > -#include "old_resource.h"
> >   #include "tst_test.h"
> >   #include "lapi/mount.h"

> > @@ -145,7 +144,7 @@ static void setup(void)
> >   	nobody_gid = ltpuser->pw_gid;

> >   	snprintf(file, PATH_MAX, "%s/%s", MNTPOINT, TESTBIN);
> > -	TST_RESOURCE_COPY(NULL, TESTBIN, file);
> > +	SAFE_CP(TESTBIN, file);

> I still think we should test nosuid behaviour on different filesystem 
> like other test function because we have expand it to all filesystems.

> Also include tmpfs, so SAFE_CP should be in test_nosuid function 
> otherwise may hit ENOENT problem.

Ah thx, good idea. I guess the point of the setup was to run copy only once, but
your points are obviously valid.

I didn't notice it before because I overlooked SAFE_EXECLP() in test_nosuid() it
had parameter TESTBIN, thus not being run from mountpoint.

nit: I suggest to move to SAFE_EXECL() as it expect path, not filename as it's
not using PATH. Similarly we could change execlp() to execl() in test_noexec(),
but I'd prefer to keep execlp(), so that we test two different libc wrappers.

> different code as below:

> [root@localhost mount]# git diff .
> diff --git a/testcases/kernel/syscalls/mount/mount03.c 
> b/testcases/kernel/syscalls/mount/mount03.c
> index 74b018d78..b0582c76b 100644
> --- a/testcases/kernel/syscalls/mount/mount03.c
> +++ b/testcases/kernel/syscalls/mount/mount03.c
> @@ -21,6 +21,7 @@

>   #define MNTPOINT        "mntpoint"
>   #define TESTBIN        "mount03_setuid_test"
> +#define BIN_PATH           MNTPOINT"/"TESTBIN
+1 for avoid the need of snprintf when there are 2 constants.
NOTE: we can separate 3 items with spaces:
#define BIN_PATH	MNTPOINT "/" TESTBIN
But I'd rename it to TESTBIN_PATH.
Or maybe even better to use just "TEST":
#define TEST        "mount03_setuid_test"
#define TEST_PATH	MNTPOINT "/" TEST

>   #define TEST_STR "abcdefghijklmnopqrstuvwxyz"
>   #define FILE_MODE      0644
>   #define SUID_MODE      0511
> @@ -75,12 +76,19 @@ static void test_nosuid(void)
>   {
>          pid_t pid;
>          int status;
> +       struct stat st;
> +
> +       snprintf(file, PATH_MAX, "%s/%s", MNTPOINT, TESTBIN);
this is not needed when we have BIN_PATH
> +       SAFE_CP(TESTBIN, file);
SAFE_CP(TESTBIN, BIN_PATH);
> +       SAFE_STAT(file, &st);
> +       if (st.st_mode != SUID_MODE)
> +               SAFE_CHMOD(file, SUID_MODE);
	SAFE_CHMOD(BIN_PATH, SUID_MODE);

>          pid = SAFE_FORK();
>          if (!pid) {
>                  SAFE_SETGID(nobody_gid);
>                  SAFE_SETREUID(-1, nobody_uid);
> -               SAFE_EXECLP(TESTBIN, TESTBIN, NULL);
> +               SAFE_EXECLP(BIN_PATH, TESTBIN, NULL);
>          }

>          SAFE_WAITPID(pid, &status, 0);
> @@ -138,18 +146,10 @@ static struct tcase {

>   static void setup(void)
>   {
> -       struct stat st;
>          struct passwd *ltpuser = SAFE_GETPWNAM("nobody");

>          nobody_uid = ltpuser->pw_uid;
>          nobody_gid = ltpuser->pw_gid;
> -
> -       snprintf(file, PATH_MAX, "%s/%s", MNTPOINT, TESTBIN);
> -       TST_RESOURCE_COPY(NULL, TESTBIN, file);
> -
> -       SAFE_STAT(file, &st);
> -       if (st.st_mode != SUID_MODE)
> -           SAFE_CHMOD(file, SUID_MODE);
>   }

>   static void cleanup(void)

Final diff is below, but for readability it's temporarily also on my fork:
https://github.com/pevik/ltp/blob/57ba1ba47987a201c39764b4259a15aa39db9d2e/testcases/kernel/syscalls/mount/mount03.c

Kind regards,
Petr

> Best Regards
> Yang Xu

diff --git testcases/kernel/syscalls/mount/mount03.c testcases/kernel/syscalls/mount/mount03.c
index 9c58783d7..eef2a65c6 100644
--- testcases/kernel/syscalls/mount/mount03.c
+++ testcases/kernel/syscalls/mount/mount03.c
@@ -18,8 +18,9 @@
 #include "tst_test.h"
 #include "lapi/mount.h"
 
-#define MNTPOINT        "mntpoint"
+#define MNTPOINT	"mntpoint"
 #define TESTBIN	"mount03_setuid_test"
+#define BIN_PATH	MNTPOINT "/" TESTBIN
 #define TEST_STR "abcdefghijklmnopqrstuvwxyz"
 #define FILE_MODE	0644
 #define SUID_MODE	0511
@@ -74,12 +75,18 @@ static void test_nosuid(void)
 {
 	pid_t pid;
 	int status;
+	struct stat st;
+
+	SAFE_CP(TESTBIN, BIN_PATH);
+	SAFE_STAT(BIN_PATH, &st);
+	if (st.st_mode != SUID_MODE)
+		SAFE_CHMOD(BIN_PATH, SUID_MODE);
 
 	pid = SAFE_FORK();
 	if (!pid) {
 		SAFE_SETGID(nobody_gid);
 		SAFE_SETREUID(-1, nobody_uid);
-		SAFE_EXECLP(TESTBIN, TESTBIN, NULL);
+		SAFE_EXECL(BIN_PATH, TESTBIN, NULL);
 	}
 
 	SAFE_WAITPID(pid, &status, 0);
@@ -137,18 +144,10 @@ static struct tcase {
 
 static void setup(void)
 {
-	struct stat st;
 	struct passwd *ltpuser = SAFE_GETPWNAM("nobody");
 
 	nobody_uid = ltpuser->pw_uid;
 	nobody_gid = ltpuser->pw_gid;
-
-	snprintf(file, PATH_MAX, "%s/%s", MNTPOINT, TESTBIN);
-	SAFE_CP(TESTBIN, file);
-
-	SAFE_STAT(file, &st);
-	if (st.st_mode != SUID_MODE)
-	    SAFE_CHMOD(file, SUID_MODE);
 }
 
 static void cleanup(void)

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp