From c84b6fda66a69553b13118948cc9eb62d8a3de1a Mon Sep 17 00:00:00 2001 From: Thom Chiovoloni Date: Mon, 6 May 2024 17:15:48 -0700 Subject: [PATCH 1/5] Finish leak doc/tests --- src/arc_str.rs | 41 +++++++++++++++++++++++++++++------------ 1 file changed, 29 insertions(+), 12 deletions(-) diff --git a/src/arc_str.rs b/src/arc_str.rs index eedea83..ae38a59 100644 --- a/src/arc_str.rs +++ b/src/arc_str.rs @@ -537,27 +537,44 @@ impl ArcStr { } } - /// Convert the `ArcStr` into a `'static` `ArcStr`, even if it was - /// originally created from runtime values. + /// Convert the `ArcStr` into a "static" `ArcStr`, even if it was originally + /// created from runtime values. The `&'static str` is returned. /// - /// This is useful + /// This is useful if you want to use [`ArcStr::as_static`] or + /// [`ArcStr::is_static`] on a value only known at runtime. /// - /// If the `ArcStr` is already static, then + /// If the `ArcStr` is already static, then this is a noop. + /// + /// # Caveats + /// Calling this function on an ArcStr will cause us to never free it, thus + /// leaking it's memory. Doing this excessively can lead to problems. + /// + /// # Examples + /// ``` + /// # use arcstr::ArcStr; + /// let s = ArcStr::from("foobar"); + /// assert!(!ArcStr::is_static(&s)); + /// assert!(ArcStr::as_static(&s).is_none()); + /// + /// let leaked: &'static str = s.leak(); + /// assert!(ArcStr::is_static(&s)); + /// assert_eq!(ArcStr::as_static(&s), Some("foobar")); + /// ``` #[inline] - pub fn leak(this: &Self) -> &'static str { - if Self::has_static_lenflag(this) { - return unsafe { Self::to_static_unchecked(this) }; + pub fn leak(&self) -> &'static str { + if Self::has_static_lenflag(self) { + return unsafe { Self::to_static_unchecked(self) }; } let is_static_count = unsafe { // Not sure about ordering, maybe relaxed would be fine. - Self::load_count_flag_raw(this, Ordering::Acquire) + Self::load_count_flag_raw(self, Ordering::Acquire) }; if is_static_count.flag_part() { - return unsafe { Self::to_static_unchecked(this) }; + return unsafe { Self::to_static_unchecked(self) }; } - unsafe { Self::become_static(this, is_static_count.uint_part() == 1) }; - debug_assert!(Self::is_static(this)); - unsafe { Self::to_static_unchecked(this) } + unsafe { Self::become_static(self, is_static_count.uint_part() == 1) }; + debug_assert!(Self::is_static(self)); + unsafe { Self::to_static_unchecked(self) } } unsafe fn become_static(this: &Self, is_unique: bool) { From f90c4a20acca0d4b5fef29e19e4ef6e8af092e72 Mon Sep 17 00:00:00 2001 From: Thom Chiovoloni Date: Mon, 6 May 2024 17:25:43 -0700 Subject: [PATCH 2/5] Suppress the leakcheckers for ArcStr::leak's doctest --- .github/workflows/ci.yml | 4 ++++ lsan_suppressions.txt | 2 ++ src/arc_str.rs | 5 +++++ 3 files changed, 11 insertions(+) create mode 100644 lsan_suppressions.txt diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 520ab53..d406bee 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -142,6 +142,9 @@ jobs: miri: name: Miri runs-on: ubuntu-latest + env: + # the tests for `ArcStr::leak` intentionally leak memory. + MIRIFLAGS: -Zmiri-ignore-leaks steps: - uses: actions/checkout@v4 - uses: hecrj/setup-rust-action@v2 @@ -181,6 +184,7 @@ jobs: RUST_BACKTRACE: 0 # only used by asan, but we set it for all of them cuz its easy ASAN_OPTIONS: detect_stack_use_after_return=1 + LSAN_OPTIONS: "suppressions=lsan_suppressions.txt" strategy: fail-fast: false matrix: diff --git a/lsan_suppressions.txt b/lsan_suppressions.txt new file mode 100644 index 0000000..2b2d1ff --- /dev/null +++ b/lsan_suppressions.txt @@ -0,0 +1,2 @@ +# This is the `ArcStr::leak` example doctest +leak:function_that_leaks diff --git a/src/arc_str.rs b/src/arc_str.rs index ae38a59..7b53a69 100644 --- a/src/arc_str.rs +++ b/src/arc_str.rs @@ -551,6 +551,9 @@ impl ArcStr { /// /// # Examples /// ``` + /// # // Wrap this in a function so that we can suppress + /// # // only this leak in the asan tests + /// # fn function_that_leaks() { /// # use arcstr::ArcStr; /// let s = ArcStr::from("foobar"); /// assert!(!ArcStr::is_static(&s)); @@ -559,6 +562,8 @@ impl ArcStr { /// let leaked: &'static str = s.leak(); /// assert!(ArcStr::is_static(&s)); /// assert_eq!(ArcStr::as_static(&s), Some("foobar")); + /// # } + /// # function_that_leaks() /// ``` #[inline] pub fn leak(&self) -> &'static str { From 8dd79c096c7c5eabcd8d2ba062689259fecc6400 Mon Sep 17 00:00:00 2001 From: Thom Chiovoloni Date: Mon, 6 May 2024 17:35:12 -0700 Subject: [PATCH 3/5] Try to suppress the leakchecker for ArcStr::leak's test in a different way --- lsan_suppressions.txt | 2 +- src/arc_str.rs | 11 +++++------ tests/arc_str.rs | 12 ++++++++++++ 3 files changed, 18 insertions(+), 7 deletions(-) diff --git a/lsan_suppressions.txt b/lsan_suppressions.txt index 2b2d1ff..cd47c67 100644 --- a/lsan_suppressions.txt +++ b/lsan_suppressions.txt @@ -1,2 +1,2 @@ # This is the `ArcStr::leak` example doctest -leak:function_that_leaks +leak:test_leaking diff --git a/src/arc_str.rs b/src/arc_str.rs index 7b53a69..cea71a8 100644 --- a/src/arc_str.rs +++ b/src/arc_str.rs @@ -550,20 +550,19 @@ impl ArcStr { /// leaking it's memory. Doing this excessively can lead to problems. /// /// # Examples - /// ``` - /// # // Wrap this in a function so that we can suppress - /// # // only this leak in the asan tests - /// # fn function_that_leaks() { + /// ```no_run + /// # // This isn't run because it needs a leakcheck suppression, + /// # // which I can't seem to make work in CI (no symbols for + /// # // doctests?). Instead, we test this in tests/arc_str.rs /// # use arcstr::ArcStr; /// let s = ArcStr::from("foobar"); /// assert!(!ArcStr::is_static(&s)); /// assert!(ArcStr::as_static(&s).is_none()); /// /// let leaked: &'static str = s.leak(); + /// assert_eq!(leaked, s); /// assert!(ArcStr::is_static(&s)); /// assert_eq!(ArcStr::as_static(&s), Some("foobar")); - /// # } - /// # function_that_leaks() /// ``` #[inline] pub fn leak(&self) -> &'static str { diff --git a/tests/arc_str.rs b/tests/arc_str.rs index 57f010d..76c98a0 100644 --- a/tests/arc_str.rs +++ b/tests/arc_str.rs @@ -353,3 +353,15 @@ fn repeat_string() { fn repeat_string_panics() { ArcStr::repeat("AAA", usize::MAX); } + +#[test] +fn test_leaking() { + let s = ArcStr::from("foobar"); + assert!(!ArcStr::is_static(&s)); + assert!(ArcStr::as_static(&s).is_none()); + + let leaked: &'static str = s.leak(); + assert_eq!(leaked, s); + assert!(ArcStr::is_static(&s)); + assert_eq!(ArcStr::as_static(&s), Some("foobar")); +} From 3c85a3b27f5eaee317ce3facb5b7fc128486af01 Mon Sep 17 00:00:00 2001 From: Thom Chiovoloni Date: Mon, 6 May 2024 17:40:36 -0700 Subject: [PATCH 4/5] Try the manual approach to suppressing this test under asan --- .github/workflows/ci.yml | 3 +++ lsan_suppressions.txt | 2 -- tests/arc_str.rs | 3 +++ 3 files changed, 6 insertions(+), 2 deletions(-) delete mode 100644 lsan_suppressions.txt diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d406bee..0baa20c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -194,6 +194,9 @@ jobs: include: - sanitizer: memory extra_rustflags: "-Zsanitizer-memory-track-origins" + - sanitizer: address + # to disable the ArcStr::leak test (can't get suppressions to work in CI) + extra_rustflags: "--cfg=asan" steps: - uses: actions/checkout@v4 diff --git a/lsan_suppressions.txt b/lsan_suppressions.txt deleted file mode 100644 index cd47c67..0000000 --- a/lsan_suppressions.txt +++ /dev/null @@ -1,2 +0,0 @@ -# This is the `ArcStr::leak` example doctest -leak:test_leaking diff --git a/tests/arc_str.rs b/tests/arc_str.rs index 76c98a0..46eb35d 100644 --- a/tests/arc_str.rs +++ b/tests/arc_str.rs @@ -4,6 +4,7 @@ // yep, we create owned instance just for comparison, to test comparison // with owned instacnces. clippy::cmp_owned, + unexpected_cfgs, )] use arcstr::ArcStr; @@ -355,6 +356,8 @@ fn repeat_string_panics() { } #[test] +#[allow(unknown_lints)] +#[cfg_attr(asan, ignore)] // Leaks memory intentionally fn test_leaking() { let s = ArcStr::from("foobar"); assert!(!ArcStr::is_static(&s)); From 609bdb3675b67ff2bd9214ad5ae479bf59608ad7 Mon Sep 17 00:00:00 2001 From: Thom Chiovoloni Date: Mon, 6 May 2024 17:49:06 -0700 Subject: [PATCH 5/5] Add a loom model that checks for a race between leak and drop --- src/arc_str.rs | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/arc_str.rs b/src/arc_str.rs index cea71a8..9ad3550 100644 --- a/src/arc_str.rs +++ b/src/arc_str.rs @@ -1678,4 +1678,20 @@ mod loomtest { t2.join().unwrap(); }); } + + #[test] + fn leak_drop() { + loom::model(|| { + let a1 = ArcStr::from("foo"); + let a2 = a1.clone(); + + let t1 = thread::spawn(move || { + drop(a1); + }); + let t2 = thread::spawn(move || a2.leak()); + t1.join().unwrap(); + let leaked: &'static str = t2.join().unwrap(); + assert_eq!(leaked, "foo"); + }); + } }