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@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> > ---
> > 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
|