[RFC] Test for syscall templates
diff mbox

Message ID 1486721452-15097-1-git-send-email-ynorov@caviumnetworks.com
State New
Headers show

Commit Message

Yury Norov Feb. 10, 2017, 10:10 a.m. UTC
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

Comments

Zack Weinberg Feb. 10, 2017, 12:51 p.m. UTC | #1
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
Yury Norov Feb. 10, 2017, 1:27 p.m. UTC | #2
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
Zack Weinberg Feb. 10, 2017, 2:10 p.m. UTC | #3
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
Yury Norov Feb. 11, 2017, 2:46 p.m. UTC | #4
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;
}
Zack Weinberg Feb. 11, 2017, 6:20 p.m. UTC | #5
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;
}

Patch
diff mbox

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;
+}