[PATCH] Add option to trace child program's threads only

[Ákos Uzonyi]

Hi,

I'm not a maintainer, just sharing some thoughts on your patch:

The patch seems to be whitespace corrupted by gmail. Using "git
send-mail" is a good way to avoid this problem.

Note that the clone syscall can be used to create processes too, not
just threads. The documentation of PTRACE_O_TRACECLONE says that it
does not trace all clone calls (it excludes fork and vfork like clone
calls), so it might not be a problem, but I think it's worth thinking
about whether those exceptional cases are enough to filter out
non-thread processes.

It would be nice if the tests you mentioned were integrated in the
strace test suite.

On Wed, 6 Jan 2021 at 17:07, Thomas S.J <sjthomas888 at gmail.com> wrote:
>
> As per issue #141 in Strace's github page, there was a
> requirement/discussion on adding a feature that would allow a user to
> attach strace to all the threads of the tracee program but does not
> trace the child process.
>
> The following patch adds the above described option.
>
> I have executed the following test-cases:
>  1. Usage help
>  2. Traced a program having child process
>  3. Traced a program having only threads
>  4. Traced a program having threads and child process
>  5. Run strace with combination of conflicting options
>
> I have also run the scripts/checkpatch.pl script (from the Linux
> kernel repository) on the patch to verify coding style.
>
> Please review and comment on the diff below.
>
> From bb3635e53beba0b9f1c70b3441cb2fd618f2c058 Mon Sep 17 00:00:00 2001
> From: S J Thomas <sjthomas888 at gmail.com>
> Date: Mon, 4 Jan 2021 20:26:41 +0530
> Subject: [PATCH v5] Add option to follow only threads
>
> ---
>  strace.c | 29 +++++++++++++++++++++++------
>  1 file changed, 23 insertions(+), 6 deletions(-)
>
> diff --git a/strace.c b/strace.c
> index 8adbe45f..99e07902 100644
> --- a/strace.c
> +++ b/strace.c
> @@ -66,6 +66,7 @@ const unsigned int syscall_trap_sig = SIGTRAP | 0x80;
>
>  cflag_t cflag = CFLAG_NONE;
>  bool followfork;
> +bool followthreads;
>  bool output_separately;
>  unsigned int ptrace_setoptions = PTRACE_O_TRACESYSGOOD | PTRACE_O_TRACEEXEC
>   | PTRACE_O_TRACEEXIT;
> @@ -297,6 +298,8 @@ Tracing:\n\
>                   follow forks\n\
>    -ff, --follow-forks --output-separately\n\
>                   follow forks with output into separate files\n\
> +  --follow-threads\n\
> +   follow threads only, not child processes\n\
>    -I INTERRUPTIBLE, --interruptible=INTERRUPTIBLE\n\
>       1, anywhere:   no signals are blocked\n\
>       2, waiting:    fatal signals are blocked while decoding syscall
> (default)\n\
> @@ -1201,7 +1204,7 @@ attach_tcb(struct tcb *const tcp)
>   DIR *dir;
>   unsigned int ntid = 0, nerr = 0;
>
> - if (followfork && tcp->pid != strace_child &&
> + if ((followfork || followthreads) && tcp->pid != strace_child &&
>      xsprintf(procdir, task_path, get_proc_pid(tcp)) > 0 &&
>      (dir = opendir(procdir)) != NULL) {
>   struct_dirent *de;
> @@ -2030,6 +2033,7 @@ init(int argc, char *argv[])
>   GETOPT_DAEMONIZE,
>   GETOPT_HEX_STR,
>   GETOPT_FOLLOWFORKS,
> + GETOPT_FOLLOWTHREADS,
>   GETOPT_OUTPUT_SEPARATELY,
>   GETOPT_TS,
>   GETOPT_PIDNS_TRANSLATION,
> @@ -2060,6 +2064,7 @@ init(int argc, char *argv[])
>   { "daemonized", optional_argument, 0, GETOPT_DAEMONIZE },
>   { "env", required_argument, 0, 'E' },
>   { "follow-forks", no_argument,   0, GETOPT_FOLLOWFORKS },
> + { "follow-threads", no_argument,   0, GETOPT_FOLLOWTHREADS },
>   { "output-separately", no_argument,   0,
>   GETOPT_OUTPUT_SEPARATELY },
>   { "help", no_argument,   0, 'h' },
> @@ -2178,6 +2183,9 @@ init(int argc, char *argv[])
>   case GETOPT_FOLLOWFORKS:
>   followfork = true;
>   break;
> + case GETOPT_FOLLOWTHREADS:
> + followthreads = true;
> + break;
>   case GETOPT_OUTPUT_SEPARATELY:
>   output_separately = true;
>   break;
> @@ -2439,9 +2447,9 @@ init(int argc, char *argv[])
>   if (nprocs && (!argc || debug_flag))
>   error_msg("--seccomp-bpf is not enabled for processes"
>    " attached with -p");
> - if (!followfork) {
> + if (!(followfork || followthreads)) {
>   error_msg("--seccomp-bpf cannot be used without "
> -  "-f/--follow-forks, disabling");
> +  "-f/--follow-forks or --follow-threads, disabling");
>   seccomp_filtering = false;
>   }
>   }
> @@ -2454,6 +2462,13 @@ init(int argc, char *argv[])
>    "please use -f/--follow-forks instead");
>   followfork = true;
>   }
> + if (followthreads) {
> + error_msg("deprecated option -F ignored");
> + } else {
> + error_msg("option -F is deprecated, "
> +  "please use --follow-threads instead");
> + followthreads = true;
> + }
>   }

I think you don't need to set followthreads if the -F option is given.


>   if (output_separately && cflag) {
> @@ -2502,7 +2517,7 @@ init(int argc, char *argv[])
>   }
>
>   if (!outfname) {
> - if (output_separately && !followfork)
> + if (output_separately && !followfork && !followthreads)
>   error_msg("--output-separately has no effect "
>    "without -o/--output");
>   if (open_append)
> @@ -2559,6 +2574,8 @@ init(int argc, char *argv[])
>   ptrace_setoptions |= PTRACE_O_TRACECLONE |
>       PTRACE_O_TRACEFORK |
>       PTRACE_O_TRACEVFORK;
> + if (followthreads)
> + ptrace_setoptions |= PTRACE_O_TRACECLONE;
>
>   if (seccomp_filtering)
>   check_seccomp_filter();
> @@ -2693,7 +2710,7 @@ init(int argc, char *argv[])
>   * -p PID1,PID2: yes (there are already more than one pid)
>   */
>   print_pid_pfx = outfname && !output_separately &&
> - ((followfork && !output_separately) || nprocs > 1);
> + (((followfork || followthreads) && !output_separately) || nprocs > 1);
>  }
>
>  static struct tcb *
> @@ -2806,7 +2823,7 @@ maybe_allocate_tcb(const int pid, int status)
>   }
>   return NULL;
>   }
> - if (followfork) {
> + if (followfork || followthreads) {
>   /* We assume it's a fork/vfork/clone child */
>   struct tcb *tcp = alloctcb(pid);
>   after_successful_attach(tcp, post_attach_sigstop);
> --
> 2.28.0