patina_internal_collections: Fix bounds checking in Storage::resize()

[garybeihl]

Description

Fixed resize() to prevent capacity shrinking and properly copy all nodes during resizing.

The root issue was that resize() allowed shrinking to a buffer smaller than the current capacity. This caused out-of-bounds access when the resize loop tried to copy node pointers. The storage layout can have gaps like [VALID | VALID | INVALID | VALID | INVALID] where deleted nodes remain in the buffer at their original indices.

Changes:

1. Changed assert from buffer.len() >= self.len() to buffer.len() >= self.capacity() to prevent resizing to a buffer smaller than current capacity
2. Changed loop from 0..self.len() to 0..self.capacity() to ensure all nodes including gaps are copied

• Impacts functionality?
• Impacts security?
• Breaking change?
• Includes tests?
• Includes documentation?

How This Was Tested

• Added test_resize_prevents_capacity_shrink(): Verifies assert prevents shrinking capacity
• Added test_resize_copies_all_nodes_including_gaps(): Verifies all nodes copied including gaps
• All 49 unit tests passing (cargo test --package patina_internal_collections)
• Cargo clippy and fmt passing

Integration Instructions

• N/A

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[makubacki]

@garybeihl, for future reference to this PR. This is adding additional cases for the change previously made in a single location in #1243, right?

[makubacki]

@garybeihl, for future reference to this PR. This is adding additional cases for the change previously made in a single location in #1243, right?

Nvm, I see your comment in the previous PR now, that is the case. Thanks for updating these too.

[Javagedes]

Resize requires that the new buffer is always equal to or larger than the old buffer. I think the bug is that you are even able to resize to a lower capacity.

We have an assert in the resize code that should be preventing resizing to a lower capacity

[Javagedes]

We cannot inherently resize to a lower capacity because there is no guarantee of ordering of nodes. Loosing any active node due to a resize to a lower capacity will actively destroy the tree.

I guess that is, unless we resize capacity to a lower capacity that is still greater than the current size. But I thought my currently implementation just directly moves everything to the new buffer as is. We don't actually build a brand new tree.

[garybeihl]

We have an assert in the resize code that should be preventing resizing to a lower capacity

Shouldn't that assert be assert!(buffer.len() >= self.capacity()) instead of assert!(buffer.len() >= self.len());?

[Javagedes]

We have an assert in the resize code that should be preventing resizing to a lower capacity

Shouldn't that assert be assert!(buffer.len() >= self.capacity()) instead of assert!(buffer.len() >= self.len());?

Yes, I agree that we should change it to that. I think that is the true bug. The buffer that makes up all of the nodes does not guarantee having all of the valid nodes at the start of the buffer. Looking at the Storage code, you can see that we "delete" a node by simply clearing its pointers and adding it to the linked list that tracks available nodes for use (

patina/core/patina_internal_collections/src/node.rs

Line 122 in f921e1e

pub fn delete(&mut self, node: *mut Node<D>) {

).

That means the buffer looks something like this:

[VALID | VALID | INVALID | VALID | INVALID | VALID | VALID].

So on top of us needing to change the assert above, as you suggested, we also need to loop through all nodes (self.capacity()) during self.resize(), not just self.size().

[garybeihl]

Sounds good. I've made those changes and re-worded the PR title to more accurately reflect the changes made. Thanks for your guidance on this.

[Javagedes]

Thank you for fixing all of my mistakes. This is great work :)

[Javagedes]

@garybeihl can you rename resize to expand? I think that will make it clear that we cannot shrink it. Maybe a bit of documentation about that in the function docs too

[garybeihl]

@garybeihl can you rename resize to expand? I think that will make it clear that we cannot shrink it. Maybe a bit of documentation about that in the function docs too

Done - please have a look - thanks!

[Javagedes]

@garybeihl can you rename resize to expand? I think that will make it clear that we cannot shrink it. Maybe a bit of documentation about that in the function docs too

Done - please have a look - thanks!

@garybeihl Looks good to me. There are a few documentation examples that look like they need updated. But once that is done I will merge :)