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

[PATCH v3] 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:

- v3: simplify path traversal code via strtok_r. (Req. Steven Rostedt)
- v2: open code execvp routine into trace-cmd. (Req. Steven Rostedt)
- v1: https://lore.kernel.org/linux-trace-devel/Y7dUo6woh9Y31cdl@mail.google.com/
---
 tracecmd/trace-record.c | 48 +++++++++++++++++++++++++++++++++++------
 1 file changed, 41 insertions(+), 7 deletions(-)

diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
index 7f0cebe..c111ef6 100644
--- a/tracecmd/trace-record.c
+++ b/tracecmd/trace-record.c
@@ -1683,6 +1683,46 @@ 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;
+	char *entry;
+	char *saveptr;
+
+	/*
+	 * if command specified by user is neither absolute nor
+	 * relative than we search for it in $PATH.
+	 */
+	if (*argv[0] != '/' && *argv[0] != '.') {
+		path = getenv("PATH");
+
+		if (!path)
+			die("can't search for '%s' if $PATH is NULL", argv[0]);
+
+		for (entry = strtok_r(path, ":", &saveptr);
+		     entry; entry = strtok_r(NULL, ":", &saveptr)) {
+
+			snprintf(buf, sizeof(buf), "%s/%s", entry, argv[0]);
+
+			/* does it exist and can we execute it? */
+			if (access(buf, X_OK) == 0)
+				break;
+
+		}
+	} else {
+		strncpy(buf, argv[0], sizeof(buf));
+	}
+
+	tracecmd_enable_tracing();
+	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;
@@ -1693,7 +1733,6 @@ static void run_cmd(enum trace_type type, const char *user, int argc, char **arg
 	if (!pid) {
 		/* child */
 		update_task_filter();
-		tracecmd_enable_tracing();
 		if (!fork_process)
 			enable_ptrace();
 		/*
@@ -1709,12 +1748,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 v3] trace-cmd: Open code execvp routine to avoid multiple execve syscalls

[Steven Rostedt]

On Sat, 21 Jan 2023 18:50:29 +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. 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:
> 
> - v3: simplify path traversal code via strtok_r. (Req. Steven Rostedt)
> - v2: open code execvp routine into trace-cmd. (Req. Steven Rostedt)
> - v1: https://lore.kernel.org/linux-trace-devel/Y7dUo6woh9Y31cdl@mail.google.com/
> ---
>  tracecmd/trace-record.c | 48 +++++++++++++++++++++++++++++++++++------
>  1 file changed, 41 insertions(+), 7 deletions(-)
> 
> diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
> index 7f0cebe..c111ef6 100644
> --- a/tracecmd/trace-record.c
> +++ b/tracecmd/trace-record.c
> @@ -1683,6 +1683,46 @@ 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;
> +	char *entry;
> +	char *saveptr;
> +
> +	/*
> +	 * if command specified by user is neither absolute nor
> +	 * relative than we search for it in $PATH.
> +	 */
> +	if (*argv[0] != '/' && *argv[0] != '.') {

All you really need to do is check if '/' exists in the name, as the path
lookup in bash will not add PATH in such cases:

 # mkdir /usr/bin/direct
 # cp /usr/bin/grep /usr/bin/direct
 # direct/grep
-bash: direct/grep: No such file or directory

So, the above really has to be just "if (strchr(argv[0], '/') == NULL)"

No need to look for '.'. As you could an application that starts with "."
and it should not think it's a path name in such cases.

In fact, if you have a '/' in the name. It will use relative paths.

 # mv /usr/bin/direct .
 # direct/grep -h
Usage: grep [OPTION]... PATTERNS [FILE]...
Try 'grep --help' for more information.

So appending PATH in such cases is incorrect, and will produce unexpected
results.

-- Steve

> +		path = getenv("PATH");
> +
> +		if (!path)
> +			die("can't search for '%s' if $PATH is NULL", argv[0]);
> +
> +		for (entry = strtok_r(path, ":", &saveptr);
> +		     entry; entry = strtok_r(NULL, ":", &saveptr)) {
> +
> +			snprintf(buf, sizeof(buf), "%s/%s", entry, argv[0]);
> +
> +			/* does it exist and can we execute it? */
> +			if (access(buf, X_OK) == 0)
> +				break;
> +
> +		}
> +	} else {
> +		strncpy(buf, argv[0], sizeof(buf));
> +	}
> +
> +	tracecmd_enable_tracing();
> +	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;
> @@ -1693,7 +1733,6 @@ static void run_cmd(enum trace_type type, const char *user, int argc, char **arg
>  	if (!pid) {
>  		/* child */
>  		update_task_filter();
> -		tracecmd_enable_tracing();
>  		if (!fork_process)
>  			enable_ptrace();
>  		/*
> @@ -1709,12 +1748,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 v3] trace-cmd: Open code execvp routine to avoid multiple execve syscalls

[Paulo Miguel Almeida]

On Sat, Jan 21, 2023 at 12:22:54PM -0500, Steven Rostedt wrote:
> On Sat, 21 Jan 2023 18:50:29 +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. 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:
> > 
> > - v3: simplify path traversal code via strtok_r. (Req. Steven Rostedt)
> > - v2: open code execvp routine into trace-cmd. (Req. Steven Rostedt)
> > - v1: https://lore.kernel.org/linux-trace-devel/Y7dUo6woh9Y31cdl@mail.google.com/
> > ---
> >  tracecmd/trace-record.c | 48 +++++++++++++++++++++++++++++++++++------
> >  1 file changed, 41 insertions(+), 7 deletions(-)
> > 
> > diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
> > index 7f0cebe..c111ef6 100644
> > --- a/tracecmd/trace-record.c
> > +++ b/tracecmd/trace-record.c
> > @@ -1683,6 +1683,46 @@ 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;
> > +	char *entry;
> > +	char *saveptr;
> > +
> > +	/*
> > +	 * if command specified by user is neither absolute nor
> > +	 * relative than we search for it in $PATH.
> > +	 */
> > +	if (*argv[0] != '/' && *argv[0] != '.') {
> 
> All you really need to do is check if '/' exists in the name, as the path
> lookup in bash will not add PATH in such cases:
> 
>  # mkdir /usr/bin/direct
>  # cp /usr/bin/grep /usr/bin/direct
>  # direct/grep
> -bash: direct/grep: No such file or directory
> 
> So, the above really has to be just "if (strchr(argv[0], '/') == NULL)"
> 
> No need to look for '.'. As you could an application that starts with "."
> and it should not think it's a path name in such cases.
> 
> In fact, if you have a '/' in the name. It will use relative paths.
> 
>  # mv /usr/bin/direct .
>  # direct/grep -h
> Usage: grep [OPTION]... PATTERNS [FILE]...
> Try 'grep --help' for more information.
> 
> So appending PATH in such cases is incorrect, and will produce unexpected
> results.
> 
> -- Steve

You are right Steve, that case slipped through the cracks somehow. good catch :)

I will submit a new one shortly.

thanks!

- Paulo A.

> 
> > +		path = getenv("PATH");
> > +
> > +		if (!path)
> > +			die("can't search for '%s' if $PATH is NULL", argv[0]);
> > +
> > +		for (entry = strtok_r(path, ":", &saveptr);
> > +		     entry; entry = strtok_r(NULL, ":", &saveptr)) {
> > +
> > +			snprintf(buf, sizeof(buf), "%s/%s", entry, argv[0]);
> > +
> > +			/* does it exist and can we execute it? */
> > +			if (access(buf, X_OK) == 0)
> > +				break;
> > +
> > +		}
> > +	} else {
> > +		strncpy(buf, argv[0], sizeof(buf));
> > +	}
> > +
> > +	tracecmd_enable_tracing();
> > +	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;
> > @@ -1693,7 +1733,6 @@ static void run_cmd(enum trace_type type, const char *user, int argc, char **arg
> >  	if (!pid) {
> >  		/* child */
> >  		update_task_filter();
> > -		tracecmd_enable_tracing();
> >  		if (!fork_process)
> >  			enable_ptrace();
> >  		/*
> > @@ -1709,12 +1748,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);
>