| Message ID | 20230413111434.2103422-1-pvorel@suse.cz |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/1] tst_tmpdir: Remove possible double/trailing slashes from TMPDIR | expand |
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. Or, the simplest way I guess is just TBROK and tell users why this TMPDIR is unusable. > - 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 > >
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. > Btw, the wise method is to use setenv() function to reset environment variables if really needed. This is a different part of shell API I have to say. > Or, the simplest way I guess is just TBROK and tell users why > this TMPDIR is unusable. > > > >> - 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 >
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()? 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. 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
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 > >
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
On Thu, Apr 20, 2023 at 10:53 PM Petr Vorel <pvorel@suse.cz> wrote: > 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). > Yeah, I just doubted the incorrect way of doing that. (in C programming) Sorry for the unclear description, I'm always distressed by my English spelling level :-(.
> On Thu, Apr 20, 2023 at 10:53 PM Petr Vorel <pvorel@suse.cz> wrote: > > 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). > Yeah, I just doubted the incorrect way of doing that. > (in C programming) > Sorry for the unclear description, I'm always distressed by my English > spelling level :-(. Don't worry, most of us are non-native speakers, thus the problem is sometimes on the other side (me) :). Kind regards, Petr
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"); - 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; }
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(-)