unwind safety fixes by Mirko-A · Pull Request #140 · Rust-for-Linux/pin-init

Mirko-A · 2026-04-27T18:48:36Z

Adds unwind-safety fixes to in-place initialization helpers so partially initialized values are dropped on both errors and panics. Changes:

Add __internal::ArrayInit guard type that safely initializes an array by running an initializer on each element, keeping track of the number of initialized elements. When dropped, if the initialization was partially successful, the guard drops the initialized portion of the array. Use the guard in [pin_]init_array_from_fn.
Use __internal::DropGuard in [pin_]chain so a panic/error in the chained closure drops the value initialized in the first stage; mem::forget the guard on success.
Add regression tests.

Closes: #136

The approach taken here was suggested in the issue itself, thus I added a Suggested-by tag. I hope that is okay.

Edits:
v1 -> v2:

rename ArrayInitGuard to ArrayInit
implement [Pin]Init for ArrayInit instead of wrapping it in [pin_]init_from_closure

v2 -> v3:

do not mem::forget the ArrayInit guard on success; state that the guard will only drop initialized elements on partial success

nbdd0121 · 2026-05-04T14:46:44Z

+///
+/// Drops the already initialized elements of the array if an error or panic occurs
+/// partway through the initialization process.
+pub struct ArrayInit<T, F> {


Suggested change

pub struct ArrayInit<T, F> {

pub(crate) struct ArrayInit<T, F> {

nbdd0121 · 2026-05-04T14:48:11Z

+            unsafe { init.__init(ptr) }?;
+            self.num_init += 1;
+        }
+        core::mem::forget(self);


This incorrectly forgets F. You should just set self.ptr to null.

Ouch. This is fixed, thank you for pointing it out.

nbdd0121 · 2026-05-04T14:48:36Z

+        // SAFETY: Safety contract of `ArrayInit` guarantees that elements
+        // `self.ptr[0..self.num_init]` are initialized and contain valid `T`
+        // values, so dropping them is safe.


This should be represented as invariants of the type.

I've added an Invariants section to ArrayInit docs, I hope it is acceptable.

nbdd0121 · 2026-05-04T14:52:30Z

        let val = unsafe { Pin::new_unchecked(val) };
-        // SAFETY: `slot` was initialized above.
-        (self.1)(val).inspect_err(|_| unsafe { core::ptr::drop_in_place(slot) })
+        (self.1)(val)?;


The whole implementation here can probably be simplified to something like

(self.1)(unsafe { Slot::new(slot) }.init(self.0).let_binding()) or something similar if #143 lands.

I could rebase this branch to dev/accessor-rework if you'd like? Otherwise I don't mind waiting for #143 to land.

nbdd0121 · 2026-05-04T14:53:24Z

+}
+
+impl<T, F> ArrayInit<T, F> {
+    /// # Safety


Use whatever you actually require for safety requirement. This function can probably not be unsafe at all.

Removed unsafe from ArrayInit::new.

nbdd0121 · 2026-05-05T13:43:19Z

I cannot review diff of diffs. Please always squash your changes to individually meaningful commits.

Mirko-A · 2026-05-05T13:53:23Z

I see, everything has been squashed to the original three commits.

When reviewing, I prefer having diffs that do exactly what each of the comments suggests and squashing after the review. I will squash future fixups immediately.

nbdd0121 · 2026-05-05T14:04:05Z

For GitHub-centric workflow that's indeed convenient, but for this project as the commits are synced to the kernel I need to review each commit individually so it's up to kernel's quality requirement.

Honestly the actual issue is that GitHub does not provide a good way to visualize the diff, despite that it has all the needed information to render as such. The "Compare" button sometimes does the job, but it breaks when you rebase and fixup commits in a single push. Longer term, either we can wait until GitHub implements it (which is quite unlikely given the slop), or we can adopt what the Rust Project's triagebot does by automatically generates a range diff like this.

Mirko-A · 2026-05-05T14:08:22Z

I understand and fully agree. Unfortunately, looking at recent trends at GitHub, I don't expect much in terms of improvements to the platform.

Thank you for taking the time to explain your reasoning. :)

Adds a guard type that safely initializes an array by running an initializer on each element, keeping track of the number of initialized elements. In the case of a panic or error in the per-element initializer, the guard drops the already-initialized portion of the array; nothing is dropped on success. The previous code only ran cleanup on the explicit error path. If the per- element initializer panicked partway through, the elements already written into the array would be leaked: their `Drop` impls would never run. Link: Rust-for-Linux#136 Reported-by: Gary Guo <gary@garyguo.net> Suggested-by: Gary Guo <gary@garyguo.net> Signed-off-by: Mirko Adzic <adzicmirko97@gmail.com>

Adds a drop guard before the call to the chained closure so that the value initialized by the first stage is dropped if the closure errors or panics; `mem::forget` the guard on success. The previous code only ran cleanup on the explicit error path, leaking the first-stage value if the chained closure panicked. Link: Rust-for-Linux#136 Reported-by: Gary Guo <gary@garyguo.net> Suggested-by: Gary Guo <gary@garyguo.net> Signed-off-by: Mirko Adzic <adzicmirko97@gmail.com>

Cover both fixes added in the series: - `[pin_]init_array_from_fn`: a panic or error from element `i`'s initializer drops the previously initialized elements `0..i`. - `[pin_]chain`: a panic or error from the chained closure drops the value initialized by the first stage. Also assert no double-drop on the success paths. Signed-off-by: Mirko Adzic <adzicmirko97@gmail.com>

nbdd0121 · 2026-05-07T11:10:45Z

+/// - `ptr` is null:
+///   - before `__init` or `__pinned_init` is called
+///   - after the array is successfully initialized
+/// - `ptr[0..num_init]` contains initialized elements of type `T`
+/// - `ptr[num_init..N]` (where N is the size of the array) contains uninitialized memory


Suggested change

/// - `ptr` is null:

/// - before `__init` or `__pinned_init` is called

/// - after the array is successfully initialized

/// - `ptr[0..num_init]` contains initialized elements of type `T`

/// - `ptr[num_init..N]` (where N is the size of the array) contains uninitialized memory

/// If `ptr` is not null:

/// - `ptr[0..num_init]` contains initialized elements of type `T`

/// - `ptr[num_init..N]` (where N is the size of the array) contains uninitialized memory

nbdd0121 · 2026-05-07T11:12:44Z

+}
+
+impl<T, F> ArrayInit<T, F> {
+    pub(crate) fn new(make_init: F) -> Self {


Needs ``#[inline]`

nbdd0121 · 2026-05-07T11:14:51Z

+            // SAFETY: Since `0 <= i < N`, `self.ptr.add(i)` is in bounds and
+            // valid for writes by the safety contract of `__init`.


This is not calling __init yet. So just mention about inbounds.

nbdd0121 · 2026-05-07T11:15:24Z

+
+impl<T, F> ArrayInit<T, F> {
+    pub(crate) fn new(make_init: F) -> Self {
+        Self {


Need // INVARIANT

nbdd0121 · 2026-05-07T11:18:20Z

+            // SAFETY: The pointer is derived from `slot` and thus satisfies the
+            // `__init` requirements.
+            unsafe { init.__init(ptr) }?;
+            self.num_init += 1;


I'd actually either put self.num_init = i; at the beginning of the loop, or explicitly specify self.num_init = 0 at the beginning of the function.

nbdd0121 · 2026-05-07T11:19:02Z

+            // valid for writes by the safety contract of `__init`.
+            let ptr = unsafe { self.ptr.add(i) };
+            // SAFETY: The pointer is derived from `slot` and thus satisfies the
+            // `__init` requirements.


This needs to be more explicit and refers to the safety requirements of Init::__init.

nbdd0121 · 2026-05-07T11:19:21Z

+    F: FnMut(usize) -> I,
+    I: PinInit<T, E>,
+{
+    unsafe fn __pinned_init(mut self, slot: *mut [T; N]) -> Result<(), E> {


Ditto for this function

nbdd0121 · 2026-05-07T11:23:16Z

+        // SAFETY: Invariants of `ArrayInit` guarantee that elements
+        // `self.ptr[0..self.num_init]` are initialized and contain valid `T`
+        // values, so dropping them is safe.


Perhaps also mention before the safety comment that "In this code path, the initialization has failed partway and we need to drop elements initialized thus far to uphold the pinning requirement."

nbdd0121 · 2026-05-07T11:25:48Z

+static DROPS: AtomicUsize = AtomicUsize::new(0);
+/// Serialize tests so the shared [DROPS] counter stays meaningful even
+/// when cargo runs tests in parallel.
+static LOCK: Mutex<()> = Mutex::new(());


Could you make Counted something like:

struct Counted<'a>(&'a AtomicUsize);

to avoid things like this?

nbdd0121 · 2026-05-07T11:25:57Z

+
+impl Drop for Counted {
+    fn drop(&mut self) {
+        DROPS.fetch_add(1, Ordering::SeqCst);


Mirko-A force-pushed the mirko/fix-unwind-safety-bug branch from 235fcfb to 45e29c0 Compare April 27, 2026 19:38

Mirko-A marked this pull request as ready for review April 27, 2026 20:00

nbdd0121 added the soundness Related to soundness issues label Apr 28, 2026

Mirko-A force-pushed the mirko/fix-unwind-safety-bug branch 2 times, most recently from 7ad7985 to 225f13b Compare May 1, 2026 23:11

nbdd0121 reviewed May 2, 2026

View reviewed changes

Comment thread src/__internal.rs Outdated

Mirko-A force-pushed the mirko/fix-unwind-safety-bug branch from e45f41e to e3b2cb6 Compare May 3, 2026 12:45

Mirko-A commented May 3, 2026

View reviewed changes

Comment thread src/__internal.rs Outdated

nbdd0121 reviewed May 4, 2026

View reviewed changes

Mirko-A force-pushed the mirko/fix-unwind-safety-bug branch from e3b2cb6 to e5f75a9 Compare May 5, 2026 13:35

Mirko-A force-pushed the mirko/fix-unwind-safety-bug branch from e5f75a9 to 5c83836 Compare May 5, 2026 13:49

Mirko-A added 3 commits May 5, 2026 16:09

Mirko-A force-pushed the mirko/fix-unwind-safety-bug branch from 5c83836 to eebe948 Compare May 5, 2026 14:11

nbdd0121 reviewed May 7, 2026

View reviewed changes

	pub struct ArrayInit<T, F> {
	pub(crate) struct ArrayInit<T, F> {

		// SAFETY: Since `0 <= i < N`, `self.ptr.add(i)` is in bounds and
		// valid for writes by the safety contract of `__init`.

Conversation

Mirko-A commented Apr 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nbdd0121 commented May 5, 2026

Uh oh!

Mirko-A commented May 5, 2026

Uh oh!

nbdd0121 commented May 5, 2026

Uh oh!

Mirko-A commented May 5, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

Mirko-A commented Apr 27, 2026 •

edited

Loading