[LTP] [PATCH 0/4] Remove double or trailing slashes in TMPDIR in C API

[LTP] [PATCH 0/4] Remove double or trailing slashes in TMPDIR in C API

[Petr Vorel]

Hi,

the initial motivation to avoid problems when people set
TMPDIR=/var/tmp/ instead of TMPDIR=/var/tmp (which is needed for
runtest/fs or runtest/syscalls since [1]).

Kind regards,
Petr

[1] https://lore.kernel.org/ltp/20240201101603.GA78772@pevik/

Petr Vorel (4):
  lib/tst_tmpdir: Normalize user defined TMPDIR
  lib: Add test for tst_tmpdir
  tst_test.sh: Print warning if slashes in $TMPDIR
  lib: Improve doc related to $TMPDIR default value

 include/tst_defaults.h        |  5 ++--
 lib/newlib_tests/runtest.sh   |  2 +-
 lib/newlib_tests/tst_tmpdir.c | 48 +++++++++++++++++++++++++++++++++++
 lib/tst_tmpdir.c              | 20 ++++++++++++++-
 testcases/lib/tst_test.sh     | 17 ++++++++++---
 5 files changed, 84 insertions(+), 8 deletions(-)
 create mode 100644 lib/newlib_tests/tst_tmpdir.c

-- 
2.43.0


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

[LTP] [PATCH 1/4] lib/tst_tmpdir: Normalize user defined TMPDIR

[Petr Vorel]

Follow the changes to shell API 273c49793 ("tst_test.sh: Remove possible
double/trailing slashes from TMPDIR") and remove: 1) trailing slash
2) double slashes.

This is needed, because some tests compare file path of files which are
in TMPDIR with strcmp() or and extra slashes break it (e.g. chdir01A,
ioctl_loop0[12], mount0[67]).

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 lib/tst_tmpdir.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/lib/tst_tmpdir.c b/lib/tst_tmpdir.c
index b73b5c66f..bc9351251 100644
--- a/lib/tst_tmpdir.c
+++ b/lib/tst_tmpdir.c
@@ -124,7 +124,8 @@ char *tst_get_tmpdir(void)
 
 const char *tst_get_tmpdir_root(void)
 {
-	const char *env_tmpdir = getenv("TMPDIR");
+	char *p, *env_tmpdir = getenv("TMPDIR");
+	int fixed = 0;
 
 	if (!env_tmpdir)
 		env_tmpdir = TEMPDIR;
@@ -134,6 +135,23 @@ const char *tst_get_tmpdir_root(void)
 				"pathname for environment variable TMPDIR");
 		return NULL;
 	}
+
+	if (env_tmpdir[strlen(env_tmpdir)-1] == '/') {
+		env_tmpdir[strlen(env_tmpdir)-1] = '\0';
+		fixed = 1;
+	}
+
+	while ((p = strstr(env_tmpdir, "//")) != NULL) {
+		memmove(p, p + 1, strlen(env_tmpdir) - (p - env_tmpdir));
+		fixed = 1;
+	}
+
+	if (fixed) {
+		tst_resm(TINFO, "WARNING: Remove double or trailing slashes from TMPDIR,"
+			 " please fix your setup to: TMPDIR='%s'",
+			 env_tmpdir);
+	}
+
 	return env_tmpdir;
 }
 
-- 
2.43.0


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

Re: [LTP] [PATCH 1/4] lib/tst_tmpdir: Normalize user defined TMPDIR

[Petr Vorel]

Hi,

> Follow the changes to shell API 273c49793 ("tst_test.sh: Remove possible
> double/trailing slashes from TMPDIR") and remove: 1) trailing slash
> 2) double slashes.

> This is needed, because some tests compare file path of files which are
> in TMPDIR with strcmp() or and extra slashes break it (e.g. chdir01A,
> ioctl_loop0[12], mount0[67]).

> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
>  lib/tst_tmpdir.c | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)

> diff --git a/lib/tst_tmpdir.c b/lib/tst_tmpdir.c
> index b73b5c66f..bc9351251 100644
> --- a/lib/tst_tmpdir.c
> +++ b/lib/tst_tmpdir.c
> @@ -124,7 +124,8 @@ char *tst_get_tmpdir(void)

>  const char *tst_get_tmpdir_root(void)
>  {
> -	const char *env_tmpdir = getenv("TMPDIR");
> +	char *p, *env_tmpdir = getenv("TMPDIR");
> +	int fixed = 0;

>  	if (!env_tmpdir)
>  		env_tmpdir = TEMPDIR;
> @@ -134,6 +135,23 @@ const char *tst_get_tmpdir_root(void)
>  				"pathname for environment variable TMPDIR");
>  		return NULL;
>  	}
> +
> +	if (env_tmpdir[strlen(env_tmpdir)-1] == '/') {
> +		env_tmpdir[strlen(env_tmpdir)-1] = '\0';
> +		fixed = 1;
> +	}
Actually, this above supposed to be after removing double slash (in case people
are crazy to put /// or more in the end, see patch bellow.

I'm sorry for the noise.

Kind regards,
Petr

> +
> +	while ((p = strstr(env_tmpdir, "//")) != NULL) {
> +		memmove(p, p + 1, strlen(env_tmpdir) - (p - env_tmpdir));
> +		fixed = 1;
> +	}
> +
> +	if (fixed) {
> +		tst_resm(TINFO, "WARNING: Remove double or trailing slashes from TMPDIR,"
> +			 " please fix your setup to: TMPDIR='%s'",
> +			 env_tmpdir);
> +	}
> +
>  	return env_tmpdir;
>  }

diff --git lib/newlib_tests/tst_tmpdir.c lib/newlib_tests/tst_tmpdir.c
index 008542808..c34430fa6 100644
--- lib/newlib_tests/tst_tmpdir.c
+++ lib/newlib_tests/tst_tmpdir.c
@@ -18,7 +18,8 @@ static struct tcase {
 } tcases[] = {
 	{NULL, TEMPDIR, "default value"},
 	{"/tmp/", "/tmp", "removing trailing slash"},
-	{"//var///tmp", "/var/tmp", "removing duplicate slashes"},
+	{"/var//tmp", "/var/tmp", "removing duplicate slashes"},
+	{"//var///tmp///", "/var/tmp", "removing too many slashes"},
 };
 
 static void do_test(unsigned int nr)

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

Re: [LTP] [PATCH 1/4] lib/tst_tmpdir: Normalize user defined TMPDIR

[Cyril Hrubis]

Hi!
> +	if (env_tmpdir[strlen(env_tmpdir)-1] == '/') {
> +		env_tmpdir[strlen(env_tmpdir)-1] = '\0';
> +		fixed = 1;
> +	}
> +
> +	while ((p = strstr(env_tmpdir, "//")) != NULL) {
> +		memmove(p, p + 1, strlen(env_tmpdir) - (p - env_tmpdir));
> +		fixed = 1;
> +	}
> +
> +	if (fixed) {
> +		tst_resm(TINFO, "WARNING: Remove double or trailing slashes from TMPDIR,"
> +			 " please fix your setup to: TMPDIR='%s'",
> +			 env_tmpdir);
> +	}
> +

This whole thing can be just a single loop (beware untested):

	size_t s = 0, d = 0;
	char prev_c = 0;

	for (;;) {
		switch (env_tmpdir[s]) {
		case '/':
			if (prev_c != '/')
				d++;
			s++;
		break;
		case '\0':
			if (d && prev_c == '/')
				env_tmpdir[d-1] = '\0';
			break;
		break;
		default:
			env_tmpdir[d++] = env_tmpdir[s++];
		}
	}

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH 1/4] lib/tst_tmpdir: Normalize user defined TMPDIR

[Petr Vorel]

> Hi!
> > +	if (env_tmpdir[strlen(env_tmpdir)-1] == '/') {
> > +		env_tmpdir[strlen(env_tmpdir)-1] = '\0';
> > +		fixed = 1;
> > +	}
> > +
> > +	while ((p = strstr(env_tmpdir, "//")) != NULL) {
> > +		memmove(p, p + 1, strlen(env_tmpdir) - (p - env_tmpdir));
> > +		fixed = 1;
> > +	}
> > +
> > +	if (fixed) {
> > +		tst_resm(TINFO, "WARNING: Remove double or trailing slashes from TMPDIR,"
> > +			 " please fix your setup to: TMPDIR='%s'",
> > +			 env_tmpdir);
> > +	}
> > +

> This whole thing can be just a single loop (beware untested):

> 	size_t s = 0, d = 0;
> 	char prev_c = 0;

> 	for (;;) {
> 		switch (env_tmpdir[s]) {

NOTE you never set prev_c, it would have to be:

	if (s)
		prev_c = env_tmpdir[s-1];

> 		case '/':
> 			if (prev_c != '/')
> 				d++;
> 			s++;
> 		break;
> 		case '\0':
> 			if (d && prev_c == '/')
> 				env_tmpdir[d-1] = '\0';
> 			break;

And also you don't break for loop, because the break above is for case, goto or
return would have to be used).

> 		break;
> 		default:
> 			env_tmpdir[d++] = env_tmpdir[s++];
> 		}
> 	}

Also if TMPDIR is not set, env_tmpdir = TEMPDIR; will lead to segfault.
strdup() will need to be used:

	if (!env_tmpdir)
		env_tmpdir = strdup(TEMPDIR);

But even with these changes I was not able to find why / is eaten:


	char *env_tmpdir = getenv("TMPDIR");
	size_t s = 0, d = 0;
	char prev_c = 0;

	if (!env_tmpdir)
		env_tmpdir = strdup(TEMPDIR);

	for (;;) {
		if (s)
			prev_c = env_tmpdir[s-1];

		switch (env_tmpdir[s]) {
		case '/':
			if (prev_c != '/')
				d++;
			s++;
		break;
		case '\0':
			if (d && prev_c == '/')
				env_tmpdir[d-1] = '\0';
			goto end;
		break;
		default:
			env_tmpdir[d++] = env_tmpdir[s++];
		}
	}

end:
	return env_tmpdir;
}


So how about using for loop? It's not that compact, but maybe easily readable.

const char *tst_get_tmpdir_root(void)
{
	char *env_tmpdir = getenv("TMPDIR");
	char prev_c = 0;
	size_t k = 0;

	if (!env_tmpdir)
		env_tmpdir = strdup(TEMPDIR);

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

	for (int i = 0; env_tmpdir[i] != '\0'; i++) {
		if (i)
			prev_c = env_tmpdir[i-1];

        if (env_tmpdir[i] != '/' || prev_c != '/')
            env_tmpdir[k++] = env_tmpdir[i];
	}

	env_tmpdir[k] = '\0';

	if (env_tmpdir[k-1] == '/')
		env_tmpdir[k-1] = '\0';

	return env_tmpdir;
}

Kind regards,
Petr

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

Re: [LTP] [PATCH 1/4] lib/tst_tmpdir: Normalize user defined TMPDIR

[Cyril Hrubis]

Hi!
> +	if (fixed) {
> +		tst_resm(TINFO, "WARNING: Remove double or trailing slashes from TMPDIR,"
> +			 " please fix your setup to: TMPDIR='%s'",
> +			 env_tmpdir);
> +	}

Also why do we bother with a warning when we fixed the path. I would
either fix it and go on or fail the test if it's detected.

-- 
Cyril Hrubis
chrubis@suse.cz

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

[LTP] [PATCH 2/4] lib: Add test for tst_tmpdir

[Petr Vorel]

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 lib/newlib_tests/runtest.sh   |  2 +-
 lib/newlib_tests/tst_tmpdir.c | 48 +++++++++++++++++++++++++++++++++++
 2 files changed, 49 insertions(+), 1 deletion(-)
 create mode 100644 lib/newlib_tests/tst_tmpdir.c

diff --git a/lib/newlib_tests/runtest.sh b/lib/newlib_tests/runtest.sh
index be707fe63..6c69e448d 100755
--- a/lib/newlib_tests/runtest.sh
+++ b/lib/newlib_tests/runtest.sh
@@ -5,7 +5,7 @@ LTP_C_API_TESTS="${LTP_C_API_TESTS:-test05 test07 test09 test15 test_runtime01
 tst_needs_cmds01 tst_needs_cmds02 tst_needs_cmds03 tst_needs_cmds06
 tst_needs_cmds07 tst_bool_expr test_exec test_timer tst_res_hexd tst_strstatus
 tst_fuzzy_sync03 test_zero_hugepage.sh test_kconfig.sh
-test_children_cleanup.sh}"
+test_children_cleanup.sh tst_tmpdir}"
 
 LTP_SHELL_API_TESTS="${LTP_SHELL_API_TESTS:-shell/tst_check_driver.sh
 shell/tst_check_kconfig0[1-5].sh shell/tst_errexit.sh shell/net/*.sh}"
diff --git a/lib/newlib_tests/tst_tmpdir.c b/lib/newlib_tests/tst_tmpdir.c
new file mode 100644
index 000000000..008542808
--- /dev/null
+++ b/lib/newlib_tests/tst_tmpdir.c
@@ -0,0 +1,48 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2024 Petr Vorel <pvorel@suse.cz>
+ */
+
+/*
+ * Tests TMPDIR variable cleanup.
+ */
+
+#include <stdlib.h>
+#include "tst_defaults.h"
+#include "tst_test.h"
+
+static struct tcase {
+	const char *orig;
+	const char *fixed;
+	const char *desc;
+} tcases[] = {
+	{NULL, TEMPDIR, "default value"},
+	{"/tmp/", "/tmp", "removing trailing slash"},
+	{"//var///tmp", "/var/tmp", "removing duplicate slashes"},
+};
+
+static void do_test(unsigned int nr)
+{
+	struct tcase *tc = &tcases[nr];
+	const char *env_tmpdir;
+
+	tst_res(TINFO, "Testing TMPDIR='%s' (%s)", tc->orig, tc->desc);
+
+	if (tc->orig)
+		SAFE_SETENV("TMPDIR", tc->orig, 1);
+
+	env_tmpdir = tst_get_tmpdir_root();
+
+	if (!env_tmpdir)
+		tst_brk(TBROK, "Failed to get TMPDIR");
+
+	if (!strcmp(tc->fixed, env_tmpdir))
+		tst_res(TPASS, "TMPDIR '%s' is '%s'", env_tmpdir, tc->fixed);
+	else
+		tst_res(TFAIL, "TMPDIR '%s' should be '%s'", env_tmpdir, tc->fixed);
+}
+
+static struct tst_test test = {
+	.test = do_test,
+	.tcnt = ARRAY_SIZE(tcases),
+};
-- 
2.43.0


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

Re: [LTP] [PATCH 2/4] lib: Add test for tst_tmpdir

[Petr Vorel]

And more diff here.

Kind regards,
Petr

diff --git lib/newlib_tests/tst_tmpdir.c lib/newlib_tests/tst_tmpdir.c
index 008542808..c34430fa6 100644
--- lib/newlib_tests/tst_tmpdir.c
+++ lib/newlib_tests/tst_tmpdir.c
@@ -18,7 +18,8 @@ static struct tcase {
 } tcases[] = {
 	{NULL, TEMPDIR, "default value"},
 	{"/tmp/", "/tmp", "removing trailing slash"},
-	{"//var///tmp", "/var/tmp", "removing duplicate slashes"},
+	{"/var//tmp", "/var/tmp", "removing duplicate slashes"},
+	{"//var///tmp///", "/var/tmp", "removing too many slashes"},
 };
 
 static void do_test(unsigned int nr)

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

[LTP] [PATCH 3/4] tst_test.sh: Print warning if slashes in $TMPDIR

[Petr Vorel]

Further improve changes in 273c49793 to print warning, because C API
started to print warning as well. Previous requirement to not actually
modify $TMPDIR (just to cleanup $TST_TMPDIR) required to rewrite the
code).

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 testcases/lib/tst_test.sh | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh
index 0d2fccb95..20bf21660 100644
--- a/testcases/lib/tst_test.sh
+++ b/testcases/lib/tst_test.sh
@@ -666,6 +666,7 @@ tst_run()
 	local _tst_data
 	local _tst_max
 	local _tst_name
+	local _tst_tmpdir
 	local _tst_pattern='[='\''"} \t\/:`$\;|].*'
 	local ret
 
@@ -734,12 +735,18 @@ tst_run()
 
 	if [ "$TST_NEEDS_TMPDIR" = 1 ]; then
 		if [ -z "$TMPDIR" ]; then
-			export TMPDIR="/tmp"
+			_tst_tmpdir="/tmp"
+			export TMPDIR="$_tst_tmpdir"
+		else
+			# remove possible double slashes or trailing slash from TMPDIR
+			_tst_tmpdir=$(echo "$TMPDIR" | sed 's~/\+~/~g; s~/$~~')
+			if [ "$_tst_tmpdir" != "$TMPDIR" ]; then
+				tst_res TINFO "WARNING: Remove double or trailing slashes from TMPDIR," \
+					"please fix your setup to: TMPDIR='$_tst_tmpdir'"
+			fi
 		fi
 
-		TST_TMPDIR=$(mktemp -d "$TMPDIR/LTP_$TST_ID.XXXXXXXXXX")
-		# remove possible trailing slash or double slashes from TMPDIR
-		TST_TMPDIR=$(echo "$TST_TMPDIR" | sed 's~/\+~/~g')
+		TST_TMPDIR=$(mktemp -d "$_tst_tmpdir/LTP_$TST_ID.XXXXXXXXXX")
 
 		chmod 777 "$TST_TMPDIR"
 
-- 
2.43.0


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

[LTP] [PATCH 4/4] lib: Improve doc related to $TMPDIR default value

[Petr Vorel]

Link C and shell API related parts to $TMPDIR.

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 include/tst_defaults.h    | 5 +++--
 testcases/lib/tst_test.sh | 2 ++
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/include/tst_defaults.h b/include/tst_defaults.h
index 083427b7e..e3eb6f587 100644
--- a/include/tst_defaults.h
+++ b/include/tst_defaults.h
@@ -7,9 +7,10 @@
 #define TST_DEFAULTS_H_
 
 /*
- * This is the default temporary directory used by tst_tmpdir().
+ * This is the default temporary directory used by tst_tmpdir(),
+ * used when TMPDIR env variable is not set.
  *
- * This is used when TMPDIR env variable is not set.
+ * If changed, update also shell API in tst_test.sh.
  */
 #define TEMPDIR	"/tmp"
 
diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh
index 20bf21660..760a49061 100644
--- a/testcases/lib/tst_test.sh
+++ b/testcases/lib/tst_test.sh
@@ -735,6 +735,8 @@ tst_run()
 
 	if [ "$TST_NEEDS_TMPDIR" = 1 ]; then
 		if [ -z "$TMPDIR" ]; then
+			# default value if TMPDIR not set.
+			# If changed, update also C API in tst_defaults.h.
 			_tst_tmpdir="/tmp"
 			export TMPDIR="$_tst_tmpdir"
 		else
-- 
2.43.0


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