Hello.
Thank you for your contribution. Thanks to it, several bugs in current prctl
decoder implementation have been revealed. Please take a look
On Thu, Nov 17, 2016 at 09:17:27PM +0800, JingPiao Chen wrote:
> From 8995d87f0c27d40d574f032bf483f6ae2b5f7375 Mon Sep 17 00:00:00 2001
> From: JingPiao Chen <chenjingp...@foxmail.com>
> Date: Thu, 17 Nov 2016 21:15:51 +0800
> Subject: [PATCH] tests: add prctl-name.test, prctl-pdeathsig.test and
> prctl-tsc.test
>
>
> * tests/prctl-name.c:New file.
Missing space.
> * tests/prctl-name.test: Likewise.
The established description format for the new *.test files is "New test".
> * tests/prctl-pdeathsig.c:Likewise.
> * tests/prctl-pdeathsig.test: Likewise.
> * tests/prctl-tsc.c: Likewise.
> * tests/prctl-tsc.test: Likewise
Summing up two previous notes, this part of commit message could be
rewritten as follows:
[[
* tests/prctl-name.c: New file.
* tests/prctl-pdeathsig.c:Likewise.
* tests/prctl-tsc.c: Likewise.
* tests/prctl-name.test: New test.
* tests/prctl-pdeathsig.test: Likewise.
* tests/prctl-tsc.test: Likewise
]]
> * tests/.gitignore: Add prctl-name, prctl-pdeathsig and prctl-tsc.
> * tests/Makefile.am (check_PROGRAMS): Likewise.
> (DECODER_TESTS): Add prctl-name.test, prctl-pdeathsig.test and prctl-tsc.test
> ---
> tests/.gitignore | 3 +++
> tests/Makefile.am | 6 +++++
> tests/prctl-name.c | 63
> ++++++++++++++++++++++++++++++++++++++++++++++
> tests/prctl-name.test | 6 +++++
> tests/prctl-pdeathsig.c | 61 ++++++++++++++++++++++++++++++++++++++++++++
> tests/prctl-pdeathsig.test | 6 +++++
> tests/prctl-tsc.c | 60 +++++++++++++++++++++++++++++++++++++++++++
> tests/prctl-tsc.test | 6 +++++
> 8 files changed, 211 insertions(+)
> create mode 100644 tests/prctl-name.c
> create mode 100755 tests/prctl-name.test
> create mode 100644 tests/prctl-pdeathsig.c
> create mode 100755 tests/prctl-pdeathsig.test
> create mode 100644 tests/prctl-tsc.c
> create mode 100755 tests/prctl-tsc.test
>
>
> diff --git a/tests/.gitignore b/tests/.gitignore
> index 6fc3cd1..8497dfa 100644
> --- a/tests/.gitignore
> +++ b/tests/.gitignore
> @@ -207,8 +207,11 @@ pkey_free
> pkey_mprotect
> poll
> ppoll
> +prctl-name
> +prctl-pdeathsig
> prctl-seccomp-filter-v
> prctl-seccomp-strict
> +prctl-tsc
> pread64-pwrite64
> preadv
> preadv-pwritev
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index df5ddb2..744ed25 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -265,8 +265,11 @@ check_PROGRAMS = \
> pkey_mprotect \
> poll \
> ppoll \
> + prctl-name \
> + prctl-pdeathsig \
> prctl-seccomp-filter-v \
> prctl-seccomp-strict \
> + prctl-tsc \
> pread64-pwrite64 \
> preadv \
> preadv-pwritev \
> @@ -637,8 +640,11 @@ DECODER_TESTS = \
> pkey_mprotect.test \
> poll.test \
> ppoll.test \
> + prctl-name.test \
> + prctl-pdeathsig.test \
> prctl-seccomp-filter-v.test \
> prctl-seccomp-strict.test \
> + prctl-tsc.test \
> pread64-pwrite64.test \
> preadv-pwritev.test \
> preadv2-pwritev2.test \
> diff --git a/tests/prctl-name.c b/tests/prctl-name.c
> new file mode 100644
> index 0000000..a508969
> --- /dev/null
> +++ b/tests/prctl-name.c
> @@ -0,0 +1,63 @@
> +/*
It is recommended to have a brief description of the file in the beginning of
this comment block. For example, "Check decoding of prctl
PR_GET_NAME/PR_SET_NAME operations."
> + * Copyright (c) 2016 JingPiao Chen <chenjingp...@foxmail.com>
> + * All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + * notice, this list of conditions and the following disclaimer in the
> + * documentation and/or other materials provided with the distribution.
> + * 3. The name of the author may not be used to endorse or promote products
> + * derived from this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE AUTHOR ``AS IS'' AND ANY EXPRESS OR
> + * IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
> + * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED.
> + * IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT,
> + * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
> + * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
> + * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#include "tests.h"
> +#include <asm/unistd.h>
> +
> +#ifdef HAVE_PRCTL
> +# include <sys/prctl.h>
> +#endif
> +
> +#if defined HAVE_PRCTL && defined PR_GET_NAME && defined PR_SET_NAME
> +
> +#include <stdio.h>
> +#include <unistd.h>
> +
> +int
> +main(void)
> +{
> + char setname[16] = "test-set-name";
> + char getname[16];
It is preferred to use tail_alloc()-based allocations in order to check
correctness of behaviour comparing to what kernel does regarding reading
data from process's address space. Please have a look:
https://github.com/strace/strace/blob/master/tests/preadv-pwritev.c#L68
https://github.com/strace/strace/blob/master/tests/perf_event_open.c#L182
https://github.com/strace/strace/blob/master/tests/aio.c#L230
> + long rc;
> +
> + rc = syscall(__NR_prctl, PR_SET_NAME, setname);
> + printf("prctl(PR_SET_NAME, \"%s\\0\\0\\0\") = %s\n",
> + setname, sprintrc(rc));
Note that this behaviour is not what expected judging by prctl(2) and
kernel/sys.c. Please take a look at
https://github.com/strace/strace/commit/43017ef466a8540c09e95da9b602e4e56dca8a94
for a fix.
> +
> + rc = syscall(__NR_prctl, PR_GET_NAME, getname);
> + printf("prctl(PR_GET_NAME, \"%s\") = %s\n",
Traditionally, the alignment produced by -a option is not included in
expected output produced by test; value of -a parameter is set in
accordance with shortest function call output instead.
> + getname, sprintrc(rc));
> +
> + puts("+++ exited with 0 +++");
> + return 0;
> +}
> +
> +#else
> +
> +SKIP_MAIN_UNDEFINED("HAVE_PRCTL && PR_GET_NAME && PR_SET_NAME")
> +
> +#endif
Overall, this test is doesn't check the following corner cases:
* Inaccessibility of data pointed by arg2
* Partial inaccessibility of data pointed by arg2
* Limit on amount of data read by kernel (kernel does not read more than
TASK_COMM_LEN - 1 bytes and termin)
After fixing up the bug this code revealed, Dmitry decided to rewrite it from
scratch:
https://github.com/strace/strace/blob/master/tests/prctl-name.c
> diff --git a/tests/prctl-name.test b/tests/prctl-name.test
> new file mode 100755
> index 0000000..fae18b3
> --- /dev/null
> +++ b/tests/prctl-name.test
> @@ -0,0 +1,6 @@
> +#!/bin/sh
> +
> +# Check prctl PR_GET_NAME PR_SET_NAME decoding.
> +
> +. "${srcdir=.}/init.sh"
> +run_strace_match_diff -a42 -e trace=prctl
> diff --git a/tests/prctl-pdeathsig.c b/tests/prctl-pdeathsig.c
> new file mode 100644
> index 0000000..db2e7b3
> --- /dev/null
> +++ b/tests/prctl-pdeathsig.c
> @@ -0,0 +1,61 @@
> +/*
> + * Copyright (c) 2016 JingPiao Chen <chenjingp...@foxmail.com>
> + * All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + * notice, this list of conditions and the following disclaimer in the
> + * documentation and/or other materials provided with the distribution.
> + * 3. The name of the author may not be used to endorse or promote products
> + * derived from this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE AUTHOR ``AS IS'' AND ANY EXPRESS OR
> + * IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
> + * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED.
> + * IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT,
> + * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
> + * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
> + * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#include "tests.h"
> +#include <asm/unistd.h>
> +
> +#ifdef HAVE_PRCTL
> +# include <sys/prctl.h>
> +#endif
> +
> +#if defined HAVE_PRCTL && defined PR_GET_PDEATHSIG && defined
> PR_SET_PDEATHSIG
> +
> +#include <stdio.h>
> +#include <unistd.h>
> +#include <sys/signal.h>
> +
> +int
> +main(void)
> +{
> + int pdeathsig;
> + long rc;
> +
> + rc = syscall(__NR_prctl, PR_SET_PDEATHSIG, SIGINT);
> + printf("prctl(PR_SET_PDEATHSIG, SIGINT) = %s\n", sprintrc(rc));
Excessive whitespace, as noted above.
> +
> + rc = syscall(__NR_prctl, PR_GET_PDEATHSIG, &pdeathsig);
> + printf("prctl(PR_GET_PDEATHSIG, [SIGINT]) = %s\n", sprintrc(rc));
Note that the expected output of this call is depended on success of this and
the previous call (it may fail due to seccomp restrictions, for example).
As a result, it should be checked somehow, for example:
int pdeathsig;
long rc;
- rc = syscall(__NR_prctl, PR_SET_PDEATHSIG, SIGINT);
+ if ((rc = syscall(__NR_prctl, PR_SET_PDEATHSIG, SIGINT)))
+ perror_msg_and_skip("prctl(PR_SET_PDEATHSIG)");
printf("prctl(PR_SET_PDEATHSIG, SIGINT) = %s\n", sprintrc(rc));
- rc = syscall(__NR_prctl, PR_GET_PDEATHSIG, &pdeathsig);
+ if ((rc = syscall(__NR_prctl, PR_GET_PDEATHSIG, &pdeathsig)))
+ perror_msg_and_skip("prctl(PR_GET_PDEATHSIG)");
printf("prctl(PR_GET_PDEATHSIG, [SIGINT]) = %s\n", sprintrc(rc));
puts("+++ exited with 0 +++");
> +
> + puts("+++ exited with 0 +++");
> + return 0;
> +}
> +
> +#else
> +
> +SKIP_MAIN_UNDEFINED("HAVE_PRCTL && PR_GET_PDEATHSIG && PR_SET_PDEATHSIG")
> +
> +#endif
This test also lacks checks for unknown signal value, for argument size (see
https://github.com/strace/strace/commit/aeff861218c14cbc5b5bf2c0f1f4ad497bc70a0b
; this is important due to the fact that long and kernel_ulong_t have different
size on X32/N32 ABIs, and prctl does not implement compat syscall for these
ABIs) and for inaccessibility of pointer. For example:
--- a/tests/prctl-pdeathsig.c
+++ b/tests/prctl-pdeathsig.c
@@ -38,17 +38,32 @@
# include <unistd.h>
# include <sys/signal.h>
+# include "kernel_types.h"
+
int
main(void)
{
- int pdeathsig;
+ static const kernel_ulong_t bogus_signal =
+ (kernel_ulong_t) 0xbadc0deddeadfeedULL;
+
+ int *pdeathsig = tail_alloc(sizeof(*pdeathsig));
long rc;
+ rc = syscall(__NR_prctl, PR_SET_PDEATHSIG, bogus_signal);
+ printf("prctl(PR_SET_PDEATHSIG, %llu) = %s\n",
+ (unsigned long long) bogus_signal, sprintrc(rc));
+
if ((rc = syscall(__NR_prctl, PR_SET_PDEATHSIG, SIGINT)))
perror_msg_and_skip("prctl(PR_SET_PDEATHSIG)");
printf("prctl(PR_SET_PDEATHSIG, SIGINT) = %s\n", sprintrc(rc));
- if ((rc = syscall(__NR_prctl, PR_GET_PDEATHSIG, &pdeathsig)))
+ rc = syscall(__NR_prctl, PR_GET_PDEATHSIG, NULL);
+ printf("prctl(PR_GET_PDEATHSIG, NULL) = %s\n", sprintrc(rc));
+
+ rc = syscall(__NR_prctl, PR_GET_PDEATHSIG, pdeathsig + 1);
+ printf("prctl(PR_GET_PDEATHSIG, %p) = %s\n",
+ pdeathsig + 1, sprintrc(rc));
+
+ if ((rc = syscall(__NR_prctl, PR_GET_PDEATHSIG, pdeathsig)))
perror_msg_and_skip("prctl(PR_GET_PDEATHSIG)");
printf("prctl(PR_GET_PDEATHSIG, [SIGINT]) = %s\n", sprintrc(rc));
> diff --git a/tests/prctl-pdeathsig.test b/tests/prctl-pdeathsig.test
> new file mode 100755
> index 0000000..5e0be13
> --- /dev/null
> +++ b/tests/prctl-pdeathsig.test
> @@ -0,0 +1,6 @@
> +#!/bin/sh
> +
> +# Check prctl PR_GET_PDEATHSIG PR_SET_PDEATHSIG decoding.
> +
> +. "${srcdir=.}/init.sh"
> +run_strace_match_diff -a34 -e trace=prctl
> diff --git a/tests/prctl-tsc.c b/tests/prctl-tsc.c
> new file mode 100644
> index 0000000..ea8bf61
> --- /dev/null
> +++ b/tests/prctl-tsc.c
> @@ -0,0 +1,60 @@
> +/*
> + * Copyright (c) 2016 JingPiao Chen <chenjingp...@foxmail.com>
> + * All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + * notice, this list of conditions and the following disclaimer in the
> + * documentation and/or other materials provided with the distribution.
> + * 3. The name of the author may not be used to endorse or promote products
> + * derived from this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE AUTHOR ``AS IS'' AND ANY EXPRESS OR
> + * IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
> + * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED.
> + * IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT,
> + * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
> + * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
> + * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#include "tests.h"
> +#include <asm/unistd.h>
> +
> +#ifdef HAVE_PRCTL
> +# include <sys/prctl.h>
> +#endif
> +
> +#if defined HAVE_PRCTL && defined PR_GET_TSC && defined PR_SET_TSC
> +
> +#include <stdio.h>
> +#include <unistd.h>
> +
> +int
> +main(void)
> +{
> + int tsc;
> + long rc;
> +
> + rc = syscall(__NR_prctl, PR_SET_TSC, PR_TSC_SIGSEGV);
> + printf("prctl(PR_SET_TSC, PR_TSC_SIGSEGV) = %s\n", sprintrc(rc));
> +
> + rc = syscall(__NR_prctl, PR_GET_TSC, &tsc);
> + printf("prctl(PR_GET_TSC, [PR_TSC_SIGSEGV]) = %s\n", sprintrc(rc));
Note that this command is arch-specific and would fail on architectures other
than x86 (in addition to other possibilities of tampering call execution by
various security modules).
> +
> + puts("+++ exited with 0 +++");
> + return 0;
> +}
> +
> +#else
> +
> +SKIP_MAIN_UNDEFINED("HAVE_PRCTL && PR_GET_TSC && PR_SET_TSC")
> +
> +#endif
Notes regarding possible corner cases are generally the same as for the previous
test.
> diff --git a/tests/prctl-tsc.test b/tests/prctl-tsc.test
> new file mode 100755
> index 0000000..c6fe65f
> --- /dev/null
> +++ b/tests/prctl-tsc.test
> @@ -0,0 +1,6 @@
> +#!/bin/sh
> +
> +# Check prctl PR_GET_TSC PR_SET_TSC decoding.
> +
> +. "${srcdir=.}/init.sh"
> +run_strace_match_diff -a36 -e trace=prctl
> --
> 2.7.4
> ------------------------------------------------------------------------------
> _______________________________________________
> Strace-devel mailing list
> Strace-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/strace-devel
------------------------------------------------------------------------------
_______________________________________________
Strace-devel mailing list
Strace-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/strace-devel
