Message ID | 20200724125052.20973-2-mdoucha@suse.cz |
---|---|
State | Superseded |
Headers | show |
Series | [v2,1/2] Convert chdir01 to the new API | expand |
Hi Martin, > +++ b/runtest/syscalls > @@ -54,6 +54,7 @@ capset04 capset04 > cacheflush01 cacheflush01 > chdir01 chdir01 > +chdir02 chdir02 > chdir01A symlink01 -T chdir01 > chdir04 chdir04 You missed to add chdir02 to runtest/quickhit. I guess this was deliberate, right? (I wonder if we really need runtest/quickhit anyway). I like both tests (nice work, thanks!), just don't like the duplicity. Isn't there a way to use getopt parameter for one of the variants and have just single test? But understand if you don't bother with it (maybe better duplicity but simpler code). Other that that LGTM. Reviewed-by: Petr Vorel <pvorel@suse.cz> Kind regards, Petr
On 24. 07. 20 15:32, Petr Vorel wrote: > Hi Martin, > >> +++ b/runtest/syscalls >> @@ -54,6 +54,7 @@ capset04 capset04 >> cacheflush01 cacheflush01 > >> chdir01 chdir01 >> +chdir02 chdir02 >> chdir01A symlink01 -T chdir01 >> chdir04 chdir04 > You missed to add chdir02 to runtest/quickhit. I guess this was deliberate, > right? > (I wonder if we really need runtest/quickhit anyway). Yes, this was deliberate since only chdir02 was there originally (the one that was checking only chdir("/"); and chdir("/etc");. > I like both tests (nice work, thanks!), just don't like the duplicity. Isn't > there a way to use getopt parameter for one of the variants and have just single > test? But understand if you don't bother with it (maybe better duplicity but > simpler code). > > Other that that LGTM. > Reviewed-by: Petr Vorel <pvorel@suse.cz> I could add a second set of expected values to the test case list and do it like this: static void run(unsigned int n) { TEST(chdir(tc->name)); /* result validation here */ SAFE_SETEUID(ltpuser->pw_uid); TEST(chdir(tc->name)); SAFE_SETEUID(0); /* result validation here */ } Should I resubmit that as v3?
Hi Martin, > >> +++ b/runtest/syscalls > >> @@ -54,6 +54,7 @@ capset04 capset04 > >> cacheflush01 cacheflush01 > >> chdir01 chdir01 > >> +chdir02 chdir02 > >> chdir01A symlink01 -T chdir01 > >> chdir04 chdir04 > > You missed to add chdir02 to runtest/quickhit. I guess this was deliberate, > > right? > > (I wonder if we really need runtest/quickhit anyway). > Yes, this was deliberate since only chdir02 was there originally (the > one that was checking only chdir("/"); and chdir("/etc");. Understand, thanks for info. > > I like both tests (nice work, thanks!), just don't like the duplicity. Isn't > > there a way to use getopt parameter for one of the variants and have just single > > test? But understand if you don't bother with it (maybe better duplicity but > > simpler code). > > Other that that LGTM. > > Reviewed-by: Petr Vorel <pvorel@suse.cz> > I could add a second set of expected values to the test case list and do > it like this: > static void run(unsigned int n) { > TEST(chdir(tc->name)); > /* result validation here */ > SAFE_SETEUID(ltpuser->pw_uid); > TEST(chdir(tc->name)); > SAFE_SETEUID(0); > /* result validation here */ > } LGTM, thank you! > Should I resubmit that as v3? Yes, please. Kind regards, Petr
diff --git a/runtest/syscalls b/runtest/syscalls index 70b3277d3..deadc21bc 100644 --- a/runtest/syscalls +++ b/runtest/syscalls @@ -54,6 +54,7 @@ capset04 capset04 cacheflush01 cacheflush01 chdir01 chdir01 +chdir02 chdir02 chdir01A symlink01 -T chdir01 chdir04 chdir04 diff --git a/testcases/kernel/syscalls/chdir/.gitignore b/testcases/kernel/syscalls/chdir/.gitignore index 1b15eb6b9..3475c5e54 100644 --- a/testcases/kernel/syscalls/chdir/.gitignore +++ b/testcases/kernel/syscalls/chdir/.gitignore @@ -1,2 +1,3 @@ /chdir01 +/chdir02 /chdir04 diff --git a/testcases/kernel/syscalls/chdir/chdir02.c b/testcases/kernel/syscalls/chdir/chdir02.c new file mode 100644 index 000000000..e62362808 --- /dev/null +++ b/testcases/kernel/syscalls/chdir/chdir02.c @@ -0,0 +1,134 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (c) International Business Machines Corp., 2001 + * 07/2001 Ported by Wayne Boyer + * Copyright (c) 2019 SUSE LLC <mdoucha@suse.cz> + */ + +/* + * Check that the chdir() syscall returns correct value and error code + * in various situations when called by regular user + */ + +#include <stdio.h> +#include <stdlib.h> +#include <sys/types.h> +#include <unistd.h> +#include <pwd.h> + +#include "tst_test.h" + +#define MNTPOINT "mntpoint" + +#define FILE_NAME "testfile" +#define DIR_NAME "subdir" +#define BLOCKED_NAME "keep_out" +#define LINK_NAME1 "symloop" +#define LINK_NAME2 "symloop2" + +static char *workdir; +static unsigned int skip_symlinks, skip_blocked; +static struct passwd *ltpuser; + +static struct test_case { + const char *name; + int retval, experr; +} testcase_list[] = { + {FILE_NAME, -1, ENOTDIR}, + {BLOCKED_NAME, -1, EACCES}, + {DIR_NAME, 0, 0}, + {".", 0, 0}, + {"..", 0, 0}, + {"/", 0, 0}, + {"missing", -1, ENOENT}, + {LINK_NAME1, -1, ELOOP}, +}; + +static void setup(void) +{ + char *cwd; + int fd; + struct stat statbuf; + + cwd = SAFE_GETCWD(NULL, 0); + workdir = SAFE_MALLOC(strlen(cwd) + strlen(MNTPOINT) + 2); + sprintf(workdir, "%s/%s", cwd, MNTPOINT); + free(cwd); + SAFE_CHDIR(workdir); + SAFE_MKDIR(DIR_NAME, 0755); + SAFE_MKDIR(BLOCKED_NAME, 0644); + + /* FAT and NTFS override file and directory permissions */ + SAFE_STAT(BLOCKED_NAME, &statbuf); + skip_blocked = statbuf.st_mode & 0111; + skip_symlinks = 0; + TEST(symlink(LINK_NAME1, LINK_NAME2)); + + if (!TST_RET) + SAFE_SYMLINK(LINK_NAME2, LINK_NAME1); + else if (TST_RET == -1 && TST_ERR == EPERM) + skip_symlinks = 1; /* man symlink(2): EPERM == unsupported */ + else + tst_brk(TBROK | TTERRNO, "Cannot create symlinks"); + + fd = SAFE_CREAT(FILE_NAME, 0644); + SAFE_CLOSE(fd); + + if (!ltpuser) + ltpuser = SAFE_GETPWNAM("nobody"); + + SAFE_SETEUID(ltpuser->pw_uid); +} + +static void run(unsigned int n) +{ + struct test_case *tc = testcase_list + n; + + if (tc->experr == EACCES && skip_blocked) { + tst_res(TCONF, "Skipping permission test, FS mangles dir mode"); + return; + } + + if (tc->experr == ELOOP && skip_symlinks) { + tst_res(TCONF, "Skipping symlink loop test, not supported"); + return; + } + + /* Reset current directory to mountpoint */ + SAFE_CHDIR(workdir); + + TEST(chdir(tc->name)); + + if (TST_RET != tc->retval) { + tst_res(TFAIL | TTERRNO, + "chdir(\"%s\") returned unexpected value %ld", + tc->name, TST_RET); + return; + } + + if (TST_RET != 0 && TST_ERR != tc->experr) { + tst_res(TFAIL | TTERRNO, + "chdir(\"%s\") returned unexpected error", tc->name); + return; + } + + tst_res(TPASS | TTERRNO, "chdir(\"%s\") returned correct value", + tc->name); +} + +static void cleanup(void) +{ + free(workdir); + SAFE_SETEUID(0); +} + +static struct tst_test test = { + .needs_root = 1, + .mount_device = 1, + .mntpoint = MNTPOINT, + .all_filesystems = 1, + .test = run, + .tcnt = ARRAY_SIZE(testcase_list), + .setup = setup, + .cleanup = cleanup +};
chdir01 tests chdir() return values with root permissions. chdir02 does the same test scenario with EUID set to nobody. Signed-off-by: Martin Doucha <mdoucha@suse.cz> --- Changes since v1: New patch runtest/syscalls | 1 + testcases/kernel/syscalls/chdir/.gitignore | 1 + testcases/kernel/syscalls/chdir/chdir02.c | 134 +++++++++++++++++++++ 3 files changed, 136 insertions(+) create mode 100644 testcases/kernel/syscalls/chdir/chdir02.c