| Message ID | Y8t9JbdH9s29fyRW@mail.google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] trace-cmd: Open code execvp routine to avoid multiple execve syscalls | expand |
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);
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); >
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);
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(-)