Merge pull request #5138 from grondo/path-insert

mergify[bot] · web-flow · commit 6f41e0b50470 · 2023-05-03T18:35:08.000Z
flux(1): avoid prepending to PATH when unnecessary
diff --git a/src/cmd/flux.c b/src/cmd/flux.c
@@ -22,6 +22,7 @@
 #include <flux/optparse.h>
 #include <pwd.h>
 
+#include "ccan/str/str.h"
 #include "src/common/libutil/log.h"
 #include "src/common/libutil/xzmalloc.h"
 #include "src/common/libutil/environment.h"
@@ -256,6 +257,41 @@ bool flux_is_installed (void)
     return (rc == 0);
 }
 
+void ensure_self_first_in_path (struct environment *e, const char *selfdir)
+{
+    const char *entry = NULL;
+    char realdir[PATH_MAX+1];
+    char path[PATH_MAX+1];
+
+    while ((entry = environment_var_next (e, "PATH", entry))) {
+        /*  Attempt to canonicalize path, skipping eny elements that
+         *  can't be resolved.
+         */
+        if (!(realpath (entry, realdir)))
+            continue;
+        /*  If this path matches "selfdir", then the current flux
+         *  executable already appears first in path. Return.
+         */
+        if (streq (realdir, selfdir))
+            return;
+        /*  Otherwise, check for a flux in this path element, if it
+         *  is present and executable, then the current flux is not
+         *  first in path, insert selfdir before this element.
+         */
+        if (snprintf (path,
+                      sizeof (path),
+                      "%s/flux",
+                      realpath (entry, realdir)) >= sizeof (path)
+            || access (path, R_OK|X_OK) == 0) {
+            if (environment_insert (e, "PATH", (char *) entry, selfdir) < 0)
+                break;
+            return;
+        }
+    }
+    /* No flux(1) found in current PATH, we can insert selfdir at back */
+    environment_push_back (e, "PATH", selfdir);
+}
+
 /*
  * If flux command was run with relative or absolute path, then
  *  prepend the directory for the flux executable to PATH. This
@@ -269,11 +305,11 @@ void setup_path (struct environment *env, const char *argv0)
     assert (argv0);
 
     /*  If argv[0] was explicitly "flux" then assume PATH is already set */
-    if (strcmp (argv0, "flux") == 0)
+    if (streq (argv0, "flux"))
         return;
     if ((selfdir = executable_selfdir ())) {
         environment_from_env (env, "PATH", "/bin:/usr/bin", ':');
-        environment_push (env, "PATH", executable_selfdir ());
+        ensure_self_first_in_path (env, selfdir);
     }
     else
         log_msg_exit ("Unable to determine flux executable dir");
diff --git a/src/common/libutil/Makefile.am b/src/common/libutil/Makefile.am
@@ -131,7 +131,8 @@ TESTS = test_sha1.t \
 	test_hola.t \
 	test_strstrip.t \
 	test_slice.t \
-	test_timestamp.t
+	test_timestamp.t \
+	test_environment.t
 
 test_ldadd = \
 	$(top_builddir)/src/common/libutil/libutil.la \
@@ -278,3 +279,7 @@ test_slice_t_LDADD = $(test_ldadd)
 test_timestamp_t_SOURCES = test/timestamp.c
 test_timestamp_t_CPPFLAGS = $(test_cppflags)
 test_timestamp_t_LDADD = $(test_ldadd)
+
+test_environment_t_SOURCES = test/environment.c
+test_environment_t_CPPFLAGS = $(test_cppflags)
+test_environment_t_LDADD = $(test_ldadd)
diff --git a/src/common/libutil/environment.c b/src/common/libutil/environment.c
@@ -162,7 +162,6 @@ static void environment_push_inner (struct environment *e,
             split_value = rev;
             split_value_len = rev_len;
         }
-
         entry = NULL;
         while((entry = argz_next (split_value, split_value_len, entry))) {
             char *found;
@@ -255,6 +254,38 @@ const char *environment_cursor (struct environment *e)
     return zhash_cursor (e->environment);
 }
 
+const char *environment_var_next (struct environment *e,
+                                  const char *key,
+                                  const char *entry)
+{
+    struct env_item *item = zhash_lookup (e->environment, key);
+    if (!item)
+        return NULL;
+    return argz_next (item->argz, item->argz_len, entry);
+}
+
+int environment_insert (struct environment *e,
+                        const char *key,
+                        char *before,
+                        const char *value)
+{
+    error_t err;
+    struct env_item *item = zhash_lookup (e->environment, key);
+    if (!item) {
+        errno = ENOENT;
+        return -1;
+    }
+    if ((err = argz_insert (&item->argz,
+                            &item->argz_len,
+                            before,
+                            value) != 0)) {
+        errno = err;
+        return -1;
+    }
+    return 0;
+}
+
+
 void environment_apply (struct environment *e)
 {
     const char *key;
diff --git a/src/common/libutil/environment.h b/src/common/libutil/environment.h
@@ -48,6 +48,19 @@ const char *environment_next (struct environment *e);
  */
 const char *environment_cursor (struct environment *e);
 
+/**
+ * @brief Iterate over a multi-element environment variable *_key
+ *
+ * @param e the environment to operate on
+ * @param key the environment variable to iterate over
+ * @param entry the current entry, set to NULL to begin iteration
+ *
+ * @return The value of the current environment variable element
+ */
+const char *environment_var_next (struct environment *e,
+                                  const char *key,
+                                  const char *entry);
+
 /**
  * @brief Apply the changes encoded in this environment to the environment of
  * the current process.
@@ -86,6 +99,22 @@ void environment_push_back (struct environment *e,
                             const char *key,
                             const char *value);
 
+/**
+ * @brief Push "value" before position "before" in the environment variable
+ *  "key."
+ *
+ * @param e the environment in which to add this element
+ * @param key the environment variable name
+ * @param before the element before which to insert this value
+ * @param value the value to insert
+ *
+ * @return 0 on success -1 with errno set on failure.
+ */
+int environment_insert (struct environment *e,
+                        const char *key,
+                        char *before,
+                        const char *value);
+
 /**
  * @brief Add the specified value to the front of the target key without
  * de-duplication, it will still be separated from the rest of the value by sep,
diff --git a/src/common/libutil/test/environment.c b/src/common/libutil/test/environment.c
@@ -0,0 +1,81 @@
+/************************************************************\
+ * Copyright 2023 Lawrence Livermore National Security, LLC
+ * (c.f. AUTHORS, NOTICE.LLNS, COPYING)
+ *
+ * This file is part of the Flux resource manager framework.
+ * For details, see https://github.com/flux-framework.
+ *
+ * SPDX-License-Identifier: LGPL-3.0
+\************************************************************/
+
+#include <errno.h>
+
+#include "ccan/str/str.h"
+#include "src/common/libtap/tap.h"
+#include "src/common/libutil/environment.h"
+
+static void test_var_next ()
+{
+    const char *entry = NULL;
+    struct environment *e = environment_create ();
+    if (e == NULL)
+        BAIL_OUT ("Failed to create environment object!");
+    ok (environment_var_next (e, "PATH", NULL) == NULL,
+        "environment_var_next () returns NULL for missing env var");
+    environment_set (e, "PATH", "/bin:/usr/bin:/usr/local/bin", ':');
+    diag ("set PATH=/bin:/usr/bin:/usr/local/bin");
+    ok ((entry = environment_var_next (e, "PATH", entry)) != NULL,
+        "environment_var_next () works");
+    is (entry, "/bin",
+        "environment_var_next returns first element");
+    ok ((entry = environment_var_next (e, "PATH", entry)) != NULL,
+        "environment_var_next () works");
+    is (entry, "/usr/bin",
+        "environment_var_next returns next element");
+    ok ((entry = environment_var_next (e, "PATH", entry)) != NULL,
+        "environment_var_next () works");
+    is (entry, "/usr/local/bin",
+        "environment_var_next returns last element");
+    ok (!(entry = environment_var_next (e, "PATH", entry)),
+        "environment_var_next () returns NULL after last element");
+    environment_destroy (e);
+}
+
+static void test_insert ()
+{
+    const char *entry = NULL;
+    struct environment *e = environment_create ();
+    if (e == NULL)
+        BAIL_OUT ("Failed to create environment object!");
+    ok (environment_insert (e, "PATH", "/bin", "/foo") < 0 && errno == ENOENT,
+        "environment_insert on missing key returns ENOENT");
+    environment_set (e, "PATH", "/bin:/usr/bin:/usr/local/bin", ':');
+    diag ("set PATH=/bin:/usr/bin:/usr/local/bin");
+    diag ("searching for entry=/usr/bin");
+    while ((entry = environment_var_next (e, "PATH", entry)))
+        if (streq (entry, "/usr/bin"))
+            break;
+    diag ("entry=%s", entry);
+    ok (environment_insert (e, "PATH", (char *) entry, "/new/path") == 0,
+        "environment_insert /new/path before /usr/bin return success");
+    is (environment_get (e, "PATH"),
+        "/bin:/new/path:/usr/bin:/usr/local/bin",
+        "PATH is now /bin:/new/path:/usr/bin:/usr/local/bin");
+    environment_destroy (e);
+}
+
+int main (int argc, char *argv[])
+{
+    plan (NO_PLAN);
+
+    test_var_next ();
+    test_insert ();
+
+    done_testing ();
+
+    return 0;
+};
+
+/*
+ * vi:tabstop=4 shiftwidth=4 expandtab
+ */
diff --git a/t/t1102-cmddriver.t b/t/t1102-cmddriver.t
@@ -82,16 +82,50 @@ test_expect_success 'cmddriver removes multiple contiguous separators in input'
 		flux env sh -c 'echo \$LUA_PATH' | grep -v ';;;;')
 "
 readlink --version >/dev/null && test_set_prereq READLINK
-test_expect_success READLINK 'cmddriver adds its own path to PATH if called with relative path' "
-	fluxcmd=\$(readlink -f \$(which flux)) &&
-	fluxdir=\$(dirname \$fluxcmd) &&
-	PATH='/bin:/usr/bin' \$fluxcmd env sh -c 'echo \$PATH' | grep ^\$fluxdir
-"
-test_expect_success READLINK 'cmddriver moves its own path to the front of PATH' "
-	fluxcmd=\$(readlink -f \$(which flux)) &&
-	fluxdir=\$(dirname \$fluxcmd) &&
-	PATH=/bin:\$fluxdir \$fluxcmd env sh -c 'echo \$PATH' | grep ^\$fluxdir
-"
+
+
+# N.B.: In the tests below we need to ensure that /bin:/usr/bin appear
+# in PATH so that core utilities like `ls` can be found by the libtool
+# wrappers or other processes invoked by calling `flux`. However, this
+# means the results of the test can be influenced by whether or not a
+# flux executable appears in /bin or /usr/bin. Therefore, care must be
+# taken to ensure consistent results no matter what is in these paths.
+#
+# Ensure a bogus 'flux' executable occurs first in path, then make sure
+# command -v flux finds the right flux:
+#
+test_expect_success 'cmddriver adds its own path to PATH' '
+	mkdir bin &&
+	cat <<-EOF >bin/flux &&
+	#!/bin/sh
+	/bin/true
+	EOF
+	chmod +x bin/flux &&
+	fluxcmd=$(command -v flux) &&
+	result=$(PATH=$(pwd)/bin:/bin:/usr/bin \
+	         $fluxcmd env sh -c "command -v flux") &&
+	test_debug "echo result=$result" &&
+	test "$result" = "$fluxcmd"
+'
+# Use bogus flux in PATH and ensure flux cmddriver inserts its own path
+# just before this path, not at front of PATH.
+test_expect_success 'cmddriver inserts its path at end of PATH' '
+	fluxdir=$(dirname $fluxcmd) &&
+	result=$(PATH=/foo:$(pwd)/bin:/bin:/usr/bin \
+	         $fluxcmd env printenv PATH) &&
+	test_debug "echo result=$result" &&
+	test "$result" = "/foo:$fluxdir:$(pwd)/bin:/bin:/usr/bin"
+'
+# Ensure a PATH that already returns current flux first is not modified
+# by flux(1)
+test_expect_success READLINK 'cmddriver does not adjust PATH if unnecessary' '
+	fluxdir=$(dirname $fluxcmd) &&
+	printenv=$(command -v printenv) &&
+	mypath=/foo:/bar:$fluxdir:/usr/bin:/bin &&
+	newpath=$(PATH=$mypath $fluxcmd env $printenv PATH) &&
+	test_debug "echo PATH=$newpath" &&
+	test "$newpath" = "$mypath"
+'
 test_expect_success 'FLUX_*_PREPEND environment variables work' '
 	( FLUX_CONNECTOR_PATH_PREPEND=/foo \
 	  flux /usr/bin/printenv | grep "FLUX_CONNECTOR_PATH=/foo" &&
