[Merged by Bors] - Fix double drop in BlobVec::replace_unchecked (#2597) #2848

sapir · 2021-09-20T00:30:13Z

Objective

I thought I'd have a go a trying to fix #2597.

Hopefully fixes #2597.

Solution

I reused the memory pointed to by the value parameter, that is already required by insert to not be dropped, to contain the extracted value while dropping it.

alice-i-cecile · 2021-09-22T21:35:56Z

Thanks! Is there a way to write a test for this?

sapir · 2021-09-23T00:00:47Z

Thanks! Is there a way to write a test for this?

I'll try to adapt the code snippet from the issue.

sapir · 2021-09-23T01:14:52Z

As it turns out, the test failed :) So I fixed another bug.

I also have a test for the second code snippet from the issue (#2597 (comment)) but it's still failing, I think due to Table::allocate increasing the column length before the data is actually written. Fixing that seems more complicated, so I'll just leave that test out and open a separate issue for it.

alice-i-cecile · 2021-09-23T02:05:51Z

Awesome, thanks for the test and the issue :)

BoxyUwU · 2021-10-10T15:15:05Z

crates/bevy_ecs/src/storage/blob_vec.rs

@@ -97,12 +98,18 @@ impl BlobVec {

    /// # Safety
    /// - index must be in-bounds
-    //  - memory must be previously initialized
+    /// - memory must be previously initialized


what memory? this should be clearer that we're talking about value (and also what type is supposed to be initialised behind it)

I changed it to say value but after seeing the same comment at initialize_unchecked I'm pretty sure this is referring to the memory at index/ptr, so I'll change it to say that

edit: I don't think I phrased it very clearly, though

BoxyUwU · 2021-10-10T15:19:23Z

crates/bevy_ecs/src/storage/blob_vec.rs

+    /// - the contents of `*value` must not be dropped by the caller. (In fact,
+    ///   this function may overwrite them.)


Suggested change

/// - the contents of `*value` must not be dropped by the caller. (In fact,

/// this function may overwrite them.)

/// - the contents of `*value` is left logically uninitialised/moved out of and as such

/// should not be used or dropped by the caller

BoxyUwU · 2021-10-10T15:22:09Z

crates/bevy_ecs/src/storage/blob_vec.rs

@@ -88,6 +88,7 @@ impl BlobVec {
    /// # Safety
    /// - index must be in bounds
    /// - memory must be reserved and uninitialized
+    /// - the contents of `*value` must not be dropped by the caller


I dont see why this is the case. this function does not drop the data that was written into value so if we aren't dropping the data behind value somewhere this would leak memory

*value is moved into the BlobVec, so if the caller drops it, that would cause a double drop when dropping the BlobVec or moving the value out again.

oh right this is copy not swap like the other function 🤦‍♀️

BoxyUwU · 2021-10-10T15:24:39Z

crates/bevy_ecs/src/storage/blob_vec.rs

+        // calling `drop`. Otherwise, if `drop` panics, then when the collection
+        // is dropped during stack unwinding, the collection's `Drop` impl will
+        // call `drop` again for the old value, so we get a double drop.
+        std::ptr::swap_nonoverlapping(value, ptr, self.item_layout.size());


I think it would be cheaper to set self.len to 0 before dropping and then set it back afterwards rather than using swap_nonoverlapping

DJMcNab

These seem correct - I'm not quite sure that the safety comments are quite as precise as I'd choose to make them, but these are strict improvements.

BoxyUwU

logic looks correct to me, the safety comments could be better but that's not the point of this PR and they're already an improvement.

DJMcNab · 2021-10-16T14:18:20Z

crates/bevy_ecs/src/bundle.rs

@@ -109,8 +109,8 @@ macro_rules! tuple_impl {
                #[allow(non_snake_case)]
                let ($(mut $name,)*) = self;
                $(
-                    func((&mut $name as *mut $name).cast::<u8>());
-                    std::mem::forget($name);
+                    let mut value = ::std::mem::ManuallyDrop::new($name);


ManuallyDrop isn't strong enough here - you need to use MaybeUninit. For example, see ea5e315

I'm happy to move this change into #2975

ok I removed this commit

cart · 2021-12-18T21:29:04Z

looks good to me. thanks!

cart · 2021-12-18T21:29:09Z

bors r+

# Objective I thought I'd have a go a trying to fix #2597. Hopefully fixes #2597. ## Solution I reused the memory pointed to by the value parameter, that is already required by `insert` to not be dropped, to contain the extracted value while dropping it.

bors · 2021-12-18T21:47:53Z

Pull request successfully merged into main.

Build succeeded:

github-actions bot added the S-Needs-Triage This issue needs to be labelled label Sep 20, 2021

sapir force-pushed the fix-blob-vec-double-drop branch 3 times, most recently from 8221c7f to 49b4071 Compare September 21, 2021 22:27

alice-i-cecile added A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior S-Needs-Review and removed S-Needs-Triage This issue needs to be labelled labels Sep 22, 2021

sapir mentioned this pull request Sep 23, 2021

Panic in drop leads to a double drop on spawning a new entity #2860

Open

sapir force-pushed the fix-blob-vec-double-drop branch from 956f072 to d423cce Compare September 23, 2021 01:22

alice-i-cecile approved these changes Sep 23, 2021

View reviewed changes

alice-i-cecile added the P-High This is particularly urgent, and deserves immediate attention label Oct 10, 2021

BoxyUwU suggested changes Oct 10, 2021

View reviewed changes

DJMcNab approved these changes Oct 10, 2021

View reviewed changes

alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review labels Oct 10, 2021

sapir force-pushed the fix-blob-vec-double-drop branch 2 times, most recently from 8ad92f9 to 1337eb1 Compare October 14, 2021 23:52

alice-i-cecile removed the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Oct 15, 2021

alice-i-cecile added this to the Bevy 0.6 milestone Oct 15, 2021

BoxyUwU approved these changes Oct 15, 2021

View reviewed changes

alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Oct 15, 2021

DJMcNab reviewed Oct 16, 2021

View reviewed changes

sapir force-pushed the fix-blob-vec-double-drop branch from 1337eb1 to 9a8ff08 Compare October 16, 2021 18:53

sapir added 2 commits October 16, 2021 23:45

Fix double drop in BlobVec::replace_unchecked (bevyengine#2597)

5a95716

Add a test for components that panic while being dropped

b499744

sapir force-pushed the fix-blob-vec-double-drop branch from 9a8ff08 to b499744 Compare October 16, 2021 20:50

bors bot changed the title ~~Fix double drop in BlobVec::replace_unchecked (#2597)~~ [Merged by Bors] - Fix double drop in BlobVec::replace_unchecked (#2597) Dec 18, 2021

bors bot closed this Dec 18, 2021

sapir deleted the fix-blob-vec-double-drop branch December 18, 2021 21:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Fix double drop in BlobVec::replace_unchecked (#2597) #2848

[Merged by Bors] - Fix double drop in BlobVec::replace_unchecked (#2597) #2848

sapir commented Sep 20, 2021

alice-i-cecile commented Sep 22, 2021

sapir commented Sep 23, 2021

sapir commented Sep 23, 2021

alice-i-cecile commented Sep 23, 2021

BoxyUwU Oct 10, 2021

sapir Oct 14, 2021 •

edited

Loading

BoxyUwU Oct 10, 2021

BoxyUwU Oct 10, 2021

bjorn3 Oct 10, 2021

BoxyUwU Oct 10, 2021

BoxyUwU Oct 10, 2021

DJMcNab left a comment

BoxyUwU left a comment

DJMcNab Oct 16, 2021

sapir Oct 16, 2021

cart commented Dec 18, 2021

cart commented Dec 18, 2021

bors bot commented Dec 18, 2021

		/// - the contents of `*value` must not be dropped by the caller. (In fact,
		/// this function may overwrite them.)

[Merged by Bors] - Fix double drop in BlobVec::replace_unchecked (#2597) #2848

[Merged by Bors] - Fix double drop in BlobVec::replace_unchecked (#2597) #2848

Conversation

sapir commented Sep 20, 2021

Objective

Solution

alice-i-cecile commented Sep 22, 2021

sapir commented Sep 23, 2021

sapir commented Sep 23, 2021

alice-i-cecile commented Sep 23, 2021

Choose a reason for hiding this comment

sapir Oct 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DJMcNab left a comment

Choose a reason for hiding this comment

BoxyUwU left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cart commented Dec 18, 2021

cart commented Dec 18, 2021

bors bot commented Dec 18, 2021

sapir Oct 14, 2021 •

edited

Loading