On 04/04/2017 01:14 PM, Steve Beattie wrote: > Hey Colin, > > On Tue, Apr 04, 2017 at 03:16:29PM -0000, Colin Ian King wrote: >> Colin Ian King has proposed merging >> lp:~colin-king/apparmor/fix-arm64-test-builds into lp:apparmor. >> >> Requested reviews: >> AppArmor Developers (apparmor-dev) >> >> For more details, see: >> https://code.launchpad.net/~colin-king/apparmor/fix-arm64-test-builds/+merge/321876 >> >> This fixes build issues for the readdir test for arm64 where getdents(2) >> is not wired up as a system call but gendents64(2) is available. >> This changes the preference to use the 64 bit system call by default >> if it is available on 64 bit systems. > > Thanks for providing this fix. > > Realastically, though, where an architecture supports both getdents() > and getdents64(), we ought to be ensuring that apparmor mediation occurs > on both syscall paths. What follows is a patch that attempts to do that.
Smart thinking. I like the idea but have one nitpick and one objection below... > > Subject: tests: readdir - test both getdents() and getdents64() if available > > In commit 3649, Colin King fixed the readdir test build issue where aarch64 > only > supports getdetns64(), not getdents(). Realistically, however, we want > to ensure mediation occurs on both syscalls where they exist. This patch > changes the test to attempt performing both versions of getdents(). > > Bug: https://bugs.launchpad.net/bugs/1674245 > > Signed-off-by: Steve Beattie <[email protected]> > --- > tests/regression/apparmor/readdir.c | 80 > ++++++++++++++++++++++++++---------- > 1 file changed, 59 insertions(+), 21 deletions(-) > > Index: b/tests/regression/apparmor/readdir.c > =================================================================== > --- a/tests/regression/apparmor/readdir.c > +++ b/tests/regression/apparmor/readdir.c > @@ -1,10 +1,14 @@ > /* > * Copyright (C) 2002-2006 Novell/SUSE > + * Copyright (C) 2017 Canonical, Ltd. > * > * This program is free software; you can redistribute it and/or > * modify it under the terms of the GNU General Public License as > * published by the Free Software Foundation, version 2 of the > * License. > + * > + * We attempt to test both getdents() and getdents64() here, but > + * some architectures like aarch64 only support getdents64(). > */ > #define _GNU_SOURCE > > @@ -18,45 +22,79 @@ > #include <string.h> > #include <sys/syscall.h> > > -int main(int argc, char *argv[]) > +#ifdef SYS_getdents > +int my_readdir(char *dirname) Nitpick... Please use my_getdents() and my_getdents64() since these are getdents wrappers and not readdir wrappers. > { > int fd; > -#if defined(SYS_getdents64) > - struct dirent64 dir; > -#else > struct dirent dir; > -#endif > > - if (argc != 2){ > - fprintf(stderr, "usage: %s dir\n", > - argv[0]); > + fd = open(dirname, O_RDONLY, 0); > + if (fd == -1) { > + printf("FAIL - open %s\n", strerror(errno)); > return 1; > } > > - fd = open(argv[1], O_RDONLY, 0); > - if (fd == -1){ > - printf("FAIL - open %s\n", strerror(errno)); > + /* getdents isn't exported by glibc, so must use syscall() */ > + if (syscall(SYS_getdents, fd, &dir, sizeof(dir)) == -1){ > + printf("FAIL - getdents %s\n", strerror(errno)); > + close(fd); > return 1; > } > > - /* > - if (fchdir(fd) == -1){ > - printf("FAIL - fchdir %s\n", strerror(errno)); > + close(fd); > + return 0; > +} > +#else > +int my_readdir(char *dirname) { return 0; } > +#endif > + > +#ifdef SYS_getdents64 > +int my_readdir64(char *dirname) > +{ > + int fd; > + struct dirent64 dir; > + > + fd = open(dirname, O_RDONLY, 0); > + if (fd == -1) { > + printf("FAIL - open %s\n", strerror(errno)); > return 1; > } > - */ > > /* getdents isn't exported by glibc, so must use syscall() */ > -#if defined(SYS_getdents64) > - if (syscall(SYS_getdents64, fd, &dir, sizeof(struct dirent64)) == -1){ > + if (syscall(SYS_getdents64, fd, &dir, sizeof(dir)) == -1){ > + printf("FAIL - getdents64 %s\n", strerror(errno)); > + close(fd); > + return 1; > + } > + > + close(fd); > + return 0; > +} > #else > - if (syscall(SYS_getdents, fd, &dir, sizeof(struct dirent)) == -1){ > +int my_readdir64(char *dirname) { return 0; } > #endif > - printf("FAIL - getdents %s\n", strerror(errno)); > - return 1; > + > +int main(int argc, char *argv[]) > +{ > + int rc = 0; > + > + if (argc != 2) { > + fprintf(stderr, "usage: %s dir\n", > + argv[0]); > + rc = 1; > + goto err; > } > > + rc = my_readdir(argv[1]); > + if (rc != 0) > + goto err; > + > + rc = my_readdir64(argv[1]); > + if (rc != 0) > + goto err; Objection... In an expected fail test case, getdents64() will not be tested because getdents() will have already failed. The test script should pass the expected behavior to the test program and the test program will need to verify that both syscalls match the expected behavior. Alternatively, a shortcut would be to test both syscalls and if the results are different, raise an ERROR instead of a FAIL. Tyler > + > printf("PASS\n"); > > - return 0; > +err: > + return rc; > } > > > >
signature.asc
Description: OpenPGP digital signature
-- AppArmor mailing list [email protected] Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
