| Message ID | 20231128192435.36507-1-void@manifault.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | trace-cmd record: Reset PATH variable after strtok search | expand |
On Tue, 28 Nov 2023 13:24:35 -0600 David Vernet <void@manifault.com> wrote: > execute_program(), in the trace-cmd record subcommand, searches for a > command in PATH to create an absolute path to pass to execve. The > implementation uses strtok_r, which mutates the underlying string in > place by replacing ':' tokens with NULL bytes. This can and does cause > the PATH that's passed to execve to only contain the first entry to > PATH, which can cause issues such as the following: > > [root@maniforge linus]# trace-cmd record -e sched -v -e sched_stat_runtime make clean > /bin/sh: line 1: uname: command not found > /bin/sh: line 1: sed: command not found > /bin/sh: line 1: head: command not found > /bin/sh: line 1: grep: command not found > /bin/sh: line 1: mkdir: command not found > ... > /bin/sh: line 1: mkdir: command not found > /bin/sh: line 1: mkdir: command not found > Makefile:681: arch//Makefile: No such file or directory > make: *** No rule to make target 'arch//Makefile'. Stop. > > We should be resetting the PATH variable to the string stored in the > saveptr argument to strtok_r. Bah, I had this fixed locally, but never made pushed it up. > > Fixes: edf9424029cc ("trace-cmd: Open code execvp routine to avoid multiple execve syscalls") > Signed-off-by: David Vernet <void@manifault.com> > --- > tracecmd/trace-record.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c > index bced80406816..63af11ecaa80 100644 > --- a/tracecmd/trace-record.c > +++ b/tracecmd/trace-record.c > @@ -1708,6 +1708,14 @@ static void execute_program(int argc, char **argv) > break; > > } > + > + /* > + * reset PATH to saveptr, as strtok_r overwrites the string > + * returned by getenv() which backs the PATH environment > + * variable. > + */ > + if (setenv("PATH", saveptr, 1)) > + die("Failed to reset PATH to %s (%s)", saveptr, strerror(errno)); > } else { > strncpy(buf, argv[0], sizeof(buf)); > } The fix I had was this: diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c index bced8040..1a461631 100644 --- a/tracecmd/trace-record.c +++ b/tracecmd/trace-record.c @@ -1698,6 +1698,9 @@ static void execute_program(int argc, char **argv) if (!path) die("can't search for '%s' if $PATH is NULL", argv[0]); + /* Do not modify the actual environment variable */ + path = strdup(path); + for (entry = strtok_r(path, ":", &saveptr); entry; entry = strtok_r(NULL, ":", &saveptr)) { @@ -1712,6 +1715,7 @@ static void execute_program(int argc, char **argv) strncpy(buf, argv[0], sizeof(buf)); } + free(path); tracecmd_enable_tracing(); if (execve(buf, argv, environ)) { fprintf(stderr, "\n********************\n"); Does that work for you? Although, I still need to test the result of strdup(). -- Steve
On Tue, Nov 28, 2023 at 02:30:44PM -0500, Steven Rostedt wrote: > On Tue, 28 Nov 2023 13:24:35 -0600 > David Vernet <void@manifault.com> wrote: > > > execute_program(), in the trace-cmd record subcommand, searches for a > > command in PATH to create an absolute path to pass to execve. The > > implementation uses strtok_r, which mutates the underlying string in > > place by replacing ':' tokens with NULL bytes. This can and does cause > > the PATH that's passed to execve to only contain the first entry to > > PATH, which can cause issues such as the following: > > > > [root@maniforge linus]# trace-cmd record -e sched -v -e sched_stat_runtime make clean > > /bin/sh: line 1: uname: command not found > > /bin/sh: line 1: sed: command not found > > /bin/sh: line 1: head: command not found > > /bin/sh: line 1: grep: command not found > > /bin/sh: line 1: mkdir: command not found > > ... > > /bin/sh: line 1: mkdir: command not found > > /bin/sh: line 1: mkdir: command not found > > Makefile:681: arch//Makefile: No such file or directory > > make: *** No rule to make target 'arch//Makefile'. Stop. > > > > We should be resetting the PATH variable to the string stored in the > > saveptr argument to strtok_r. > > Bah, I had this fixed locally, but never made pushed it up. > > > > > Fixes: edf9424029cc ("trace-cmd: Open code execvp routine to avoid multiple execve syscalls") > > Signed-off-by: David Vernet <void@manifault.com> > > --- > > tracecmd/trace-record.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c > > index bced80406816..63af11ecaa80 100644 > > --- a/tracecmd/trace-record.c > > +++ b/tracecmd/trace-record.c > > @@ -1708,6 +1708,14 @@ static void execute_program(int argc, char **argv) > > break; > > > > } > > + > > + /* > > + * reset PATH to saveptr, as strtok_r overwrites the string > > + * returned by getenv() which backs the PATH environment > > + * variable. > > + */ > > + if (setenv("PATH", saveptr, 1)) > > + die("Failed to reset PATH to %s (%s)", saveptr, strerror(errno)); > > } else { > > strncpy(buf, argv[0], sizeof(buf)); > > } > > The fix I had was this: > > > diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c > index bced8040..1a461631 100644 > --- a/tracecmd/trace-record.c > +++ b/tracecmd/trace-record.c > @@ -1698,6 +1698,9 @@ static void execute_program(int argc, char **argv) > if (!path) > die("can't search for '%s' if $PATH is NULL", argv[0]); > > + /* Do not modify the actual environment variable */ > + path = strdup(path); > + > for (entry = strtok_r(path, ":", &saveptr); > entry; entry = strtok_r(NULL, ":", &saveptr)) { > > @@ -1712,6 +1715,7 @@ static void execute_program(int argc, char **argv) > strncpy(buf, argv[0], sizeof(buf)); > } > > + free(path); > tracecmd_enable_tracing(); > if (execve(buf, argv, environ)) { > fprintf(stderr, "\n********************\n"); > > Does that work for you? That would work too, though I don't think strtok_r() is doing anything useful at that point. IMO it's better to either do the setenv() with saveptr, or change that strtok_r() to a regular strtok(). > > Although, I still need to test the result of strdup(). > > -- Steve
On Tue, Nov 28, 2023 at 02:08:56PM -0600, David Vernet wrote: > On Tue, Nov 28, 2023 at 02:30:44PM -0500, Steven Rostedt wrote: > > On Tue, 28 Nov 2023 13:24:35 -0600 > > David Vernet <void@manifault.com> wrote: > > > > > execute_program(), in the trace-cmd record subcommand, searches for a > > > command in PATH to create an absolute path to pass to execve. The > > > implementation uses strtok_r, which mutates the underlying string in > > > place by replacing ':' tokens with NULL bytes. This can and does cause > > > the PATH that's passed to execve to only contain the first entry to > > > PATH, which can cause issues such as the following: > > > > > > [root@maniforge linus]# trace-cmd record -e sched -v -e sched_stat_runtime make clean > > > /bin/sh: line 1: uname: command not found > > > /bin/sh: line 1: sed: command not found > > > /bin/sh: line 1: head: command not found > > > /bin/sh: line 1: grep: command not found > > > /bin/sh: line 1: mkdir: command not found > > > ... > > > /bin/sh: line 1: mkdir: command not found > > > /bin/sh: line 1: mkdir: command not found > > > Makefile:681: arch//Makefile: No such file or directory > > > make: *** No rule to make target 'arch//Makefile'. Stop. > > > > > > We should be resetting the PATH variable to the string stored in the > > > saveptr argument to strtok_r. > > > > Bah, I had this fixed locally, but never made pushed it up. > > > > > > > > Fixes: edf9424029cc ("trace-cmd: Open code execvp routine to avoid multiple execve syscalls") > > > Signed-off-by: David Vernet <void@manifault.com> > > > --- > > > tracecmd/trace-record.c | 8 ++++++++ > > > 1 file changed, 8 insertions(+) > > > > > > diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c > > > index bced80406816..63af11ecaa80 100644 > > > --- a/tracecmd/trace-record.c > > > +++ b/tracecmd/trace-record.c > > > @@ -1708,6 +1708,14 @@ static void execute_program(int argc, char **argv) > > > break; > > > > > > } > > > + > > > + /* > > > + * reset PATH to saveptr, as strtok_r overwrites the string > > > + * returned by getenv() which backs the PATH environment > > > + * variable. > > > + */ > > > + if (setenv("PATH", saveptr, 1)) > > > + die("Failed to reset PATH to %s (%s)", saveptr, strerror(errno)); > > > } else { > > > strncpy(buf, argv[0], sizeof(buf)); > > > } > > > > The fix I had was this: > > > > > > diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c > > index bced8040..1a461631 100644 > > --- a/tracecmd/trace-record.c > > +++ b/tracecmd/trace-record.c > > @@ -1698,6 +1698,9 @@ static void execute_program(int argc, char **argv) > > if (!path) > > die("can't search for '%s' if $PATH is NULL", argv[0]); > > > > + /* Do not modify the actual environment variable */ > > + path = strdup(path); > > + > > for (entry = strtok_r(path, ":", &saveptr); > > entry; entry = strtok_r(NULL, ":", &saveptr)) { > > > > @@ -1712,6 +1715,7 @@ static void execute_program(int argc, char **argv) > > strncpy(buf, argv[0], sizeof(buf)); > > } > > > > + free(path); Also, this should either be brought into the !strchr() branch, or path should be initialized to NULL. > > tracecmd_enable_tracing(); > > if (execve(buf, argv, environ)) { > > fprintf(stderr, "\n********************\n"); > > > > Does that work for you? > > That would work too, though I don't think strtok_r() is doing anything > useful at that point. IMO it's better to either do the setenv() with > saveptr, or change that strtok_r() to a regular strtok(). > > > > > Although, I still need to test the result of strdup(). > > > > -- Steve
On Tue, 28 Nov 2023 14:13:02 -0600 David Vernet <void@manifault.com> wrote: > > > > > > > > Fixes: edf9424029cc ("trace-cmd: Open code execvp routine to avoid multiple execve syscalls") > > > > Signed-off-by: David Vernet <void@manifault.com> > > > > --- > > > > tracecmd/trace-record.c | 8 ++++++++ > > > > 1 file changed, 8 insertions(+) > > > > > > > > diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c > > > > index bced80406816..63af11ecaa80 100644 > > > > --- a/tracecmd/trace-record.c > > > > +++ b/tracecmd/trace-record.c > > > > @@ -1708,6 +1708,14 @@ static void execute_program(int argc, char **argv) > > > > break; > > > > > > > > } > > > > + > > > > + /* > > > > + * reset PATH to saveptr, as strtok_r overwrites the string > > > > + * returned by getenv() which backs the PATH environment > > > > + * variable. > > > > + */ > > > > + if (setenv("PATH", saveptr, 1)) > > > > + die("Failed to reset PATH to %s (%s)", saveptr, strerror(errno)); > > > > } else { > > > > strncpy(buf, argv[0], sizeof(buf)); > > > > } > > > > > > The fix I had was this: > > > > > > > > > diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c > > > index bced8040..1a461631 100644 > > > --- a/tracecmd/trace-record.c > > > +++ b/tracecmd/trace-record.c > > > @@ -1698,6 +1698,9 @@ static void execute_program(int argc, char **argv) > > > if (!path) > > > die("can't search for '%s' if $PATH is NULL", argv[0]); > > > > > > + /* Do not modify the actual environment variable */ > > > + path = strdup(path); > > > + > > > for (entry = strtok_r(path, ":", &saveptr); > > > entry; entry = strtok_r(NULL, ":", &saveptr)) { > > > > > > @@ -1712,6 +1715,7 @@ static void execute_program(int argc, char **argv) > > > strncpy(buf, argv[0], sizeof(buf)); > > > } > > > > > > + free(path); > > Also, this should either be brought into the !strchr() branch, or path > should be initialized to NULL. yeah, that's probably why I haven't actually committed it yet (it's still just a "diff" in my tree ;-) > > > > tracecmd_enable_tracing(); > > > if (execve(buf, argv, environ)) { > > > fprintf(stderr, "\n********************\n"); > > > > > > Does that work for you? > > > > That would work too, though I don't think strtok_r() is doing anything > > useful at that point. IMO it's better to either do the setenv() with > > saveptr, or change that strtok_r() to a regular strtok(). I always use strtok_r() over strtok() just because it's "safer"! I know it's not necessary, but the number of times I had to switch it to make the code thread safe, I just decided to always use it. Just my personal preference. -- Steve > > > > > > > > Although, I still need to test the result of strdup(). > > > > > > -- Steve
On 2023-11-28 15:18, Steven Rostedt wrote: > On Tue, 28 Nov 2023 14:13:02 -0600 > David Vernet <void@manifault.com> wrote: [...] >>> That would work too, though I don't think strtok_r() is doing anything >>> useful at that point. IMO it's better to either do the setenv() with >>> saveptr, or change that strtok_r() to a regular strtok(). > > I always use strtok_r() over strtok() just because it's "safer"! > > I know it's not necessary, but the number of times I had to switch it to > make the code thread safe, I just decided to always use it. Just my personal > preference. And if you want to make your code thread-safe, you should favor working on a strdup() copy rather than modifying the argv or env content. Also, modifying global state prevents code from being eventually re-used in libraries. Thanks, Mathieu > > -- Steve > > >>> >>>> >>>> Although, I still need to test the result of strdup(). >>>> >>>> -- Steve > >
On Tue, 28 Nov 2023 15:22:19 -0500 Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote: > On 2023-11-28 15:18, Steven Rostedt wrote: > > On Tue, 28 Nov 2023 14:13:02 -0600 > > David Vernet <void@manifault.com> wrote: > > [...] > > >>> That would work too, though I don't think strtok_r() is doing anything > >>> useful at that point. IMO it's better to either do the setenv() with > >>> saveptr, or change that strtok_r() to a regular strtok(). > > > > I always use strtok_r() over strtok() just because it's "safer"! > > > > I know it's not necessary, but the number of times I had to switch it to > > make the code thread safe, I just decided to always use it. Just my personal > > preference. > > And if you want to make your code thread-safe, you should favor working > on a strdup() copy rather than modifying the argv or env content. Yes, the fix was to use strdup(). It wasn't even a thread issue, nor a library issue, as the code in question is part of the trace-cmd executable and not the libraries. The problem was that it modified the environment variable and then reused that same environment variable in the exec() operation :-p > > Also, modifying global state prevents code from being eventually re-used > in libraries. That's why I now always use strtok_r() by default. It is thread (and library) safe. I avoid using strtok() even when it's perfectly fine to do so. -- Steve
diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c index bced80406816..63af11ecaa80 100644 --- a/tracecmd/trace-record.c +++ b/tracecmd/trace-record.c @@ -1708,6 +1708,14 @@ static void execute_program(int argc, char **argv) break; } + + /* + * reset PATH to saveptr, as strtok_r overwrites the string + * returned by getenv() which backs the PATH environment + * variable. + */ + if (setenv("PATH", saveptr, 1)) + die("Failed to reset PATH to %s (%s)", saveptr, strerror(errno)); } else { strncpy(buf, argv[0], sizeof(buf)); }
execute_program(), in the trace-cmd record subcommand, searches for a command in PATH to create an absolute path to pass to execve. The implementation uses strtok_r, which mutates the underlying string in place by replacing ':' tokens with NULL bytes. This can and does cause the PATH that's passed to execve to only contain the first entry to PATH, which can cause issues such as the following: [root@maniforge linus]# trace-cmd record -e sched -v -e sched_stat_runtime make clean /bin/sh: line 1: uname: command not found /bin/sh: line 1: sed: command not found /bin/sh: line 1: head: command not found /bin/sh: line 1: grep: command not found /bin/sh: line 1: mkdir: command not found ... /bin/sh: line 1: mkdir: command not found /bin/sh: line 1: mkdir: command not found Makefile:681: arch//Makefile: No such file or directory make: *** No rule to make target 'arch//Makefile'. Stop. We should be resetting the PATH variable to the string stored in the saveptr argument to strtok_r. Fixes: edf9424029cc ("trace-cmd: Open code execvp routine to avoid multiple execve syscalls") Signed-off-by: David Vernet <void@manifault.com> --- tracecmd/trace-record.c | 8 ++++++++ 1 file changed, 8 insertions(+)