| Message ID | 1486721452-15097-1-git-send-email-ynorov@caviumnetworks.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Feb 10, 2017 at 5:10 AM, Yury Norov <ynorov@caviumnetworks.com> wrote: > > The following test sets the errno to invalid value (0xdead), and then calls > write() with intentionally wrong file descriptor to force kernel to return > EBADF which should be stored at errno location by SYSCALL_ERROR_HANDLER(). > > The test is failed before and passed after [1] for aarch64/ilp32 [2-3]. > > This is RFC because I'm not sure this is right way to check the macro > as it implicitly assumes that the write() is implemented with templates, > which may be wrong for some platform, or will become wrong in future. What you're testing here is errno. You're verifying that a particular syscall does set errno to a meaningful value when it fails. The way to make this a better test, and ensure you're not just testing one of several possible implementations of the trap-to-kernel-then-set-errno sequence, is to apply the same test to _as many syscalls as practical_. I think it would be enough to get all of the ones that can fail due to invalid arguments. Testing for _all_ the error cases is out of scope for glibc's testsuite -- that's more of an LTP thing -- and there are quite a few that can only fail due to resource exhaustion or inadequate permissions, both of which require more machinery to set up than we have. This should probably be two test programs: posix/test-errno.c would test all the syscalls in POSIX, and sysdeps/unix/sysv/linux/test-errno.c would test all the Linux-specific ones. zw
On Fri, Feb 10, 2017 at 07:51:07AM -0500, Zack Weinberg wrote: > tatus: RO > Content-Length: 1507 > Lines: 31 > > On Fri, Feb 10, 2017 at 5:10 AM, Yury Norov <ynorov@caviumnetworks.com> wrote: > > > > The following test sets the errno to invalid value (0xdead), and then calls > > write() with intentionally wrong file descriptor to force kernel to return > > EBADF which should be stored at errno location by SYSCALL_ERROR_HANDLER(). > > > > The test is failed before and passed after [1] for aarch64/ilp32 [2-3]. > > > > This is RFC because I'm not sure this is right way to check the macro > > as it implicitly assumes that the write() is implemented with templates, > > which may be wrong for some platform, or will become wrong in future. > > What you're testing here is errno. You're verifying that a particular > syscall does set errno to a meaningful value when it fails. > > The way to make this a better test, and ensure you're not just testing > one of several possible implementations of the > trap-to-kernel-then-set-errno sequence, is to apply the same test to > _as many syscalls as practical_. I think it would be enough to get > all of the ones that can fail due to invalid arguments. Testing for > _all_ the error cases is out of scope for glibc's testsuite -- that's > more of an LTP thing -- and there are quite a few that can only fail > due to resource exhaustion or inadequate permissions, both of which > require more machinery to set up than we have. I wrote this test to address this comment of Adhemerval: >> My only concern is if and why glibc own testsuite did not trigger this >> kind of issue in any tests and in this case if it was by chance or lack >> of coverage. If glibc own testsuite does not trigger this in any case, >> I think a regression should be added. https://sourceware.org/ml/libc-alpha/2017-02/msg00129.html So I only tried to test error path of syscall template. If you think that the correct way to do it is to call each syscall that may fail in each possible scenario - then I think glibc don't need the test like that, and we'd run LTP to find bugs of that sort - like I did in this case. > This should probably be two test programs: posix/test-errno.c would > test all the syscalls in POSIX, and > sysdeps/unix/sysv/linux/test-errno.c would test all the Linux-specific > ones. > zw
On Fri, Feb 10, 2017 at 8:27 AM, Yury Norov <ynorov@caviumnetworks.com> wrote: > On Fri, Feb 10, 2017 at 07:51:07AM -0500, Zack Weinberg wrote: >> On Fri, Feb 10, 2017 at 5:10 AM, Yury Norov <ynorov@caviumnetworks.com> wrote: >> > >> > This is RFC because I'm not sure this is right way to check the macro >> >> The way to make this a better test, and ensure you're not just testing >> one of several possible implementations of the >> trap-to-kernel-then-set-errno sequence, is to apply the same test to >> _as many syscalls as practical_. > > So I only tried to test error path of syscall template. If you think > that the correct way to do it is to call each syscall that may fail > in each possible scenario - then I think glibc don't need the test > like that, and we'd run LTP to find bugs of that sort - like I did in > this case. You asked how to make the test better. I told you how I think you could make the test better. I realize I'm asking for some extra work, but it should not take more than a half hour and it really will be a better test this way. (I _do_ think glibc should have a test like this. It does not require elaborate setup, and the more bugs we can catch _during development_, rather than months after the fact when someone thinks to run LTP, the better.) zw
On Fri, Feb 10, 2017 at 09:10:03AM -0500, Zack Weinberg wrote: > On Fri, Feb 10, 2017 at 8:27 AM, Yury Norov <ynorov@caviumnetworks.com> wrote: > > On Fri, Feb 10, 2017 at 07:51:07AM -0500, Zack Weinberg wrote: > >> On Fri, Feb 10, 2017 at 5:10 AM, Yury Norov <ynorov@caviumnetworks.com> wrote: > >> > > >> > This is RFC because I'm not sure this is right way to check the macro > >> > >> The way to make this a better test, and ensure you're not just testing > >> one of several possible implementations of the > >> trap-to-kernel-then-set-errno sequence, is to apply the same test to > >> _as many syscalls as practical_. > > > > So I only tried to test error path of syscall template. If you think > > that the correct way to do it is to call each syscall that may fail > > in each possible scenario - then I think glibc don't need the test > > like that, and we'd run LTP to find bugs of that sort - like I did in > > this case. > > You asked how to make the test better. I told you how I think you > could make the test better. > > I realize I'm asking for some extra work, but it should not take more > than a half hour and it really will be a better test this way. > > (I _do_ think glibc should have a test like this. It does not require > elaborate setup, and the more bugs we can catch _during development_, > rather than months after the fact when someone thinks to run LTP, the > better.) > > zw OK. Below is what I have. This is not the glibc test but standalone application. If I do what you mean, I will finish the syscall list (I take it from sysdeps/unix/syscalls.list), and send it as glibc test for POSIX syscalls. IIUC we need similar test for linux syscalls in sysdeps/unix/sysv/linux/syscalls.list Surprisingly, some syscalls cause segfaults on aarch64/lp64, which is presumably wrong. I didn't analyze it yet, and didn't finish the test. Just asking if I understand you right. Yury -- #include <errno.h> #include <fcntl.h> #include <stdio.h> #include <string.h> #include <sys/ioctl.h> #include <sys/socket.h> #include <sys/stat.h> #include <sys/time.h> #include <sys/types.h> #include <sys/vfs.h> #include <sys/mman.h> #include <unistd.h> #define test_wrp(err, test_syscall, params...) do { \ errno = 0xdead; \ int ret = test_syscall(params); \ if (ret != -1) \ printf("Syscall '" #test_syscall \ "' didn't fail as expected\n"); \ else if (errno == 0xdead) \ printf("Syscall'" #test_syscall \ "' didn't update errno\n"); \ else if (errno != err) { \ printf("Syscall'" #test_syscall "':\n"); \ printf("errno is: %d (%s)\nexpected: %d (%s)\n", \ errno, strerror (errno), err, strerror (err)); \ } \ else \ break; \ return -1; \ } while (0) int main(int argc, char *argv[]) { test_wrp(EBADF, write, -1, "Hello", sizeof("Hello") ); test_wrp(EFAULT, access, (void *) -1, 0); test_wrp(EFAULT, acct, (void *) -1); test_wrp(EBADF, bind, -1, (void *) -1, 0); test_wrp(EFAULT, chdir, (void *) -1); test_wrp(EFAULT, chmod, (void *) -1, 0); test_wrp(EBADF, close, -1); test_wrp(EBADF, connect, -1, (void *) -1, -1); test_wrp(EBADF, dup, -1); test_wrp(EBADF, fcntl, -1, 0); test_wrp(EBADF, fstatfs, -1, (void *) -1); test_wrp(EBADF, fsync, -1); test_wrp(EBADF, ftruncate, -1, 0); //test_wrp(EFAULT, getdomainname, (void *) -1, 1); //segfault test_wrp(EINVAL, getgroups, -1, (void *) -1); //test_wrp(EFAULT, gethostname, -1, (void *) -1); //segfault //test_wrp(EFAULT, gettimeofday, (void *) -1, (void *) -1); //segfault test_wrp(EBADF, ioctl, -1, 0); test_wrp(EFAULT, link, (void *) -1, (void *) -1); test_wrp(EBADF, listen, -1, -1); test_wrp(EBADF, lseek, -1, 0, 0); test_wrp(EINVAL, madvise, (void *) -1, 0, 0); test_wrp(EFAULT, mkdir, (void *) -1, 0); //test_wrp(EBADF, mmap, (void *) -1, 0, 0, 0, -1, 0); //compiler warning test_wrp(EINVAL, mprotect, (void *) -1, 0, 0); test_wrp(EINVAL, msync, (void *) -1, 0, 0); test_wrp(EINVAL, munmap, (void *) -1, 0); test_wrp(EFAULT, open, (void *) -1, 0); test_wrp(EBADF, read, -1, (void *) -1, 0); test_wrp(EINVAL, readlink, (void *) -1, (void *) -1, 0); test_wrp(EBADF, readv, -1, (void *) -1, 1); //test_wrp(EFAULT, recv, -1, (void *) -1, 1, 0); lp64 and ilp32 //different errors test_wrp(EBADF, recvmsg, -1, (void *) -1, 0); test_wrp(EFAULT, rename, (void *) -1, (void *) -1); test_wrp(EFAULT, rmdir, (void *) -1); //test_wrp(EINVAL, select, -1, (void *) -1, (void *) -1, (void *) -1, (void *) -1); //segfault return 0; }
On Sat, Feb 11, 2017 at 9:46 AM, Yury Norov <ynorov@caviumnetworks.com> wrote: > > OK. Below is what I have. This is not the glibc test but standalone > application. If I do what you mean, I will finish the syscall list > (I take it from sysdeps/unix/syscalls.list), and send it as glibc test > for POSIX syscalls. Yes, this is what I had in mind. (POSIX syscalls is not exactly the same thing as sysdeps/unix/syscalls.list but it's probably easier to put the test in sysdeps/unix than worry about which syscalls are or are not standardized.) > IIUC we need similar test for linux syscalls in > sysdeps/unix/sysv/linux/syscalls.list Right. > Surprisingly, some syscalls cause segfaults on aarch64/lp64, which is > presumably wrong. I dug into this a bit. The problem is that under any conditions where a syscall is documented to return EFAULT, it is also allowed to trigger a SIGSEGV. (See http://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_03 under [EFAULT].) So we cannot do this test for syscalls whose only invalid-arguments failure mode is EFAULT, and we need to avoid setting up EFAULT conditions for any other syscalls. I've revised what you had with that in mind. I also fixed the problem with mmap, made sure that we were only setting up _one_ failure condition for each system call, made it always run all the tests instead of stopping at the first failure, and reformatted according to GNU style. It should be pretty easy to go on from here for the rest of syscalls.list. zw #include <errno.h> #include <fcntl.h> #include <stdio.h> #include <string.h> #include <sys/ioctl.h> #include <sys/socket.h> #include <sys/stat.h> #include <sys/time.h> #include <sys/types.h> #include <sys/vfs.h> #include <sys/mman.h> #include <unistd.h> #include <netinet/in.h> // Test that failing system calls do set errno to the correct value. // // This is not an exhaustive test: only system calls that can be // persuaded to fail with a consistent error code and no side effects // are included. Usually these are failures due to invalid arguments, // with errno code EBADF or EINVAL. The order of argument checks is // unspecified, so we must take care to provide arguments that only // allow _one_ failure mode. // // Note that all system calls that can fail with EFAULT are permitted // to deliver a SIGSEGV signal instead, so we avoid supplying invalid // pointers in general, and we do not attempt to test system calls // that can only fail with EFAULT (e.g. gettimeofday, gethostname). // // Also note that root-only system calls (e.g. acct, reboot) may, when // the test is run as an unprivileged user, fail due to insufficient // privileges before bothering to do argument checks, so those are not // tested either. // // Some tests assume "/bin/sh" names a file that exists and is not a // directory. #define test_wrp_rv(rtype, prtype, experr, syscall, ...) \ (__extension__ ({ \ errno = 0xdead; \ rtype ret = syscall (__VA_ARGS__); \ int err = errno; \ int fail; \ if (ret == (rtype)-1 && err == experr) \ fail = 0; \ else \ { \ fail = 1; \ if (ret != (rtype)-1) \ fprintf (stderr, #syscall ": didn't fail as expected" \ " (return "prtype")\n", ret); \ else if (err == 0xdead) \ fputs(#syscall ": didn't update errno\n", stderr); \ else if (err != experr) \ fprintf (stderr, #syscall \ ": errno is: %d (%s) expected: %d (%s)\n", \ err, strerror (err), experr, strerror (experr)); \ } \ fail; \ })) #define test_wrp(experr, syscall, ...) \ test_wrp_rv(int, "%d", experr, syscall, __VA_ARGS__) int main(void) { size_t pagesize = sysconf(_SC_PAGESIZE); struct statfs sfs; char buf[1]; struct iovec iov[1] = { { buf, 1 } }; struct sockaddr_in sin; sin.sin_family = AF_INET; sin.sin_port = htons (1026); sin.sin_addr.s_addr = htonl (INADDR_LOOPBACK); struct msghdr msg; memset(&msg, 0, sizeof msg); msg.msg_iov = iov; msg.msg_iovlen = 1; int fails = 0; fails |= test_wrp (EINVAL, access, "/", -1); fails |= test_wrp (EBADF, bind, -1, (struct sockaddr *)&sin, sizeof sin); fails |= test_wrp (ENOTDIR, chdir, "/bin/sh"); fails |= test_wrp (EBADF, close, -1); fails |= test_wrp (EBADF, connect, -1, (struct sockaddr *)&sin, sizeof sin); fails |= test_wrp (EBADF, dup, -1); fails |= test_wrp (EBADF, fcntl, -1, 0); fails |= test_wrp (EBADF, fstatfs, -1, &sfs); fails |= test_wrp (EBADF, fsync, -1); fails |= test_wrp (EBADF, ftruncate, -1, 0); fails |= test_wrp (EINVAL, getgroups, -1, 0); fails |= test_wrp (EBADF, ioctl, -1, TIOCNOTTY); fails |= test_wrp (EBADF, listen, -1, 1); fails |= test_wrp (EBADF, lseek, -1, 0, 0); fails |= test_wrp (EINVAL, madvise, (void *) -1, -1, 0); fails |= test_wrp_rv (void *, "%p", EBADF, mmap, 0, pagesize, PROT_READ, MAP_PRIVATE, -1, 0); fails |= test_wrp (EINVAL, mprotect, (void *) -1, pagesize, -1); fails |= test_wrp (EINVAL, msync, (void *) -1, pagesize, -1); fails |= test_wrp (EINVAL, munmap, (void *) -1, 0); fails |= test_wrp (EINVAL, open, "/bin/sh", -1, 0); fails |= test_wrp (EBADF, read, -1, buf, 1); fails |= test_wrp (EINVAL, readlink, "/", buf, -1); fails |= test_wrp (EBADF, readv, -1, iov, 1); fails |= test_wrp (EBADF, recv, -1, buf, 1, 0); fails |= test_wrp (EBADF, recvmsg, -1, &msg, 0); fails |= test_wrp (EINVAL, select, -1, 0, 0, 0, 0); fails |= test_wrp (EBADF, write, -1, "Hello", sizeof("Hello") ); return fails; }
diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile index b3d6866..8c4cfd0 100644 --- a/sysdeps/unix/sysv/linux/Makefile +++ b/sysdeps/unix/sysv/linux/Makefile @@ -43,7 +43,7 @@ sysdep_headers += sys/mount.h sys/acct.h sys/sysctl.h \ bits/mman-linux.h tests += tst-clone tst-clone2 tst-fanotify tst-personality tst-quota \ - tst-sync_file_range + tst-sync_file_range tst-syscall-template # Generate the list of SYS_* macros for the system calls (__NR_* macros). diff --git a/sysdeps/unix/sysv/linux/tst-syscall-template.c b/sysdeps/unix/sysv/linux/tst-syscall-template.c new file mode 100644 index 0000000..47e47d0 --- /dev/null +++ b/sysdeps/unix/sysv/linux/tst-syscall-template.c @@ -0,0 +1,55 @@ +/* Basic tests for syscall template errno setup. + + Copyright (C) 2016 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <http://www.gnu.org/licenses/>. */ + +#include <stdio.h> +#include <errno.h> +#include <unistd.h> + +static int do_test (void); +#define TEST_FUNCTION do_test () + +/* This defines the `main' function and some more. */ +#include <test-skeleton.c> + +static int +do_test (void) +{ + const char str[] = "Hello wold!\n"; + int err; + + errno = 0xDEAD; + + if (errno != 0xDEAD) + FAIL_EXIT1 ("Cannot access errno location"); + + /* Write at intectionally invalid file descriptor. + Function should return -1, and set errno to EBADF. */ + err = write (-1, str, sizeof (str)); + + if (err != -1) + FAIL_EXIT1 ("Unexpected write success"); + + if (errno == 0xDEAD) + FAIL_EXIT1 ("Errno is not updated"); + + if (errno != EBADF) + FAIL_EXIT1 ("Wrong error code: %d", errno); + + return 0; +}
The patch [1] fixes the bug in (not public available yet) aarc64/ilp32 port in SYSCALL_ERROR_HANDLER() macro - wrong calculation of errno location. The macro is called in error path of a syscall generated from syscall templates with PSEUDO* machinery. The bug has been discovered during the work on LTP tests, and it looks like there's no a glibc test to check it. For sure, there's no specific test for syscall templates. The following test sets the errno to invalid value (0xdead), and then calls write() with intentionally wrong file descriptor to force kernel to return EBADF which should be stored at errno location by SYSCALL_ERROR_HANDLER(). The test is failed before and passed after [1] for aarch64/ilp32 [2-3]. This is RFC because I'm not sure this is right way to check the macro as it implicitly assumes that the write() is implemented with templates, which may be wrong for some platform, or will become wrong in future. Though, if the test is OK, please apply it. [1] https://sourceware.org/ml/libc-alpha/2017-02/msg00110.html [2] https://github.com/norov/glibc/tree/dev9 [3] https://github.com/norov/linux/tree/ilp32-20170203 * sysdeps/unix/sysv/linux/Makefile: Enable new test tst-syscall-template. * sysdeps/unix/sysv/linux/tst-syscall-template.c: New test. Signed-off-by: Yury Norov <ynorov@caviumnetworks.com> --- sysdeps/unix/sysv/linux/Makefile | 2 +- sysdeps/unix/sysv/linux/tst-syscall-template.c | 55 ++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 1 deletion(-) create mode 100644 sysdeps/unix/sysv/linux/tst-syscall-template.c