StaticVec::extend panics because of out of range memory access.

[Wasabi375]

The get_unchecked_mut call within drop_in_place sometimes panics.
When the capacity of the extended vector is not enough to hold all extra values extend_ex tries to drop the rest. However the calculation on what this rest is, is wrong. It should use the length of the iter variable and not the capacity (as it is currently doing).

The following patch fixes this:

diff --git a/src/trait_impls.rs b/src/trait_impls.rs
index 8e912f2..b127f6d 100644
--- a/src/trait_impls.rs
+++ b/src/trait_impls.rs
@@ -215,16 +215,21 @@ impl<T, const N1: usize, const N2: usize> ExtendEx<T, StaticVec<T, N1>> for Stat
         .as_ptr()
         .copy_to_nonoverlapping(self.mut_ptr_at_unchecked(old_length), added_length);
       self.set_len(old_length + added_length);
-      // Wrap the values in a MaybeUninit to inhibit their destructors (if any),
-      // then manually drop any excess ones. This is the same kind of "trick" as
-      // is used in `new_from_array`, as you may or may not have noticed.
-      let mut forgotten = MaybeUninit::new(iter);
-      ptr::drop_in_place(
-        forgotten
-          .assume_init_mut()
-          .as_mut_slice()
-          .get_unchecked_mut(N1.min(N2)..N1),
-      );
+
+      let drop_range = N1.min(N2)..iter.len().try_into().unwrap();
+      if !drop_range.is_empty() {
+          // Wrap the values in a MaybeUninit to inhibit their destructors (if any),
+          // then manually drop any excess ones. This is the same kind of "trick" as
+          // is used in `new_from_array`, as you may or may not have noticed.
+          let mut forgotten = MaybeUninit::new(iter);
+
+          ptr::drop_in_place(
+              forgotten
+                  .assume_init_mut()
+                  .as_mut_slice()
+                  .get_unchecked_mut(drop_range),
+          );
+      }
     }
   }

And here is a test to reproduce the error

#[cfg(test)]
mod test {
    use crate::StaticVec;

    #[test]
    fn test_extend_drop_no_panic() {
        let mut a = StaticVec::<u64, 6>::new();
        let mut b = StaticVec::<u64, 6>::new();

        a.extend_from_slice(&[1, 2]);
        b.extend_from_slice(&[3, 4, 5]);

        // This panics
        a.extend(b);

        assert_eq!(a, [1, 2, 3, 4, 5]);
    }
}

I would send a PR but #60 is blocking any real changes.

[slightlyoutofphase]

To be completely honest, I haven't written any Rust code for a WHILE now, as I essentially became extremely frustrated with the fact that having an always-run-on-Miri test suite literally longer than the one for std::vec (last time I checked at least) still wasn't good enough for some people. And the fact that people acted as though I wasn't, in fact, actively interested in fixing any bugs that did relate to use of unsafe code. I just felt like nobody actually ever cared about context and solely operated on the basis of "trust me bro, random ass people in California who aren't even older than the dude who wrote this crate are the only people on Earth who know how to write correct code".

All of that said: given we know this crate literally doesn't compile anymore on latest nightly (and hasn't for a while), can you give some info on like, what version you were running at the time you compiled your example for reproducing the issue? It had always been my intent to come back to this whenever they stopped screwing around with the syntax for const generics. And I still might someday.

[slightlyoutofphase]

Also, I don't think I claim that this crate "never panics", at that? Like I guess what I should ask is, what does Miri in particular say as far as the problem you're highlighting here, when it occurs? I'm not really concerned about anything beyond that.

[Wasabi375]

To be completely honest, I haven't written any Rust code for a WHILE now, as I essentially became extremely frustrated with the fact that having an always-run-on-Miri test suite literally longer than the one for std::vec (last time I checked at least) still wasn't good enough for some people. And the fact that people acted as though I wasn't, in fact, actively interested in fixing any bugs that did relate to use of unsafe code. I just felt like nobody actually ever cared about context and solely operated on the basis of "trust me bro, random ass people in California who aren't even older than the dude who wrote this crate are the only people on Earth who know how to write correct code".

I get your frustrations. Open source can be a thankless job. I did not intend to imply that you don't care or should do the work to keep this crate working. My personal opinion on OSS is that maintainers should do only do the work that they personally rely on or that they enjoy doing (assuming they are unpaid).
This crate has been immensely helpful and I am fine with fixing any issues I find.

All of that said: given we know this crate literally doesn't compile anymore on latest nightly (and hasn't for a while), can you give some info on like, what version you were running at the time you compiled your example for reproducing the issue?

I am using my own fork of this library. I am playing around with all kind of funky stuff with generics, e.g. I replaced the size field with a generic that allows for any singed/unsinged number type. I don't think most of my changes are valuable so I haven't filled any issues/PRs so far. You can check it out here: https://github.com/Wasabi375/WasabiOS/tree/fs/staticvec
I am currently on a pretty recent nightly version cargo 1.86.0-nightly (2928e3273 2025-02-07).
I actually have the compilation issues fixed in my branch but I have so many other changes that it would be more work to pull them out than to just fix this here again.

Also, I don't think I claim that this crate "never panics", at that? Like I guess what I should ask is, what does Miri in particular say as far as the problem you're highlighting here, when it occurs? I'm not really concerned about anything beyond that.

This is caught in the standard library as a unsafe precondition:

assert_unsafe_precondition!(
            check_library_ub,
            "slice::get_unchecked_mut requires that the range is within the slice",
            (
                start: usize = self.start,
                end: usize = self.end,
                len: usize = slice.len()
            ) => end >= start && end <= len
        );

https://github.com/rust-lang/rust/blob/097cd98869cf07a2860bef16c0d4a2b3b019b23a/library/core/src/slice/index.rs#L412C5-L428C6

This causes a panic in debug builds and is just ignored with release. Miri just detects the panic.
This is the panic message:

thread 'trait_impls::test::test_extend_drop_no_panic' panicked at library/core/src/panicking.rs:218:5:
unsafe precondition(s) violated: slice::get_unchecked_mut requires that the range is within the slice

It's a simple out of bounds memory access.

[slightlyoutofphase]

Thanks for the well-stated feedback. I'll have to have a more in-depth look at your fork - it seems quite significant in terms of overall changes (in a way where it's like, I can't entirely tell at a quick glance which stuff you did that was a needed compilation fix and which was more kinda an opinionated tweak) but at the very least your fix for this particular issue seems solid. Basically TLDR I think I definitely need to get more up to speed on the current "state of bleeding edge nightly" in terms of const generics syntax and whatnot to really make any serious decisions on how to do a full main branch update of the crate, whenever I get around to it.

[slightlyoutofphase]

Looking a bit closer, I assume your reason for essentially nuking a lot of the std feature stuff in one commit was basically just related to your exact use case of what seems to be an embedded OS that would never need that? Just trying to wrap my head around everything, and like again to be clear I appreciate that you've found usefulness for the crate and have even fixed it even just for your own use case, definitely nice to see lol.

[Wasabi375]

Looking a bit closer, I assume your reason for essentially nuking a lot of the std feature stuff in one commit was basically just related to your exact use case of what seems to be an embedded OS that would never need that?

Yeah. I just wanted a working solution for my use case and the easiest way to achieve that was to just delete everything that I could not use anyways.

I suggest we move this discussion into another issue(maybe #60 ) as it is not really relevant to this bug.

[slightlyoutofphase]

I'll keep this open anyways as whatever issue you might have truly unearthed here is basically irrelevant until I can at least compile the crate with the same functionality it had before the massive nightly syntax change that pretty much caused me to set it aside.