Re: [LTP] [PATCH 1/1] tst_tmpdir: Remove possible double/trailing slashes from TMPDIR

[Petr Vorel <pvorel@suse.cz>]

Hi Li,
...
> > > >> +++ b/lib/tst_tmpdir.c
> > > >> @@ -124,16 +124,28 @@ char *tst_get_tmpdir(void)

> > > >>  const char *tst_get_tmpdir_root(void)
> > > >>  {
> > > >> -       const char *env_tmpdir = getenv("TMPDIR");
> > > >> +       char *env_tmpdir = getenv("TMPDIR");


> > > > It seems that modifying the environment variables is generally
> > > > not a good practice.

> > > > The getenv() function returns a pointer to the value of an
> > > > environment variable, which is stored in the memory managed
> > > > by the system. Any attempt to modify this memory directly can
> > > > cause unexpected behavior or even crash the program.

> > > > Instead of modifying the return value of getenv(), it is recommended
> > > > to create a copy of the value and modify the copy instead.

> > Do you mean to use strdup()?


> Yeah, something like that, or we declare a buffer, and use strcpy()
> to copy the string pointed to by the return value of getenv() into the
> buffer that we can safely modify.

> I prefer it in this way.


Sure, I'll post new version with this. Until then I keep this patch open if
anybody wants to comment it.

> > Also man getenv(3) says:

> >        As typically implemented, getenv() returns a pointer to a string
> >        within the environment list.  The caller must take care not to
> >        modify this string, since that would change the environment of
> >        the process.

> > => I would not mind $TMPDIR got updated in the environment.

> > > Btw, the wise method is to use setenv() function to reset
> > > environment variables if really needed.

> > Well, I don't know any C test which needs it (only NFS tests which are
> > shell
> > tests). But I wanted to have the same behavior in both APIs.

> > > This is a different part of shell API I have to say.

> > Yes, the behavior is slightly different from shell API [1],
> > where it modifies $TST_TMPDIR (keep $TMPDIR untouched).


> > > > Or, the simplest way I guess is just TBROK and tell users why
> > > > this TMPDIR is unusable.

> > If you prefer it's better to TBROK on:
> > * double slashes
> > * trailing slash

> > I can do that. But at least on trailing slash looks to me quite strict.


> -1, trailing and double slash all accepted by shell in command line,
> maybe we shouldn't set a more strict policy than that.

Agree, I just didn't understand before your concern (you mostly objected the C
code, not the fact that the resulted path is modified).

Thanks for your reviewn!

Kind regards,
Petr




> > Whatever path we choose, I'd need also to update docs. BTW the need
> > to absolute path for TMPDIR is only in C - shell happily takes relative
> > path
> > and IMHO it's not documented.

> > Kind regards,
> > Petr

> > [1]
> > https://patchwork.ozlabs.org/project/ltp/patch/20230412073953.1983857-1-pvorel@suse.cz/

> > > >> -       if (!env_tmpdir)
> > > >> +       if (env_tmpdir) {
> > > >> +               /* remove duplicate slashes */
> > > >> +               for (char *p = env_tmpdir, *q = env_tmpdir; *q;) {
> > > >> +                       if (*++q != '/' || *p != '/')
> > > >> +                               *++p = *q;
> > > >> +               }
> > > >> +               /* Remove slash on the last place  */
> > > >> +               size_t last = strlen(env_tmpdir)-1;
> > > >> +               if (env_tmpdir[last] == '/')
> > > >> +                       env_tmpdir[last] = '\0';
> > > >> +       } else {
> > > >>                 env_tmpdir = TEMPDIR;
> > > >> +       }

> > > >>         if (env_tmpdir[0] != '/') {
> > > >>                 tst_brkm(TBROK, NULL, "You must specify an absolute "
> > > >>                                 "pathname for environment variable
> > > >> TMPDIR");
> > > >>                 return NULL;
> > > >>         }
> > > >> +
> > > >>         return env_tmpdir;
> > > >>  }

> > > >> --
> > > >> 2.40.0



> > > > --
> > > > Regards,
> > > > Li Wang

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp