Merge pull request #9771 from mqudsi/asan_take5

Rework ASAN integration

.github/workflows/main.yml

@@ -112,15 +112,16 @@ jobs:
     - name: make test
       env:
           FISH_CI_SAN: 1
-          ASAN_OPTIONS: check_initialization_order=1:detect_stack_use_after_return=1:detect_leaks=1
+          ASAN_OPTIONS: check_initialization_order=1:detect_stack_use_after_return=1:detect_leaks=1:fast_unwind_on_malloc=0
           UBSAN_OPTIONS: print_stacktrace=1:report_error_type=1
           # use_tls=0 is a workaround for LSAN crashing with "Tracer caught signal 11" (SIGSEGV),
           # which seems to be an issue with TLS support in newer glibc versions under virtualized
           # environments. Follow https://github.com/google/sanitizers/issues/1342 and
           # https://github.com/google/sanitizers/issues/1409 to track this issue.
-          LSAN_OPTIONS: verbosity=0:log_threads=0:use_tls=0
+          # UPDATE: this can cause spurious leak reports for __cxa_thread_atexit_impl() under glibc.
+          LSAN_OPTIONS: verbosity=0:log_threads=0:use_tls=1:print_suppressions=0
       run: |
-        make test
+        env LSAN_OPTIONS="$LSAN_OPTIONS:suppressions=$PWD/build_tools/lsan_suppressions.txt" make test
 
   # Our clang++ tsan builds are not recognizing safe rust patterns (such as the fact that Drop
   # cannot be called while a thread is using the object in question). Rust has its own way of

build_tools/lsan_suppressions.txt

@@ -0,0 +1,6 @@
+# LSAN can detect leaks tracing back to __asan::AsanThread::ThreadStart (probably caused by our
+# threads not exiting before their TLS dtors are called). Just ignore it.
+leak:AsanThread
+
+# ncurses leaks allocations freely as it assumes it will be running throughout. Ignore these.
+leak:tparm

cmake/Rust.cmake

@@ -25,6 +25,7 @@ set(fish_rust_target "fish-rust")
 
 set(fish_autocxx_gen_dir "${CMAKE_BINARY_DIR}/fish-autocxx-gen/")
 
+set(FISH_CRATE_FEATURES "fish-ffi-tests")
 if(NOT DEFINED CARGO_FLAGS)
     # Corrosion doesn't like an empty string as FLAGS. This is basically a no-op alternative.
     # See https://github.com/corrosion-rs/corrosion/issues/356
@@ -32,11 +33,12 @@ if(NOT DEFINED CARGO_FLAGS)
 endif()
 if(DEFINED ASAN)
     list(APPEND CARGO_FLAGS "-Z" "build-std")
+    list(APPEND FISH_CRATE_FEATURES "asan")
 endif()
 
 corrosion_import_crate(
     MANIFEST_PATH "${CMAKE_SOURCE_DIR}/fish-rust/Cargo.toml"
-    FEATURES "fish-ffi-tests"
+    FEATURES "${FISH_CRATE_FEATURES}"
     FLAGS "${CARGO_FLAGS}"
 )
 

fish-rust/Cargo.toml

@@ -44,6 +44,7 @@ default = ["fish-ffi-tests"]
 fish-ffi-tests = ["inventory"]
 
 # The following features are auto-detected by the build-script and should not be enabled manually.
+asan = []
 bsd = []
 
 [patch.crates-io]

fish-rust/src/threads.rs

@@ -75,6 +75,8 @@ mod ffi {
     extern "Rust" {
         #[cxx_name = "make_detached_pthread"]
         fn spawn_ffi(callback: &SharedPtr<CppCallback>) -> bool;
+        fn asan_before_exit();
+        fn asan_maybe_exit(code: i32);
     }
 
     extern "Rust" {
@@ -265,10 +267,12 @@ pub fn spawn<F: FnOnce() + Send + 'static>(callback: F) -> bool {
     // We don't have to port the PTHREAD_CREATE_DETACHED logic. Rust threads are detached
     // automatically if the returned join handle is dropped.
 
-    let result = match std::thread::Builder::new().spawn(callback) {
+    let result = match std::thread::Builder::new().spawn(move || {
+        (callback)();
+    }) {
         Ok(handle) => {
-            let id = handle.thread().id();
-            FLOG!(iothread, "rust thread", id, "spawned");
+            let thread_id = handle.thread().id();
+            FLOG!(iothread, "rust thread", thread_id, "spawned");
             // Drop the handle to detach the thread
             drop(handle);
             true
@@ -299,6 +303,38 @@ fn spawn_ffi(callback: &cxx::SharedPtr<ffi::CppCallback>) -> bool {
     })
 }
 
+/// Exits calling onexit handlers if running under ASAN, otherwise does nothing.
+///
+/// This function is always defined but is a no-op if not running under ASAN. This is to make it
+/// more ergonomic to call it in general and also makes it possible to call it via ffi at all.
+pub fn asan_maybe_exit(#[allow(unused)] code: i32) {
+    #[cfg(feature = "asan")]
+    {
+        asan_before_exit();
+        unsafe {
+            libc::exit(code);
+        }
+    }
+}
+
+/// When running under ASAN, free up some allocations that would normally have been left for the OS
+/// to reclaim to avoid some false positive LSAN reports.
+///
+/// This function is always defined but is a no-op if not running under ASAN. This is to make it
+/// more ergonomic to call it in general and also makes it possible to call it via ffi at all.
+pub fn asan_before_exit() {
+    #[cfg(feature = "asan")]
+    if !is_forked_child() {
+        unsafe {
+            // Free ncurses terminal state
+            extern "C" {
+                fn env_cleanup();
+            }
+            env_cleanup();
+        }
+    }
+}
+
 /// Data shared between the thread pool [`ThreadPool`] and worker threads [`WorkerThread`].
 #[derive(Default)]
 struct ThreadPoolProtected {

src/env_dispatch.cpp

@@ -565,6 +565,15 @@ static bool does_term_support_setting_title(const environment_t &vars) {
     return true;
 }
 
+extern "C" {
+void env_cleanup() {
+    if (cur_term != nullptr) {
+        del_curterm(cur_term);
+        cur_term = nullptr;
+    }
+}
+}
+
 /// Initialize the curses subsystem.
 static void init_curses(const environment_t &vars) {
     for (const auto &var_name : curses_variables) {
@@ -580,6 +589,10 @@ static void init_curses(const environment_t &vars) {
         }
     }
 
+    // init_curses() is called more than once, which can lead to a memory leak if the previous
+    // ncurses TERMINAL isn't freed before initializing it again with `setupterm()`.
+    env_cleanup();
+
     int err_ret{0};
     if (setupterm(nullptr, STDOUT_FILENO, &err_ret) == ERR) {
         if (is_interactive_session()) {

src/fish.cpp

@@ -63,6 +63,7 @@ Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA
 #include "proc.h"
 #include "reader.h"
 #include "signals.h"
+#include "threads.rs.h"
 #include "wcstringutil.h"
 #include "wutil.h"  // IWYU pragma: keep
 
@@ -614,6 +615,7 @@ int main(int argc, char **argv) {
     if (debug_output) {
         fclose(debug_output);
     }
+    asan_maybe_exit(exit_status);
     exit_without_destructors(exit_status);
     return EXIT_FAILURE;  // above line should always exit
 }

src/fish_tests.cpp

@@ -96,6 +96,7 @@
 #include "signals.h"
 #include "smoke.rs.h"
 #include "termsize.h"
+#include "threads.rs.h"
 #include "tokenizer.h"
 #include "topic_monitor.h"
 #include "utf8.h"
@@ -6514,6 +6515,9 @@ int main(int argc, char **argv) {
     say(L"Encountered %d errors in low-level tests", err_count);
     if (s_test_run_count == 0) say(L"*** No Tests Were Actually Run! ***");
 
+    // If under ASAN, reclaim some resources before exiting.
+    asan_before_exit();
+
     if (err_count != 0) {
         return 1;
     }