From 25300c0ecdedcd8720b996b41e78dfe1037bfcff Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?F=C3=A9lix=20Baylac=20Jacqu=C3=A9?=
 <felix@alternativebit.fr>
Date: Wed, 1 Mar 2023 20:01:36 +0100
Subject: [PATCH] Treat empty env var paths as unset

We make sure the env var paths are actually set (ie. not "") before
sending them to the canonicalization function. If we forget to do so,
the user will end up facing a puzzled failed assertion internal error.

We issue a non-failing warning as a stop-gap measure. We could want to
revisit this to issue a detailed failing error message in the future.
---
 src/libstore/build/local-derivation-goal.cc |  2 +-
 src/libstore/globals.cc                     | 14 +++++++-------
 src/libutil/util.cc                         | 12 ++++++++++++
 src/libutil/util.hh                         |  4 ++++
 4 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc
index a961d8eed..1c0860993 100644
--- a/src/libstore/build/local-derivation-goal.cc
+++ b/src/libstore/build/local-derivation-goal.cc
@@ -2063,7 +2063,7 @@ void LocalDerivationGoal::runChild()
 
             /* The tmpDir in scope points at the temporary build directory for our derivation. Some packages try different mechanisms
                to find temporary directories, so we want to open up a broader place for them to dump their files, if needed. */
-            Path globalTmpDir = canonPath(getEnv("TMPDIR").value_or("/tmp"), true);
+            Path globalTmpDir = canonPath(getEnvNonEmpty("TMPDIR").value_or("/tmp"), true);
 
             /* They don't like trailing slashes on subpath directives */
             if (globalTmpDir.back() == '/') globalTmpDir.pop_back();
diff --git a/src/libstore/globals.cc b/src/libstore/globals.cc
index 8e33a3dec..fae79c1a0 100644
--- a/src/libstore/globals.cc
+++ b/src/libstore/globals.cc
@@ -30,15 +30,15 @@ static GlobalConfig::Register rSettings(&settings);
 
 Settings::Settings()
     : nixPrefix(NIX_PREFIX)
-    , nixStore(canonPath(getEnv("NIX_STORE_DIR").value_or(getEnv("NIX_STORE").value_or(NIX_STORE_DIR))))
-    , nixDataDir(canonPath(getEnv("NIX_DATA_DIR").value_or(NIX_DATA_DIR)))
-    , nixLogDir(canonPath(getEnv("NIX_LOG_DIR").value_or(NIX_LOG_DIR)))
-    , nixStateDir(canonPath(getEnv("NIX_STATE_DIR").value_or(NIX_STATE_DIR)))
-    , nixConfDir(canonPath(getEnv("NIX_CONF_DIR").value_or(NIX_CONF_DIR)))
+    , nixStore(canonPath(getEnvNonEmpty("NIX_STORE_DIR").value_or(getEnvNonEmpty("NIX_STORE").value_or(NIX_STORE_DIR))))
+    , nixDataDir(canonPath(getEnvNonEmpty("NIX_DATA_DIR").value_or(NIX_DATA_DIR)))
+    , nixLogDir(canonPath(getEnvNonEmpty("NIX_LOG_DIR").value_or(NIX_LOG_DIR)))
+    , nixStateDir(canonPath(getEnvNonEmpty("NIX_STATE_DIR").value_or(NIX_STATE_DIR)))
+    , nixConfDir(canonPath(getEnvNonEmpty("NIX_CONF_DIR").value_or(NIX_CONF_DIR)))
     , nixUserConfFiles(getUserConfigFiles())
-    , nixBinDir(canonPath(getEnv("NIX_BIN_DIR").value_or(NIX_BIN_DIR)))
+    , nixBinDir(canonPath(getEnvNonEmpty("NIX_BIN_DIR").value_or(NIX_BIN_DIR)))
     , nixManDir(canonPath(NIX_MAN_DIR))
-    , nixDaemonSocketFile(canonPath(getEnv("NIX_DAEMON_SOCKET_PATH").value_or(nixStateDir + DEFAULT_SOCKET_PATH)))
+    , nixDaemonSocketFile(canonPath(getEnvNonEmpty("NIX_DAEMON_SOCKET_PATH").value_or(nixStateDir + DEFAULT_SOCKET_PATH)))
 {
     buildUsersGroup = getuid() == 0 ? "nixbld" : "";
     lockCPU = getEnv("NIX_AFFINITY_HACK") == "1";
diff --git a/src/libutil/util.cc b/src/libutil/util.cc
index 885bae69c..5377f093b 100644
--- a/src/libutil/util.cc
+++ b/src/libutil/util.cc
@@ -54,6 +54,18 @@ std::optional<std::string> getEnv(const std::string & key)
     return std::string(value);
 }
 
+std::optional<std::string> getEnvNonEmpty(const std::string & key) {
+    auto value = getEnv(key);
+    if (value == "") {
+        // TODO: determine whether this should be a warning or an error.
+        logWarning({
+                .msg = hintfmt("ignoring the '%1%' env variable, its value has been set to \"\"", key)
+         });
+        return std::nullopt;
+    } else {
+        return value;
+    }
+}
 
 std::map<std::string, std::string> getEnv()
 {
diff --git a/src/libutil/util.hh b/src/libutil/util.hh
index b5625ecef..3293c758f 100644
--- a/src/libutil/util.hh
+++ b/src/libutil/util.hh
@@ -39,6 +39,10 @@ extern const std::string nativeSystem;
 /* Return an environment variable. */
 std::optional<std::string> getEnv(const std::string & key);
 
+/* Return a non empty environment variable. Returns nullopt if the env
+variable is set to "" */
+std::optional<std::string> getEnvNonEmpty(const std::string & key);
+
 /* Get the entire environment. */
 std::map<std::string, std::string> getEnv();