diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 463112e48..23716cff0 100644 --- a/.github/workflows/main.yml +++ b/.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 diff --git a/build_tools/lsan_suppressions.txt b/build_tools/lsan_suppressions.txt new file mode 100644 index 000000000..2ba5ddae6 --- /dev/null +++ b/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 diff --git a/cmake/Rust.cmake b/cmake/Rust.cmake index 6b7170a26..bd836fed6 100644 --- a/cmake/Rust.cmake +++ b/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}" ) diff --git a/fish-rust/Cargo.toml b/fish-rust/Cargo.toml index e157678bc..849282d17 100644 --- a/fish-rust/Cargo.toml +++ b/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] diff --git a/fish-rust/src/threads.rs b/fish-rust/src/threads.rs index be6f45f31..fc096c7df 100644 --- a/fish-rust/src/threads.rs +++ b/fish-rust/src/threads.rs @@ -75,6 +75,8 @@ mod ffi { extern "Rust" { #[cxx_name = "make_detached_pthread"] fn spawn_ffi(callback: &SharedPtr) -> bool; + fn asan_before_exit(); + fn asan_maybe_exit(code: i32); } extern "Rust" { @@ -265,10 +267,12 @@ pub fn spawn(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) -> 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 { diff --git a/src/env_dispatch.cpp b/src/env_dispatch.cpp index ee4a3636a..9e4be10ba 100644 --- a/src/env_dispatch.cpp +++ b/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()) { diff --git a/src/fish.cpp b/src/fish.cpp index 0176985d7..eb0c258e3 100644 --- a/src/fish.cpp +++ b/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 } diff --git a/src/fish_tests.cpp b/src/fish_tests.cpp index 0ba5c6c70..0ee84aa27 100644 --- a/src/fish_tests.cpp +++ b/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; }