[PATCH] trace-cmd: document expected behaviour of execvp for record command

[PATCH] trace-cmd: document expected behaviour of execvp for record command

[Paulo Miguel Almeida]

In tracecmd/trace-record.c:<run_cmd>, trace-cmd record -F <executable>
is launched via the libc's execvp() routine.

If you run a command which is meant to be found in one of the entries
of the PATH env var then you should see <n> invocations of the
__x64_sys_execve() syscall where <n> will be equal to the number of
attempts that execvp() routine had done until it could find the
executable.

While the routine works as expected, its behaviour can seem a bit
cryptic for untrained eyes and makes one wonder whether there is
something wrong in any of the parts involved in the tracing operation.
Some time after one digs into trace-cmd's and libc's code base, one
realises that everything is working as expected but documenting it could
save other people's time :-)

Document the expected behaviour ftrace users should see depending on
the way -F <executable> is used.

Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
---
Additional context:

- absolute path example:

	# trace-cmd record -p function_graph \
		-g __x64_sys_execve -O nofuncgraph-irqs \
	        -n __cond_resched --max-graph-depth 1  \
		-F /usr/bin/echo "ftrace" > /dev/null
	
	# trace-cmd report 
	echo-172994 [000] 185539.798539: funcgraph_entry:      ! 803.376 us |  __x64_sys_execve();

- PATH-dependent path example:

	# trace-cmd record -p function_graph \
		-g __x64_sys_execve -O nofuncgraph-irqs \
		-n __cond_resched --max-graph-depth 1  \
		-F echo "ftrace" > /dev/null

	# trace-cmd report
        echo-172656 [002] 185009.671586: funcgraph_entry:      ! 288.732 us |  __x64_sys_execve();
        echo-172656 [002] 185009.671879: funcgraph_entry:      ! 158.337 us |  __x64_sys_execve();
        echo-172656 [002] 185009.672042: funcgraph_entry:      ! 161.843 us |  __x64_sys_execve();
        echo-172656 [002] 185009.672207: funcgraph_entry:      ! 157.656 us |  __x64_sys_execve();
        echo-172656 [002] 185009.672369: funcgraph_entry:      ! 156.343 us |  __x64_sys_execve();
        echo-172656 [002] 185009.672529: funcgraph_entry:      ! 863.629 us |  __x64_sys_execve();

---
 Documentation/trace-cmd/trace-cmd-record.1.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/trace-cmd/trace-cmd-record.1.txt b/Documentation/trace-cmd/trace-cmd-record.1.txt
index b709e48..a000cbe 100644
--- a/Documentation/trace-cmd/trace-cmd-record.1.txt
+++ b/Documentation/trace-cmd/trace-cmd-record.1.txt
@@ -113,6 +113,10 @@ OPTIONS
     Using *-F* will let you trace only events that are caused by the given
     command.
 
+    Note: if the specified filename is neither absolute or relative then libc
+    will invoke execve() syscall for every entry in the colon-separated list of
+    directory pathnames specified in the PATH environment variable.
+
 *-P* 'pid'::
      Similar to *-F* but lets you specify a process ID to trace.
 
-- 
2.38.1

Re: [PATCH] trace-cmd: document expected behaviour of execvp for record command

[Steven Rostedt]

On Fri, 6 Jan 2023 11:52:19 +1300
Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com> wrote:

> In tracecmd/trace-record.c:<run_cmd>, trace-cmd record -F <executable>
> is launched via the libc's execvp() routine.
> 
> If you run a command which is meant to be found in one of the entries
> of the PATH env var then you should see <n> invocations of the
> __x64_sys_execve() syscall where <n> will be equal to the number of
> attempts that execvp() routine had done until it could find the
> executable.
> 
> While the routine works as expected, its behaviour can seem a bit
> cryptic for untrained eyes and makes one wonder whether there is
> something wrong in any of the parts involved in the tracing operation.
> Some time after one digs into trace-cmd's and libc's code base, one
> realises that everything is working as expected but documenting it could
> save other people's time :-)
> 
> Document the expected behaviour ftrace users should see depending on
> the way -F <executable> is used.
> 
> Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
> ---
> Additional context:
> 
> - absolute path example:
> 
> 	# trace-cmd record -p function_graph \
> 		-g __x64_sys_execve -O nofuncgraph-irqs \
> 	        -n __cond_resched --max-graph-depth 1  \
> 		-F /usr/bin/echo "ftrace" > /dev/null
> 	
> 	# trace-cmd report 
> 	echo-172994 [000] 185539.798539: funcgraph_entry:      ! 803.376 us |  __x64_sys_execve();
> 
> - PATH-dependent path example:
> 
> 	# trace-cmd record -p function_graph \
> 		-g __x64_sys_execve -O nofuncgraph-irqs \
> 		-n __cond_resched --max-graph-depth 1  \
> 		-F echo "ftrace" > /dev/null
> 
> 	# trace-cmd report
>         echo-172656 [002] 185009.671586: funcgraph_entry:      ! 288.732 us |  __x64_sys_execve();
>         echo-172656 [002] 185009.671879: funcgraph_entry:      ! 158.337 us |  __x64_sys_execve();
>         echo-172656 [002] 185009.672042: funcgraph_entry:      ! 161.843 us |  __x64_sys_execve();
>         echo-172656 [002] 185009.672207: funcgraph_entry:      ! 157.656 us |  __x64_sys_execve();
>         echo-172656 [002] 185009.672369: funcgraph_entry:      ! 156.343 us |  __x64_sys_execve();
>         echo-172656 [002] 185009.672529: funcgraph_entry:      ! 863.629 us |  __x64_sys_execve();

Honestly, the above should be in the change log.

> 
> ---
>  Documentation/trace-cmd/trace-cmd-record.1.txt | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/trace-cmd/trace-cmd-record.1.txt b/Documentation/trace-cmd/trace-cmd-record.1.txt
> index b709e48..a000cbe 100644
> --- a/Documentation/trace-cmd/trace-cmd-record.1.txt
> +++ b/Documentation/trace-cmd/trace-cmd-record.1.txt
> @@ -113,6 +113,10 @@ OPTIONS
>      Using *-F* will let you trace only events that are caused by the given
>      command.
>  
> +    Note: if the specified filename is neither absolute or relative then libc
> +    will invoke execve() syscall for every entry in the colon-separated list of
> +    directory pathnames specified in the PATH environment variable.
> +

Hmm, do you think it may be worth open-coding execvp() and looking for it
from trace-cmd, and then only enabling tracing when it found the full path?

-- Steve


>  *-P* 'pid'::
>       Similar to *-F* but lets you specify a process ID to trace.
>  

Re: [PATCH] trace-cmd: document expected behaviour of execvp for record command

[Paulo Miguel Almeida]

On Thu, Jan 05, 2023 at 06:13:17PM -0500, Steven Rostedt wrote:
> On Fri, 6 Jan 2023 11:52:19 +1300
> Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com> wrote:
> 
> > In tracecmd/trace-record.c:<run_cmd>, trace-cmd record -F <executable>
> > is launched via the libc's execvp() routine.
> > 
> > If you run a command which is meant to be found in one of the entries
> > of the PATH env var then you should see <n> invocations of the
> > __x64_sys_execve() syscall where <n> will be equal to the number of
> > attempts that execvp() routine had done until it could find the
> > executable.
> > 
> > While the routine works as expected, its behaviour can seem a bit
> > cryptic for untrained eyes and makes one wonder whether there is
> > something wrong in any of the parts involved in the tracing operation.
> > Some time after one digs into trace-cmd's and libc's code base, one
> > realises that everything is working as expected but documenting it could
> > save other people's time :-)
> > 
> > Document the expected behaviour ftrace users should see depending on
> > the way -F <executable> is used.
> > 
> > Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
> > ---
> > Additional context:
> > 
> > - absolute path example:
> > 
> > 	# trace-cmd record -p function_graph \
> > 		-g __x64_sys_execve -O nofuncgraph-irqs \
> > 	        -n __cond_resched --max-graph-depth 1  \
> > 		-F /usr/bin/echo "ftrace" > /dev/null
> > 	
> > 	# trace-cmd report 
> > 	echo-172994 [000] 185539.798539: funcgraph_entry:      ! 803.376 us |  __x64_sys_execve();
> > 
> > - PATH-dependent path example:
> > 
> > 	# trace-cmd record -p function_graph \
> > 		-g __x64_sys_execve -O nofuncgraph-irqs \
> > 		-n __cond_resched --max-graph-depth 1  \
> > 		-F echo "ftrace" > /dev/null
> > 
> > 	# trace-cmd report
> >         echo-172656 [002] 185009.671586: funcgraph_entry:      ! 288.732 us |  __x64_sys_execve();
> >         echo-172656 [002] 185009.671879: funcgraph_entry:      ! 158.337 us |  __x64_sys_execve();
> >         echo-172656 [002] 185009.672042: funcgraph_entry:      ! 161.843 us |  __x64_sys_execve();
> >         echo-172656 [002] 185009.672207: funcgraph_entry:      ! 157.656 us |  __x64_sys_execve();
> >         echo-172656 [002] 185009.672369: funcgraph_entry:      ! 156.343 us |  __x64_sys_execve();
> >         echo-172656 [002] 185009.672529: funcgraph_entry:      ! 863.629 us |  __x64_sys_execve();
> 
> Honestly, the above should be in the change log.

Fair enough. I will send a new patch shortly.

> 
> > 
> > ---
> >  Documentation/trace-cmd/trace-cmd-record.1.txt | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/Documentation/trace-cmd/trace-cmd-record.1.txt b/Documentation/trace-cmd/trace-cmd-record.1.txt
> > index b709e48..a000cbe 100644
> > --- a/Documentation/trace-cmd/trace-cmd-record.1.txt
> > +++ b/Documentation/trace-cmd/trace-cmd-record.1.txt
> > @@ -113,6 +113,10 @@ OPTIONS
> >      Using *-F* will let you trace only events that are caused by the given
> >      command.
> >  
> > +    Note: if the specified filename is neither absolute or relative then libc
> > +    will invoke execve() syscall for every entry in the colon-separated list of
> > +    directory pathnames specified in the PATH environment variable.
> > +
> 
> Hmm, do you think it may be worth open-coding execvp() and looking for it
> from trace-cmd, and then only enabling tracing when it found the full path?
> 
> -- Steve

That's a good point. I thought about it for a while and this is really a pickle.

I'll dump the things I'm thinking about and peharps you can help me
choosing between the approaches :-)

Launching executables via:

libc->execvp: It avoid some sorts of TOCTOU race conditions by trying to
execve executables and leave it up to the kernel to do a 'single'
file-exists validation (best case scenario) instead of validate whether
file exists twice, once in user-space and again in kernel (best case
scenario).

bash: As a counter point (and also because that's most
likely how trace-cmd will be executed). Bash actually does the
open-coding execvp() approach as shown below:

# trace-cmd record -p function_graph \
	-g __x64_sys_execve -O nofuncgraph-irqs \
	--max-graph-depth 1  \
	-F /usr/bin/bash -c "ls" > /dev/null


ls-178525 [010] 197387.772776: funcgraph_entry:      ! 775.074 us |  __x64_sys_execve(); # for /usr/bin/bash
ls-178525 [010] 197387.775568: funcgraph_entry:      ! 993.453 us |  __x64_sys_execve(); # for /usr/bin/ls

I guess this will boil down to whether you want to make trace-cmd as
'transparent' as possible (in comparison to running the command without
prepending trace-cmd) OR if you actually care about/want to abide by
the TOCTOU 'guarantees' that libc implements for userspace applications

Let me know your thoughts. I'm flexible with either approach

- Paulo Almeida

> 
> 
> >  *-P* 'pid'::
> >       Similar to *-F* but lets you specify a process ID to trace.
> >  
> 

Re: [PATCH] trace-cmd: document expected behaviour of execvp for record command

[Steven Rostedt]

On Fri, 6 Jan 2023 15:09:58 +1300
Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com> wrote:

> That's a good point. I thought about it for a while and this is really a pickle.
> 
> I'll dump the things I'm thinking about and peharps you can help me
> choosing between the approaches :-)
> 
> Launching executables via:
> 
> libc->execvp: It avoid some sorts of TOCTOU race conditions by trying to
> execve executables and leave it up to the kernel to do a 'single'
> file-exists validation (best case scenario) instead of validate whether
> file exists twice, once in user-space and again in kernel (best case
> scenario).
> 
> bash: As a counter point (and also because that's most
> likely how trace-cmd will be executed). Bash actually does the
> open-coding execvp() approach as shown below:
> 
> # trace-cmd record -p function_graph \
> 	-g __x64_sys_execve -O nofuncgraph-irqs \
> 	--max-graph-depth 1  \
> 	-F /usr/bin/bash -c "ls" > /dev/null
> 
> 
> ls-178525 [010] 197387.772776: funcgraph_entry:      ! 775.074 us |  __x64_sys_execve(); # for /usr/bin/bash
> ls-178525 [010] 197387.775568: funcgraph_entry:      ! 993.453 us |  __x64_sys_execve(); # for /usr/bin/ls
> 
> I guess this will boil down to whether you want to make trace-cmd as
> 'transparent' as possible (in comparison to running the command without
> prepending trace-cmd) OR if you actually care about/want to abide by
> the TOCTOU 'guarantees' that libc implements for userspace applications
> 
> Let me know your thoughts. I'm flexible with either approach

I think I rather do it the bash way. I don't believe TOCTOU is important
here.

-- Steve

[PATCH v2] trace-cmd: open code execvp routine to avoid multiple execve syscalls

[Paulo Miguel Almeida]

In tracecmd/trace-record.c:<run_cmd>, trace-cmd record -F <executable>
is launched via the libc's execvp() routine. The way that execvp() routine
works is by invoking execve syscall for every entry on the $PATH if
command specified is neither absolute nor relative which can come across
as a bit cryptic to untrained eyes.

- absolute path example:

        # trace-cmd record -p function_graph \
                -g __x64_sys_execve -O nofuncgraph-irqs \
                -n __cond_resched --max-graph-depth 1  \
                -F /usr/bin/echo "ftrace" > /dev/null

        # trace-cmd report
        echo-172994 [000] 185539.798539: funcgraph_entry:      ! 803.376 us |  __x64_sys_execve();

- PATH-dependent path example:

        # trace-cmd record -p function_graph \
                -g __x64_sys_execve -O nofuncgraph-irqs \
                -n __cond_resched --max-graph-depth 1  \
                -F echo "ftrace" > /dev/null

        # trace-cmd report
        echo-172656 [002] 185009.671586: funcgraph_entry:      ! 288.732 us |  __x64_sys_execve();
        echo-172656 [002] 185009.671879: funcgraph_entry:      ! 158.337 us |  __x64_sys_execve();
        echo-172656 [002] 185009.672042: funcgraph_entry:      ! 161.843 us |  __x64_sys_execve();
        echo-172656 [002] 185009.672207: funcgraph_entry:      ! 157.656 us |  __x64_sys_execve();
        echo-172656 [002] 185009.672369: funcgraph_entry:      ! 156.343 us |  __x64_sys_execve();
        echo-172656 [002] 185009.672529: funcgraph_entry:      ! 863.629 us |  __x64_sys_execve();

Open code the libc's execvp routine into trace-cmd so ftrace will only
start recording once the command is found when it needs to be found in
PATH.

Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
---
Changelog:

- v2: open code execvp routine into trace-cmd. (Req. Steve Rostedt)
- v1: https://lore.kernel.org/linux-trace-devel/Y7dUo6woh9Y31cdl@mail.google.com/
---
 tracecmd/trace-record.c | 59 ++++++++++++++++++++++++++++++++++++-----
 1 file changed, 53 insertions(+), 6 deletions(-)

diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
index 7f0cebe..4a54637 100644
--- a/tracecmd/trace-record.c
+++ b/tracecmd/trace-record.c
@@ -1683,6 +1683,58 @@ static int change_user(const char *user)
 	return 0;
 }
 
+static void execute_program(int argc, char **argv)
+{
+	char buf[PATH_MAX + NAME_MAX + 1];	
+	char *path_env;
+	size_t path_len;
+	size_t entry_len;
+	char *ptr_start;
+	char *ptr_end;
+
+	/*
+	 * if command specified by user is neither absolute nor 
+	 * relative than we search for it in $PATH. 
+	 */
+	if (!strchr(argv[0], '/') && !strchr(argv[0], '.')) {
+		path_env = getenv("PATH");
+		path_len = strlen(path_env);
+		ptr_start = path_env;
+
+		while ((ptr_start - path_env) < path_len) {
+			ptr_end = strchr(ptr_start, ':');
+			
+			/* single entry in PATH? */
+			if (!ptr_end)
+				entry_len = path_len;
+			else
+				entry_len = ptr_end - ptr_start;
+
+			strncpy(buf, ptr_start, entry_len);
+
+			if (buf[entry_len - 1] != '/')
+				buf[entry_len++] = '/';
+			
+			strncpy(buf + entry_len, argv[0], sizeof(buf) - entry_len);
+
+			/* does it exist and can we execute it? */
+			if (access(buf, X_OK) == 0)
+				break;
+
+			ptr_start = ptr_end + 1;
+		}
+	} else {
+		strncpy(buf, argv[0], sizeof(buf));
+	}
+
+	if (execve(buf, argv, environ)) {
+		fprintf(stderr, "\n********************\n");
+		fprintf(stderr, " Unable to exec %s\n", argv[0]);
+		fprintf(stderr, "********************\n");
+		die("Failed to exec %s", argv[0]);
+	}
+}
+
 static void run_cmd(enum trace_type type, const char *user, int argc, char **argv)
 {
 	int status;
@@ -1709,12 +1761,7 @@ static void run_cmd(enum trace_type type, const char *user, int argc, char **arg
 		if (change_user(user) < 0)
 			die("Failed to change user to %s", user);
 
-		if (execvp(argv[0], argv)) {
-			fprintf(stderr, "\n********************\n");
-			fprintf(stderr, " Unable to exec %s\n", argv[0]);
-			fprintf(stderr, "********************\n");
-			die("Failed to exec %s", argv[0]);
-		}
+		execute_program(argc, argv);
 	}
 	if (fork_process)
 		exit(0);
-- 
2.38.1

Re: [PATCH v2] trace-cmd: open code execvp routine to avoid multiple execve syscalls

[Paulo Miguel Almeida]

On Sat, Jan 14, 2023 at 11:58:41AM +1300, Paulo Miguel Almeida wrote:
> In tracecmd/trace-record.c:<run_cmd>, trace-cmd record -F <executable>
> is launched via the libc's execvp() routine. The way that execvp() routine
> works is by invoking execve syscall for every entry on the $PATH if
> command specified is neither absolute nor relative which can come across
> as a bit cryptic to untrained eyes.
> 
> - absolute path example:
> 
>         # trace-cmd record -p function_graph \
>                 -g __x64_sys_execve -O nofuncgraph-irqs \
>                 -n __cond_resched --max-graph-depth 1  \
>                 -F /usr/bin/echo "ftrace" > /dev/null
> 
>         # trace-cmd report
>         echo-172994 [000] 185539.798539: funcgraph_entry:      ! 803.376 us |  __x64_sys_execve();
> 
> - PATH-dependent path example:
> 
>         # trace-cmd record -p function_graph \
>                 -g __x64_sys_execve -O nofuncgraph-irqs \
>                 -n __cond_resched --max-graph-depth 1  \
>                 -F echo "ftrace" > /dev/null
> 
>         # trace-cmd report
>         echo-172656 [002] 185009.671586: funcgraph_entry:      ! 288.732 us |  __x64_sys_execve();
>         echo-172656 [002] 185009.671879: funcgraph_entry:      ! 158.337 us |  __x64_sys_execve();
>         echo-172656 [002] 185009.672042: funcgraph_entry:      ! 161.843 us |  __x64_sys_execve();
>         echo-172656 [002] 185009.672207: funcgraph_entry:      ! 157.656 us |  __x64_sys_execve();
>         echo-172656 [002] 185009.672369: funcgraph_entry:      ! 156.343 us |  __x64_sys_execve();
>         echo-172656 [002] 185009.672529: funcgraph_entry:      ! 863.629 us |  __x64_sys_execve();
> 
> Open code the libc's execvp routine into trace-cmd so ftrace will only
> start recording once the command is found when it needs to be found in
> PATH.
> 
> Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
> ---
> Changelog:
> 
> - v2: open code execvp routine into trace-cmd. (Req. Steve Rostedt)
> - v1: https://lore.kernel.org/linux-trace-devel/Y7dUo6woh9Y31cdl@mail.google.com/
> ---

Ignore this patch. I just realised that I didn't tweak the CUnit tests.
I will submit another patch shortly.

- Paulo A.

Re: [PATCH v2] trace-cmd: open code execvp routine to avoid multiple execve syscalls

[Paulo Miguel Almeida]

On Sat, Jan 14, 2023 at 12:05:44PM +1300, Paulo Miguel Almeida wrote:
> On Sat, Jan 14, 2023 at 11:58:41AM +1300, Paulo Miguel Almeida wrote:
> > In tracecmd/trace-record.c:<run_cmd>, trace-cmd record -F <executable>
> > is launched via the libc's execvp() routine. The way that execvp() routine
> > works is by invoking execve syscall for every entry on the $PATH if
> > command specified is neither absolute nor relative which can come across
> > as a bit cryptic to untrained eyes.
> > 
> > - absolute path example:
> > 
> >         # trace-cmd record -p function_graph \
> >                 -g __x64_sys_execve -O nofuncgraph-irqs \
> >                 -n __cond_resched --max-graph-depth 1  \
> >                 -F /usr/bin/echo "ftrace" > /dev/null
> > 
> >         # trace-cmd report
> >         echo-172994 [000] 185539.798539: funcgraph_entry:      ! 803.376 us |  __x64_sys_execve();
> > 
> > - PATH-dependent path example:
> > 
> >         # trace-cmd record -p function_graph \
> >                 -g __x64_sys_execve -O nofuncgraph-irqs \
> >                 -n __cond_resched --max-graph-depth 1  \
> >                 -F echo "ftrace" > /dev/null
> > 
> >         # trace-cmd report
> >         echo-172656 [002] 185009.671586: funcgraph_entry:      ! 288.732 us |  __x64_sys_execve();
> >         echo-172656 [002] 185009.671879: funcgraph_entry:      ! 158.337 us |  __x64_sys_execve();
> >         echo-172656 [002] 185009.672042: funcgraph_entry:      ! 161.843 us |  __x64_sys_execve();
> >         echo-172656 [002] 185009.672207: funcgraph_entry:      ! 157.656 us |  __x64_sys_execve();
> >         echo-172656 [002] 185009.672369: funcgraph_entry:      ! 156.343 us |  __x64_sys_execve();
> >         echo-172656 [002] 185009.672529: funcgraph_entry:      ! 863.629 us |  __x64_sys_execve();
> > 
> > Open code the libc's execvp routine into trace-cmd so ftrace will only
> > start recording once the command is found when it needs to be found in
> > PATH.
> > 
> > Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
> > ---
> > Changelog:
> > 
> > - v2: open code execvp routine into trace-cmd. (Req. Steve Rostedt)
> > - v1: https://lore.kernel.org/linux-trace-devel/Y7dUo6woh9Y31cdl@mail.google.com/
> > ---
> 
> Ignore this patch. I just realised that I didn't tweak the CUnit tests.
> I will submit another patch shortly.
> 
> - Paulo A.

False alarm Steve. You can review this patch as it is :-)

The error I was getting before:

---
Suite: trace-cmd
  Test: Simple record and report ...passed
  Test: Test convert from v7 to v6 ...passed
  Test: Use libraries to read file ...FAILED
    1. tracecmd-utest.c:441  - data.counter > 0
  Test: Test max length ...passed

Run Summary:    Type  Total    Ran Passed Failed Inactive
              suites      1      1    n/a      0        0
               tests      4      4      3      1        0
             asserts     24     24     23      1      n/a

Elapsed time =    0.193 seconds
---

After git bisect'ing the error I realised that my commit has nothing
to do with the CUnit test failure given that this particular test case
has been failing for me since its introduction to the code base on
commit <d83b6628927326d158>.

Regardless of what fix it requires, I'm assuming that would be done
in a different patch anyway. So as soon as this one is reviewed and
merged, I'm happy to further investigate the other :-)

thanks!

- Paulo A.

Re: [PATCH v2] trace-cmd: open code execvp routine to avoid multiple execve syscalls

[Steven Rostedt]

On Sat, 14 Jan 2023 11:58:41 +1300
Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com> wrote:

Hi Paulo,

A couple of nits about submitting a follow up patch.

1) A second patch should always start a new thread. It's easier to find
in inboxes.

If you want, you could add a link to the first thread in the "changes"
section (see below).

2) Please start the subject with a capital letter:

[PATCH v2] trace-cmd: Open code execvp routine to avoid multiple execve syscalls


> In tracecmd/trace-record.c:<run_cmd>, trace-cmd record -F <executable>
> is launched via the libc's execvp() routine. The way that execvp() routine
> works is by invoking execve syscall for every entry on the $PATH if
> command specified is neither absolute nor relative which can come across
> as a bit cryptic to untrained eyes.
> 
> - absolute path example:
> 
>         # trace-cmd record -p function_graph \
>                 -g __x64_sys_execve -O nofuncgraph-irqs \
>                 -n __cond_resched --max-graph-depth 1  \
>                 -F /usr/bin/echo "ftrace" > /dev/null
> 
>         # trace-cmd report
>         echo-172994 [000] 185539.798539: funcgraph_entry:      ! 803.376 us |  __x64_sys_execve();
> 
> - PATH-dependent path example:
> 
>         # trace-cmd record -p function_graph \
>                 -g __x64_sys_execve -O nofuncgraph-irqs \
>                 -n __cond_resched --max-graph-depth 1  \
>                 -F echo "ftrace" > /dev/null
> 
>         # trace-cmd report
>         echo-172656 [002] 185009.671586: funcgraph_entry:      ! 288.732 us |  __x64_sys_execve();
>         echo-172656 [002] 185009.671879: funcgraph_entry:      ! 158.337 us |  __x64_sys_execve();
>         echo-172656 [002] 185009.672042: funcgraph_entry:      ! 161.843 us |  __x64_sys_execve();
>         echo-172656 [002] 185009.672207: funcgraph_entry:      ! 157.656 us |  __x64_sys_execve();
>         echo-172656 [002] 185009.672369: funcgraph_entry:      ! 156.343 us |  __x64_sys_execve();
>         echo-172656 [002] 185009.672529: funcgraph_entry:      ! 863.629 us |  __x64_sys_execve();
> 
> Open code the libc's execvp routine into trace-cmd so ftrace will only
> start recording once the command is found when it needs to be found in
> PATH.
> 
> Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
> ---
> Changelog:
> 
> - v2: open code execvp routine into trace-cmd. (Req. Steve Rostedt)
> - v1: https://lore.kernel.org/linux-trace-devel/Y7dUo6woh9Y31cdl@mail.google.com/
> ---
>  tracecmd/trace-record.c | 59 ++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 53 insertions(+), 6 deletions(-)
> 
> diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
> index 7f0cebe..4a54637 100644
> --- a/tracecmd/trace-record.c
> +++ b/tracecmd/trace-record.c
> @@ -1683,6 +1683,58 @@ static int change_user(const char *user)
>  	return 0;
>  }
>  
> +static void execute_program(int argc, char **argv)
> +{
> +	char buf[PATH_MAX + NAME_MAX + 1];	
> +	char *path_env;
> +	size_t path_len;
> +	size_t entry_len;
> +	char *ptr_start;
> +	char *ptr_end;
> +
> +	/*
> +	 * if command specified by user is neither absolute nor 
> +	 * relative than we search for it in $PATH. 
> +	 */
> +	if (!strchr(argv[0], '/') && !strchr(argv[0], '.')) {

Why the search of '.'? If you have an executable called:

   my.exec

Wouldn't that be found?

Can you have a relative path without '/'? Usually, you would do:

  ./exec

> +		path_env = getenv("PATH");

Need to check for NULL, in the rare case that no "PATH" is defined.

> +		path_len = strlen(path_env);
> +		ptr_start = path_env;
> +
> +		while ((ptr_start - path_env) < path_len) {
> +			ptr_end = strchr(ptr_start, ':');

Why not just use strtok_r() here?

Something like (untested):

		char *saveptr;

		for (path = strtok_r(path_env, ":", &saveptr);
		     path; path = strtok_r(NULL, ":", &saveptr) {

			snprintf(buf, PATH_MAX, "%s/%s", path, argv[0]);

			if (access(buf, X_OK) == 0)
				break;
		}

> +			
> +			/* single entry in PATH? */
> +			if (!ptr_end)
> +				entry_len = path_len;
> +			else
> +				entry_len = ptr_end - ptr_start;
> +
> +			strncpy(buf, ptr_start, entry_len);
> +
> +			if (buf[entry_len - 1] != '/')
> +				buf[entry_len++] = '/';
> +			
> +			strncpy(buf + entry_len, argv[0], sizeof(buf) - entry_len);
> +
> +			/* does it exist and can we execute it? */
> +			if (access(buf, X_OK) == 0)
> +				break;
> +
> +			ptr_start = ptr_end + 1;
> +		}
> +	} else {
> +		strncpy(buf, argv[0], sizeof(buf));
> +	}

Don't we want to enable tracing here?

-- Steve

> +
> +	if (execve(buf, argv, environ)) {
> +		fprintf(stderr, "\n********************\n");
> +		fprintf(stderr, " Unable to exec %s\n", argv[0]);
> +		fprintf(stderr, "********************\n");
> +		die("Failed to exec %s", argv[0]);
> +	}
> +}
> +
>  static void run_cmd(enum trace_type type, const char *user, int argc, char **argv)
>  {
>  	int status;
> @@ -1709,12 +1761,7 @@ static void run_cmd(enum trace_type type, const char *user, int argc, char **arg
>  		if (change_user(user) < 0)
>  			die("Failed to change user to %s", user);
>  
> -		if (execvp(argv[0], argv)) {
> -			fprintf(stderr, "\n********************\n");
> -			fprintf(stderr, " Unable to exec %s\n", argv[0]);
> -			fprintf(stderr, "********************\n");
> -			die("Failed to exec %s", argv[0]);
> -		}
> +		execute_program(argc, argv);
>  	}
>  	if (fork_process)
>  		exit(0);

Re: [PATCH v2] trace-cmd: open code execvp routine to avoid multiple execve syscalls

[Steven Rostedt]

On Sat, 14 Jan 2023 00:51:05 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:
> Hi Paulo,
> 
> A couple of nits about submitting a follow up patch.
> 
> 1) A second patch should always start a new thread. It's easier to find
> in inboxes.
> 
> If you want, you could add a link to the first thread in the "changes"
> section (see below).

And I forgot to update that "see below" comment.

[..]

> > Open code the libc's execvp routine into trace-cmd so ftrace will only
> > start recording once the command is found when it needs to be found in
> > PATH.
> > 
> > Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
> > ---
> > Changelog:
> > 
> > - v2: open code execvp routine into trace-cmd. (Req. Steve Rostedt)
> > - v1: https://lore.kernel.org/linux-trace-devel/Y7dUo6woh9Y31cdl@mail.google.com/

Because you did what I was going to say and I did not update the
previous statement ;-)

-- Steve