From c43e040c7c38bf4dfd4f755a2cf3bff6ccba1f2a Mon Sep 17 00:00:00 2001 From: Mahmoud Al-Qudsi Date: Mon, 1 May 2023 17:37:44 -0500 Subject: [PATCH 1/7] Fix spurious ASAN __cxa_thread_atexit_impl() leaks Set use_tls back to its default of 1. This is required to work around an ASAN/LSAN virtualization bug but seems to be behind the random __cxa_thread_atexit_impl() leaks? --- .github/workflows/main.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 463112e48..7921ac406 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -118,7 +118,8 @@ jobs: # 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 run: | make test From 3651e0e9d80abbe74e99bd10519dd6dbf751c08b Mon Sep 17 00:00:00 2001 From: Mahmoud Al-Qudsi Date: Sat, 29 Apr 2023 12:07:59 -0500 Subject: [PATCH 2/7] Actually report ASAN memory leaks The new asan exit handlers are called to get proper ASAN leak reports (as calling _exit(0) skips the LSAN reporting stage and exits with success every time). They are no-ops when not compiled for ASAN. --- cmake/Rust.cmake | 4 +++- fish-rust/Cargo.toml | 1 + fish-rust/src/threads.rs | 35 ++++++++++++++++++++++++++++++++--- src/fish.cpp | 2 ++ src/fish_tests.cpp | 4 ++++ 5 files changed, 42 insertions(+), 4 deletions(-) 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..877a248ec 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,31 @@ 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() { + } +} + /// Data shared between the thread pool [`ThreadPool`] and worker threads [`WorkerThread`]. #[derive(Default)] struct ThreadPoolProtected { 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; } From 905430629d767b81d68adbe77917206991c3243b Mon Sep 17 00:00:00 2001 From: Mahmoud Al-Qudsi Date: Mon, 1 May 2023 19:59:45 -0500 Subject: [PATCH 3/7] Use ASAN_OPTIONS fast_unwind_on_malloc=0 This is much slower but gives proper stack traces for calls emanating from code that wasn't compiled with -fno-omit-frame-pointer. --- .github/workflows/main.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 7921ac406..6970ded03 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -112,7 +112,7 @@ 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 From 73983bada50feb2bf9bb6b6beb99eb243f7c12cc Mon Sep 17 00:00:00 2001 From: Mahmoud Al-Qudsi Date: Mon, 1 May 2023 12:04:26 -0500 Subject: [PATCH 4/7] Fix ncurses memory leak in init_curses() init_curses() is/can be called more than once, in which case the previous ncurses terminal state is leaked and a new one is allocated. `del_curterm(cur_term)` is supposed to be called prior to calling `setupterm()` if `setupterm()` is being used to reinit the default `TERMINAL *cur_term`. --- src/env_dispatch.cpp | 13 +++++++++++++ 1 file changed, 13 insertions(+) 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()) { From 91485c90caee331c612ae20d366cfdc087592abb Mon Sep 17 00:00:00 2001 From: Mahmoud Al-Qudsi Date: Mon, 1 May 2023 20:23:15 -0500 Subject: [PATCH 5/7] Also free ncurses terminal state when exiting under ASAN --- fish-rust/src/threads.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/fish-rust/src/threads.rs b/fish-rust/src/threads.rs index 877a248ec..fc096c7df 100644 --- a/fish-rust/src/threads.rs +++ b/fish-rust/src/threads.rs @@ -325,6 +325,13 @@ pub fn asan_maybe_exit(#[allow(unused)] code: i32) { pub fn asan_before_exit() { #[cfg(feature = "asan")] if !is_forked_child() { + unsafe { + // Free ncurses terminal state + extern "C" { + fn env_cleanup(); + } + env_cleanup(); + } } } From 7b0cc33f2ed7680cdba003e0678bdc8408147d15 Mon Sep 17 00:00:00 2001 From: Mahmoud Al-Qudsi Date: Mon, 1 May 2023 20:27:38 -0500 Subject: [PATCH 6/7] Add LSAN suppressions file Suppress TLS variable leaks caused by outstanding background threads by suppressing the ASAN interposer functions. This is possible because because we're now using use_tls=1. ----------------------- Direct leak of 64 byte(s) in 2 object(s) allocated from: #0 0x5627a1f0cc86 in __interceptor_realloc (/home/runner/work/fish-shell/fish-shell/build/fish_tests+0xb9fc86) (BuildId: da87d16730727369ad5fa46052d10337d6941fa9) #1 0x7f04d8800f79 in pthread_getattr_np (/lib/x86_64-linux-gnu/libc.so.6+0x95f79) (BuildId: 69389d485a9793dbe873f0ea2c93e02efaa9aa3d) #2 0x5627a1f2f664 in __sanitizer::GetThreadStackTopAndBottom(bool, unsigned long*, unsigned long*) (/home/runner/work/fish-shell/fish-shell/build/fish_tests+0xbc2664) (BuildId: da87d16730727369ad5fa46052d10337d6941fa9) #3 0x5627a1f2fb83 in __sanitizer::GetThreadStackAndTls(bool, unsigned long*, unsigned long*, unsigned long*, unsigned long*) (/home/runner/work/fish-shell/fish-shell/build/fish_tests+0xbc2b83) (BuildId: da87d16730727369ad5fa46052d10337d6941fa9) #4 0x5627a1f19a0d in __asan::AsanThread::SetThreadStackAndTls(__asan::AsanThread::InitOptions const*) (/home/runner/work/fish-shell/fish-shell/build/fish_tests+0xbaca0d) (BuildId: da87d16730727369ad5fa46052d10337d6941fa9) #5 0x5627a1f19615 in __asan::AsanThread::Init(__asan::AsanThread::InitOptions const*) (/home/runner/work/fish-shell/fish-shell/build/fish_tests+0xbac615) (BuildId: da87d16730727369ad5fa46052d10337d6941fa9) #6 0x5627a1f19b01 in __asan::AsanThread::ThreadStart(unsigned long long) (/home/runner/work/fish-shell/fish-shell/build/fish_tests+0xbacb01) (BuildId: da87d16730727369ad5fa46052d10337d6941fa9) #7 0x7f04d87ffb42 (/lib/x86_64-linux-gnu/libc.so.6+0x94b42) (BuildId: 69389d485a9793dbe873f0ea2c93e02efaa9aa3d) #8 0x7f04d88919ff (/lib/x86_64-linux-gnu/libc.so.6+0x1269ff) (BuildId: 69389d485a9793dbe873f0ea2c93e02efaa9aa3d) --- .github/workflows/main.yml | 4 ++-- build_tools/lsan_suppressions.txt | 6 ++++++ 2 files changed, 8 insertions(+), 2 deletions(-) create mode 100644 build_tools/lsan_suppressions.txt diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 6970ded03..23716cff0 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -119,9 +119,9 @@ jobs: # environments. Follow https://github.com/google/sanitizers/issues/1342 and # https://github.com/google/sanitizers/issues/1409 to track this issue. # UPDATE: this can cause spurious leak reports for __cxa_thread_atexit_impl() under glibc. - LSAN_OPTIONS: verbosity=0:log_threads=0:use_tls=1 + 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 From 40be27c0025f8f6ae52e0b63b11e9bcee479d27a Mon Sep 17 00:00:00 2001 From: Mahmoud Al-Qudsi Date: Tue, 2 May 2023 13:13:11 -0500 Subject: [PATCH 7/7] Avoid unnecessary vector shift in re::regex_make_anchored() There's no reason to inject prefix into our newly allocated str after storing pattern in there. Just allocate with the needed capacity up front and then insert in the correct order. --- fish-rust/src/re.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/fish-rust/src/re.rs b/fish-rust/src/re.rs index 5431d726d..95ecc5d0d 100644 --- a/fish-rust/src/re.rs +++ b/fish-rust/src/re.rs @@ -4,12 +4,12 @@ use crate::wchar::{wstr, WString, L}; /// This is a workaround for the fact that PCRE2_ENDANCHORED is unavailable on pre-2017 PCRE2 /// (e.g. 10.21, on Xenial). pub fn regex_make_anchored(pattern: &wstr) -> WString { - let mut anchored = pattern.to_owned(); // PATTERN -> ^(:?PATTERN)$. let prefix = L!("^(?:"); let suffix = L!(")$"); - anchored.reserve(pattern.len() + prefix.len() + suffix.len()); - anchored.insert_utfstr(0, prefix); + let mut anchored = WString::with_capacity(prefix.len() + pattern.len() + suffix.len()); + anchored.push_utfstr(prefix); + anchored.push_utfstr(pattern); anchored.push_utfstr(suffix); anchored }