From f7e7396c691ed90a68fc63bd7e029eab16cd15d2 Mon Sep 17 00:00:00 2001
From: ridiculousfish <rf@fishshell.com>
Date: Sat, 7 Oct 2023 15:20:09 -0700
Subject: [PATCH] Fix a deadlock affecting fish_config

This fixes the following deadlock. The C++ functions path_get_config and
path_get_data lazily determine paths and then cache those in a C++ static
variable. The path determination requires inspecting the environment stack.
If these functions are first called while the environment stack is locked
(in this case, when fetching the $history variable) we can get a deadlock.

The fix is to call them eagerly during env_init. This can be removed once
the corresponding C++ functions are removed.

This issue caused fish_config to fail to report colors and themes.

Add a test.
---
 fish-rust/src/env/environment.rs |  9 +++++++++
 src/env.cpp                      | 14 ++++++++++++++
 tests/checks/invocation.fish     |  3 +++
 3 files changed, 26 insertions(+)

diff --git a/fish-rust/src/env/environment.rs b/fish-rust/src/env/environment.rs
index ae323e2d3..385c40b63 100644
--- a/fish-rust/src/env/environment.rs
+++ b/fish-rust/src/env/environment.rs
@@ -520,6 +520,10 @@ fn setup_path() {
 /// This is a simple key->value map and not e.g. cut into paths.
 pub static INHERITED_VARS: OnceCell<HashMap<WString, WString>> = OnceCell::new();
 
+extern "C" {
+    fn env_cpp_init();
+}
+
 pub fn env_init(paths: Option<&ConfigPaths>, do_uvars: bool, default_paths: bool) {
     let vars = &PRINCIPAL_STACK;
 
@@ -586,6 +590,11 @@ pub fn env_init(paths: Option<&ConfigPaths>, do_uvars: bool, default_paths: bool
         }
     }
 
+    // See the env_cpp_init() comment for this.
+    unsafe {
+        env_cpp_init();
+    }
+
     // Set $USER, $HOME and $EUID
     // This involves going to passwd and stuff.
     vars.set_one(
diff --git a/src/env.cpp b/src/env.cpp
index 37aca7b75..be3abc054 100644
--- a/src/env.cpp
+++ b/src/env.cpp
@@ -148,6 +148,20 @@ void misc_init() {
     }
 }
 
+extern "C" {
+void env_cpp_init() {
+    // Temporary for the Rust port.
+    // path_get_config and path_get_data both inspect the environment stack to determine
+    // config paths, and then remember those paths in a static variable. This can lead to
+    // a deadlock if these functions are called while already holding the environment lock.
+    // Call them immediately to trigger the caching.
+    // This can be removed once path_get_config and path_get_data are removed.
+    wcstring dir;
+    path_get_config(dir);
+    path_get_data(dir);
+}
+}
+
 static std::map<wcstring, wcstring> inheriteds;
 
 const std::map<wcstring, wcstring> &env_get_inherited() { return inheriteds; }
diff --git a/tests/checks/invocation.fish b/tests/checks/invocation.fish
index f81a67ed0..94915937c 100644
--- a/tests/checks/invocation.fish
+++ b/tests/checks/invocation.fish
@@ -105,3 +105,6 @@ $fish --no-config -c 'echo notprinted | and true'
 # CHECKERR: echo notprinted | and true
 # CHECKERR:                   ^~^
 
+# Regression test for a hang.
+echo "set -L" | $fish > /dev/null
+