tests: dis/allow '.' in PATH?

tests: dis/allow '.' in PATH?

[Bernhard Voelker]

GNU findutils got a bug report for a failing test in the testsuite [1].
It turned out that the calling environment had the current directory '.'
in PATH.  This triggered a warning in `find -execdir ...` and therefore
made some tests fail.

[1] https://lists.gnu.org/r/bug-findutils/2021-11/msg00004.html

Of course, the findutils tests could (and probably will) remove '.' from PATH,
but I wonder if this should or should not be done for other packages via a change
in 'tests/init.sh' as well?
If a certain test needs '.' in the PATH, it will have to add it anyway.

Any comments?

Have a nice day,
Berny

Re: tests: dis/allow '.' in PATH?

[Paul Eggert]

On 11/23/21 14:19, Bernhard Voelker wrote:
> Of course, the findutils tests could (and probably will) remove '.' from PATH,
> but I wonder if this should or should not be done for other packages via a change
> in 'tests/init.sh' as well?

Sounds good to me.

It might also help to sanitize other sensitive environment variables, 
such as HOME, SHELL, ENV.

Re: tests: dis/allow '.' in PATH?

[Kamil Dudka]

On Tuesday, November 23, 2021 11:19:22 PM CET Bernhard Voelker wrote:
> GNU findutils got a bug report for a failing test in the testsuite [1].
> It turned out that the calling environment had the current directory '.'
> in PATH.  This triggered a warning in `find -execdir ...` and therefore
> made some tests fail.
> 
> [1] https://lists.gnu.org/r/bug-findutils/2021-11/msg00004.html
> 
> Of course, the findutils tests could (and probably will) remove '.' from
> PATH, but I wonder if this should or should not be done for other packages
> via a change in 'tests/init.sh' as well?
> If a certain test needs '.' in the PATH, it will have to add it anyway.
> 
> Any comments?

Note that having an "empty item" in PATH will have the same effect as
having '.' in PATH, according to POSIX [1]:

    A zero-length prefix is a legacy feature that indicates the current
    working directory.  It appears as two adjacent <colon> characters
    ( "::" ), as an initial <colon> preceding the rest of the list, or
    as a trailing <colon> following the rest of the list.

Kamil

[1] https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap08.html#tag_08_03

Re: tests: dis/allow '.' in PATH?

[Bernhard Voelker]

[-- Attachment #1: Type: text/plain, Size: 290 bytes --]

On 11/24/21 08:24, Kamil Dudka wrote:
> Note that having an "empty item" in PATH will have the same effect as
> having '.' in PATH, according to POSIX [1]:

Sure, the attached is an approach with shell tools, i.e., avoiding maybe
non-portable shell extensions.
WDYT?

Have a nice day,
Berny

[-- Attachment #2: 0001-test-framework-sh-remove-.-and-empty-entries-from-PA.patch --]
[-- Type: text/x-patch, Size: 2344 bytes --]

From a864d77c8b723fb3aede775c545320cfcedcc8bf Mon Sep 17 00:00:00 2001
From: Bernhard Voelker <mail@bernhard-voelker.de>
Date: Wed, 24 Nov 2021 23:59:00 +0100
Subject: [PATCH] test-framework-sh: remove '.' and empty entries from PATH

Running tests with '.' in the PATH may yield unspecified results,
and is deemed unsafe per se.  This includes empty entries as well
which are treated like a '.' entry as per POSIX.

* tests/init.sh (setup_): Add snippet to remove '.' and empty entries
from the PATH environment variable.
---
 ChangeLog     |  9 +++++++++
 tests/init.sh | 18 ++++++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/ChangeLog b/ChangeLog
index 3e752b238..f858fee07 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,12 @@
+2021-11-24  Bernhard Voelker  <mail@bernhard-voelker.de>
+
+	test-framework-sh: remove '.' and empty entries from PATH
+	Running tests with '.' in the PATH may yield unspecified results,
+	and is deemed unsafe per se.  This includes empty entries as well
+	which are treated like a '.' entry as per POSIX.
+	* tests/init.sh (setup_): Add snippet to remove '.' and empty entries
+	from the PATH environment variable.
+
 2021-11-24  Paul Eggert  <eggert@cs.ucla.edu>
 
 	regex: merge from glibc
diff --git a/tests/init.sh b/tests/init.sh
index 9ef834888..1f4d29e8b 100644
--- a/tests/init.sh
+++ b/tests/init.sh
@@ -426,6 +426,24 @@ setup_ ()
   for sig_ in 1 2 3 13 15; do
     eval "trap 'Exit $(expr $sig_ + 128)' $sig_"
   done
+
+  # Remove '.' from PATH, and also avoid "empty entries" which are treated as
+  # '.' as per POSIX, see PATH in:
+  # https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap08.html
+  # Strategy:
+  # printf: wrap PATH with additional separator characters.
+  # tr: change all separators to newline.
+  # sed: remove dot entries; remove empty entries (note: trailing newline).
+  # tr: change newlines back to separators
+  # head: remove last trailing separator.
+  path_=`printf '%s' "${PATH_SEPARATOR}${PATH}${PATH_SEPARATOR}" \
+           | tr "${PATH_SEPARATOR}" "$gl_init_sh_nl_" \
+           | sed -e '/^\.$/d' -e '/^$/d'\
+           | tr "$gl_init_sh_nl_" "${PATH_SEPARATOR}" \
+           | head -c -1
+           `
+  PATH="$path_"
+  export PATH
 }
 
 # This is a stub function that is run upon trap (upon regular exit and
-- 
2.33.1

Re: tests: dis/allow '.' in PATH?

[Paul Eggert]

On 11/24/21 15:03, Bernhard Voelker wrote:
> Sure, the attached is an approach with shell tools

Can't this be done without using any subsidiary commands?

Something like the following untested code. This removes all relative 
names from PATH, not just '.'.

saved_IFS=$IFS
IFS=:
new_PATH=
for dir in $PATH; do
   case $dir in
     /*) new_PATH=$new_PATH${new_PATH:-:}$dir;;
   esac
done
IFS=$saved_IFS
PATH=$new_PATH

Re: tests: dis/allow '.' in PATH?

[Bernhard Voelker]

[-- Attachment #1: Type: text/plain, Size: 555 bytes --]

On 11/25/21 00:54, Paul Eggert wrote:
> On 11/24/21 15:03, Bernhard Voelker wrote:
> Something like the following untested code. This removes all relative 
> names from PATH, not just '.'.

Good idea.  Looking at some code from coreutils, I also suggest to
test if the entries exist.

> saved_IFS=$IFS
> IFS=:
> new_PATH=
> for dir in $PATH; do
>    case $dir in
>      /*) new_PATH=$new_PATH${new_PATH:-:}$dir;;
_______________________________________^^
This operator doesn't do what we need here.

PFA the revised patch.

Thanks & have a nice day,
Berny

[-- Attachment #2: 0001-test-framework-sh-remove-unsafe-entries-from-PATH.patch --]
[-- Type: text/x-patch, Size: 1971 bytes --]

From d50912b6c60732476bb2955d947bacb73aaa2d59 Mon Sep 17 00:00:00 2001
From: Bernhard Voelker <mail@bernhard-voelker.de>
Date: Wed, 24 Nov 2021 23:59:00 +0100
Subject: [PATCH] test-framework-sh: remove unsafe entries from PATH

Running tests with '.' in the PATH may yield unspecified results,
and is deemed unsafe per se.  This includes empty entries as well
which are treated like a '.' entry as per POSIX.

* tests/init.sh (setup_): Add snippet to remove relative and non-
accessible entries from the PATH environment variable.
---
 ChangeLog     |  9 +++++++++
 tests/init.sh | 17 +++++++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/ChangeLog b/ChangeLog
index 3e752b238..efbe6c888 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,12 @@
+2021-11-25  Bernhard Voelker  <mail@bernhard-voelker.de>
+
+	test-framework-sh: remove unsafe entries from PATH
+	Running tests with '.' in the PATH may yield unspecified results,
+	and is deemed unsafe per se.  This includes empty entries as well
+	which are treated like a '.' entry as per POSIX.
+	* tests/init.sh (setup_): Add snippet to remove relative and non-
+	accessible entries from the PATH environment variable.
+
 2021-11-24  Paul Eggert  <eggert@cs.ucla.edu>
 
 	regex: merge from glibc
diff --git a/tests/init.sh b/tests/init.sh
index 9ef834888..a975592ff 100644
--- a/tests/init.sh
+++ b/tests/init.sh
@@ -426,6 +426,23 @@ setup_ ()
   for sig_ in 1 2 3 13 15; do
     eval "trap 'Exit $(expr $sig_ + 128)' $sig_"
   done
+
+  # Remove relative and non-accessible directories from PATH, including '.'
+  # and Zero-length entries.
+  saved_IFS="$IFS"
+  IFS=:
+  new_PATH=
+  sep_=
+  for dir in $PATH; do
+    case "$dir" in
+      /*) test -d "$dir/." || continue
+          new_PATH="${new_PATH}${sep_}${dir}"
+          sep_=':';;
+    esac
+  done
+  IFS="$saved_IFS"
+  PATH="$new_PATH"
+  export PATH
 }
 
 # This is a stub function that is run upon trap (upon regular exit and
-- 
2.34.0