patina_internal_collections: Fix UB memory read in Node fields

[garybeihl]

Description

Fixes an undefined behavior issue where Cell::set() reads uninitialized memory during linked list creation in Storage::resize().

Root Cause

• Cell::set() internally uses mem::replace(), which reads the old value before writing the new one.
• When Storage::resize() allocates new nodes and calls build_linked_list(), the Cell fields contain uninitialized memory.
• Reading uninitialized memory is undefined behavior, even if immediately overwritten. Unwanted compiler "optimizations" could follow.

Impact

• Any package using patina_internal_collections.
• Potential memory corruption and non-deterministic errors
• Detected by Miri testing (issue Consider running unit tests (or a subset of them) under Miri #560)

Fix

• Initialize all Cell fields using ptr::write() before build_linked_list()
• Use addr_of_mut!() to read field pointers without creating references to uninitialized data

Introduced by

• PR Replace AtomicPtr with Cell in patina_internal_collections. #1050 (Nov 13, 2025) Replace AtomicPtr with Cell in patina_internal_collections
• AtomicPtr::store() writes without reading the old value, but Cell::set() uses mem::replace() which reads before writing

Related to #560

• [X ] Impacts functionality?
• [ X] Impacts security?
• Breaking change?
• Includes tests?
• Includes documentation?

How This Was Tested

• Tested with: cargo +nightly-2025-09-19 miri test -p patina_dxe_core.
• 7 tests now pass (previously 0/469 due to this UB issue).

Integration Instructions

N/A

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[os-d]

Couldn't this be:

for node in buffer {
  node.color = Cell::new(BLACK);
  node.parent = Cell::new(null);
  ...
}

i.e. we can avoid the pointer manipulation?

[joschock]

Also, recommend making the values consistent with the defaults in Node::new(). i.e. Cell::new(RED).

[garybeihl]

Couldn't this be:

for node in buffer {
  node.color = Cell::new(BLACK);
  node.parent = Cell::new(null);
  ...
}

i.e. we can avoid the pointer manipulation?

But then in that case buffer still contains uninitialized memory, right? So for node in buffer creates &mut Node<D> references to uninitialized memory, which is still UB, even if immediately overwritten. node.color = Cell::new(BLACK) creates a reference &mut Node<D> and then the assignment operator reads the old value, just like mem::replace reads uninitialized memory. We need to get a raw pointer to the field without creating a reference to uninitialized memory, hence the pointer manipulation. I agree it's not very intuitive...

[os-d]

Does miri complain on this? Maybe a case for MaybeUninit so that doesn't happen? I'm speculating.

[joschock]

Also, recommend making the values consistent with the defaults in Node::new(). i.e. Cell::new(RED).

[joschock]

I think this needs to be done in all cases, not just the capacity == 0 case. In fact, I think it might need to be done before the let buffer at the beginning of the call.

[garybeihl]

Yes - I agree. We also need to update the test for this. Currently, it only takes the capacity==0 path,

[Javagedes]

@garybeihl

I need to look at this more in depth, but if this is the solution, it should probably be completed in Self::build_linked_list. This function is already looping through each uninitialized node to set up the linked list of available nodes for allocations. A simple solution would just be to initialize it there while setting up the linked list. It also stops the need to writing null_ptr to the left and right nodes because we are setting them (except for the first and last node in the linked list)

[os-d]

I had thought the same thing, but build_linked_list gets called when the list is expected to be initialized, it looks like.

[Javagedes]

also @joschock I think this was an issue before your change. My implementation always had these nodes as only partially initialized because the logic was I only need the linked list of available nodes (so left and right ptr). Nothing else was necessary. When we go to use a node, we popped it and fully initialized it for use.

So theoretically it is safe, but if misused, would definitely be UB. So adding this initialization is for the best.

[Javagedes]

I had thought the same thing, but build_linked_list gets called when the list is expected to be initialized, it looks like.

Nah, build_linked_list does not need to be initialized because build_linked_list is not reading any data from the initialized node. It is only setting the left and right pointers.

[joschock]

I think this code creates a mut ref to the slice without initializing the nodes within is technically UB already. So I don't think you can defer it to build_linked_list unless you defer creation of the slice to there.

let buffer = unsafe {
            slice::from_raw_parts_mut::<'a, Node<D>>(
                slice as *mut [u8] as *mut Node<D>,
                slice.len() / mem::size_of::<Node<D>>(),
            )
        };

[Javagedes]

@garybeihl can you post the actual full miri test failure?

[garybeihl]

The full backtrace miri test output is >250K. The gist is that resize calls build_linked_list on uninitialized data (buffer). build_linked_list calls Cell::set_right which calls Cell::set on uninitialized data. Cell::set does a read before write which triggers the UB.

To reproduce the error, install miri with

    rustup component add miri --toolchain nightly-2025-09-19

Run the test with Stacked Borrows disabled to see the definite UB:

MIRIFLAGS="-Zmiri-backtrace=full -Zmiri-disable-stacked-borrows" \
cargo +nightly-2025-09-19 miri test -p patina_dxe_core --lib \
allocate_deallocate_test                                                     │

[Javagedes]

Another option is you could require D implements default then just do this:

for i in 0..self.data.len() {
    unsafe { self.data.as_mut_ptr().add(i).write(Node::new(D::default())); }
}

[garybeihl]

That would work I think, but you'd be changing the API by adding the D: Default trait and you'd waste some CPU with all the new calls to Default. If that is what the consensus favors, however I can try to implement that path.

[joschock]

Without something like Default, it's not clear how to initialize D. Since it's generic, we can't assume that filling it with zeros is legit, for example. The UB is not just about accessing uninitialized data; I suspect it would be possible to construct a D such that the compiler would make poor choices based on assumptions of initialization that are not upheld, even without a direct read to the corresponding uninit memory. I think the read is just where Miri happens to detect the UB.

If we don't do Default, we might have to do data: Option<D> so that we can make data the well-defined value of None, and then make the slice with uninitialized nodes:

        let buffer = unsafe {
            let unint_buffer = slice::from_raw_parts_mut::<<'a, Node<D>>(
                slice as *mut [u8] as *mut <'a, Node<D>,
                slice.len() / mem::size_of::<Node<D>>(),
            )
            for i in 0..unint_buffer .len() {
              // new_empty makes a Node with data = None, and everything else defaults.
              unint_buffer .as_mut_ptr().add(i).write(Node::new_empty()); } 
            }
        };

[garybeihl]

Shall I redo the PR switching to the D: Default trait method?

[joschock]

I want other opinions, but mine is that Default is unnecessarily constraining on D when what we are trying to solve is essentially an internal problem of the Node implementation. I would go with the Option<D> approach instead.

I also need to study more, but I suspect we need to make the unint_buffer a slice of MaybeUninit<Node<D>> as well, and initialize it before transmuting it to a slice of Node<D>. I think (though my confidence is not great) that simply doing a slice from raw parts on uninitialized Node<D> might be UB.

[Javagedes]

There are many options I would like to investigate before considering Node<Option<D>>. Using option is going to have multiple issues:

1. I'm guessing that is going to get you stuck with more Cell wrappers because you may not have mutable access to the node to set D once we've initialized it.
2. It's going to make you have to unwrap D even though we know it exists
3. It is most likely going to change the memory layout as rust will need to add a discernment for the option.

I would like to see usage of MaybeUninit at all levels investigated, e.g. MaybeUninit<[Node<D>]>, &[MaybeUninit<Node<D>>], and even Node<MaybeUnit<D>>. At a bare minimum, MaybeUninit is guaranteed to keep the same memory layout and alignment.

[Javagedes]

Hi @garybeihl , I noticed you've been keeping this PR up to date, but not working on it (which is completely fine).

I just wanted to make sure you were not waiting on additional information from any of us that have commented on this PR (i.e. make sure we are not blocking you in any way)!

[garybeihl]

No - not blocked - I uploaded a version of the changes that uses MaybeUninit and D: Default - just waiting for further comments or change requests. If you could have a look when you get a chance, that would be great - thanks!