Re: [PATCH] trace-cmd record: Reset PATH variable after strtok search

[David Vernet <void@manifault.com>]

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