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

[Li Wang]

On Wed, Apr 19, 2023 at 5:59 PM Petr Vorel <pvorel@suse.cz> wrote:

> Hi Li,
>
> > On Wed, Apr 19, 2023 at 2:47 PM Li Wang <liwang@redhat.com> wrote:
>
> > > Hi Petr,
>
> > > On Thu, Apr 13, 2023 at 7:14 PM Petr Vorel <pvorel@suse.cz> wrote:
>
> > >> Similarly to previous commit, which normalized TMPDIR for shell API,
> > >> do the same for C API.
>
> > >> Unlike for shell API, here we modify $TMPDIR directly, because
> > >> tst_get_tmpdir_root() is used o more places.
>
> > >> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> > >> ---
> > >> Follow up of the same change in shell API:
> > >> https://lore.kernel.org/ltp/20230412073953.1983857-1-pvorel@suse.cz/
>
> > >> Kind regards,
> > >> Petr
>
> > >>  lib/tst_tmpdir.c | 16 ++++++++++++++--
> > >>  1 file changed, 14 insertions(+), 2 deletions(-)
>
> > >> diff --git a/lib/tst_tmpdir.c b/lib/tst_tmpdir.c
> > >> index b73b5c66f..8db5c47e8 100644
> > >> --- a/lib/tst_tmpdir.c
> > >> +++ 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.



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



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

-- 
Regards,
Li Wang