On Tue, 28 Nov 2023 14:13:02 -0600
David Vernet <void@xxxxxxxxxxxxx> wrote:
> > > >
> > > > 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.
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
|