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@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);
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
|