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

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

[David Vernet]

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(+)

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));
 	}
-- 
2.42.1

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

[Steven Rostedt]

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

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

[David Vernet]

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

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

[David Vernet]

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

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

[Steven Rostedt]

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  

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

[Mathieu Desnoyers]

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

-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com

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

[Steven Rostedt]

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