[RFC v1] [GSoC] Add test for -l/--syscall-limit option

[Dmitry V. Levin]

Hi,

On Tue, Feb 21, 2023 at 12:17:09AM +0530, Sahil Siddiq wrote:
> This test checks the correctness of the -l/--syscall-limit
> option when used with the --trace option
> 
> * test/strace--syscall-limit.c: New file
> * test/gen_tests.in: Add rule to generate test
> * test/Makefile.am (check_PROGRAMS): Add strace--syscall-limit
> * test/.gitignore: Add strace--syscall-limit

It's a nice test, it actually demonstrates the aspect of the
implementation I didn't expect, see below.

> Signed-off-by: Sahil Siddiq <icegambit91 at gmail.com>
> ---
> Hi,
> 
> I haven't finished writing all the tests yet. I have to admit
> that I found the learning curve for writing tests to be a lot
> higher than working on the actual feature especially since I
> am not used to working with bash scripts. So, this has been a
> nice learning experience.
> 
> I would like to get some feedback on this test before I move
> on to the other tests. It checks the correctness of the -l
> option with the --trace option.
> 
> I went through several of the existing tests to understand how
> best to write this test. I primarily took inspiration from the
> "syscall--decode-pids-comm" test. Please do let me know if the
> test can be improved.

Sure, see below.

> Thanks,
> Sahil

> 
>  tests/.gitignore              |  1 +
>  tests/Makefile.am             |  1 +
>  tests/gen_tests.in            |  1 +
>  tests/strace--syscall-limit.c | 60 +++++++++++++++++++++++++++++++++++
>  4 files changed, 63 insertions(+)
>  create mode 100644 tests/strace--syscall-limit.c
> 
> diff --git a/tests/.gitignore b/tests/.gitignore
> index 5935c39d9..65999ad24 100644
> --- a/tests/.gitignore
> +++ b/tests/.gitignore
> @@ -1020,6 +1020,7 @@ strace--strings-in-hex-all
>  strace--strings-in-hex-non-ascii
>  strace--strings-in-hex-non-ascii-chars
>  strace--strings-in-hex-none
> +strace--syscall-limit

You keep the list sorted here, thanks.

>  strace-Y-0123456789
>  strace-n
>  strace-no-x
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index 4426cab0c..cc4fae736 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -379,6 +379,7 @@ check_PROGRAMS = $(PURE_EXECUTABLES) \
>  	status-successful-threads \
>  	status-unfinished-threads \
>  	strace--decode-pids-comm \
> +	strace--syscall-limit \

You keep the list sorted here, thanks again.

>  	strace-Y-0123456789 \
>  	strace-p-Y-p2 \
>  	strace-p1-Y-p \
> diff --git a/tests/gen_tests.in b/tests/gen_tests.in
> index eceac8d60..c11c49329 100644
> --- a/tests/gen_tests.in
> +++ b/tests/gen_tests.in
> @@ -995,6 +995,7 @@ strace--absolute-timestamps-format-unix-precision-ns +strace-ttt.test 9 --absolu
>  strace--absolute-timestamps-format-unix-precision-s +strace-ttt.test 0 --absolute-timestamps=format:unix --absolute-timestamps=precision:s
>  strace--absolute-timestamps-format-unix-precision-us +strace-ttt.test 6 --absolute-timestamps=precision:us --absolute-timestamps=format:unix
>  strace--decode-pids-comm	--decode-pids=comm --trace=getppid,tgkill --signal='!SIGCHLD,SIGCONT' -q -f -a 18
> +strace--syscall-limit	--syscall-limit=4 --trace=getpid,getppid -q -f -a 9
>  strace--follow-forks-output-separately +strace-ff.test --follow-forks --output-separately

Please keep this list sorted, too.

>  strace--relative-timestamps +strace-r.test --relative-timestamps
>  strace--relative-timestamps-ms +strace-r.test --relative-timestamps=ms
> diff --git a/tests/strace--syscall-limit.c b/tests/strace--syscall-limit.c
> new file mode 100644
> index 000000000..0905ebd85
> --- /dev/null
> +++ b/tests/strace--syscall-limit.c
> @@ -0,0 +1,60 @@
> +/*
> + * Test -l/--syscall-limit=limit option.
> + *
> + * Copyright (c) 2023 The strace developers.
> + * All rights reserved.
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "tests.h"
> +
> +#include <errno.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include <sys/types.h>
> +#include <sys/wait.h>
> +
> +static int
> +syscall_limit_detach(void)
> +{
> +	pid_t pid  = getpid();
> +	pid_t ppid = getppid();
> +
> +	printf("%u getpid() = %d\n", pid, pid);
> +	printf("%u getppid() = %d\n", pid, ppid);

The format string has to be %-5u instead of plain %u, to match the strace
output.

There is nothing wrong in using two syscalls, but if I were to write a
test like this, I'd be likely using a single syscall instead of two, e.g.
chdir(".").

> +	fflush(stdout);
> +
> +	pid_t child = fork();
> +	if (child < 0)
> +		perror_msg_and_fail("fork");
> +	else if (child == 0) {
> +		pid = getpid();
> +		ppid = getppid();
> +		printf("%u getpid() = %d\n", pid, pid);
> +		printf("%u getppid() = %d\n", pid, ppid);
> +		fflush(stdout);
> +		return 0;
> +	}
> +	int status;
> +	while ((waitpid(child, &status, 0)) != child) {
> +		if (errno == EINTR)
> +			continue;
> +		perror_msg_and_fail("waitpid: %d", child);
> +	}

Note that your --syscall-limit implementation detaches from the parent
process in a way that causes it to be killed by strace with SIGTERM,
so the remaining code is not reached.  I'm not sure you planned to
implement --syscall-limit this way, and I'm doubt that users would
expect such behavior.  This is also in contrast with --detach-on=execve
which also detaches its tracee but doesn't kill it, see detach_on_execve
handling in maybe_allocate_tcb().

> +	printf("+++ exited with 0 +++\n");
> +
> +	ppid = getpid();
> +	ppid = getppid();
> +	printf("%u getpid() = %d\n", pid, pid);
> +	printf("%u getppid() = %d\n", pid, ppid);
> +	printf("+++ exited with 0 +++\n");
> +	return WEXITSTATUS(status);
> +}

So this code works unless executed under strace --syscall-limit.

> +
> +int
> +main(int argc, char **argv)
> +{
> +	return syscall_limit_detach();
> +}

It would be better to declare main without arguments if they are not going
to be used anyway.


-- 
ldv