[tooson9010-spec]

Found while researching panic-safety in Rust containers.

Summary

Three places destroy or move components before committing the state that says
which slots are live. Component has no bound excluding a panicking Drop, so
a caller only needs to catch the unwind and touch the container again. All three
are double-drops (CWE-415) reachable from safe Rust.

Location	Skipped commit	Second drop
Archetype::clear	self.len = 0	Drop for Archetype calls clear again
Archetype::remove	self.len = last	the slot already dropped, and the duplicate left by copy_nonoverlapping
CommandBuffer::run_on	self.components.clear()	CommandBuffer::clear drops components already moved into the world

run_on already carries a comment saying "Wipe out component references so
clear doesn't try to double-free". It just does not run when a command
unwinds.

Fix

clear takes the length before running any destructor. Components not reached
are leaked, which is safe.

remove runs the loop under a guard. On unwind the guard finishes the backfill
for the remaining types without running further destructors, moves the entity
id, and commits the length. The dropped slot is overwritten by the moved entity
and the duplicate falls out of range, so the archetype is left consistent.
Components of the removed entity past the panic are leaked.

run_on tracks how many components the world has taken and splits the two
groups in a guard. Those already handed over are dropped from the list without
running destructors, the rest are destroyed normally. No leak on this path.

World::despawn is not covered. It updates its entity index from the value
remove returns, and that is skipped when the unwind passes through, so the
moved entity keeps a stale index entry. That is a logical inconsistency, not
memory unsafety. I left it alone since the right shape looks like your call.

Verification

Three regression tests count destructor invocations. Against the unpatched
code clear runs 7 destructors where 4 components exist, run_on runs 3
where 2 exist, and despawn runs 8 where 6 exist. With the fix clear
destroys 3 of the 4 and run_on 2 of 2; the despawn test asserts only
that the count stays at or below 6, since how far the loop gets before the
unwind depends on the order the archetype stores its types in.

The double-frees were confirmed earlier with AddressSanitizer (attempting double-free) on the unpatched code, through World::clear,
World::despawn, World::spawn_at and World::spawn_column_batch_at.
Existing tests pass and cargo build --no-default-features still builds.
Confirmed on 0.11.1.

[Ralith]

Thanks!

[Ralith]

Each test runs on its own thread, so a thread-local gives the same isolation without allocating.

Is this still true when passing --test-threads=1, or do we need to explicitly reinitialize the thread-locals in every test that uses them?

[Ralith]

Also, for easier review, please squash changes from feedback into the commits that were changed.

[tooson9010-spec]

Sorry again for the slow reply.

You're right, it isn't. The tests reset the counters at the start now, and the
suite passes with --test-threads=1.

Also squashed the feedback commits into the ones they changed.

[Ralith]

Sorry again for the slow reply.

You're fine, we're all volunteers here, participating as time permits.

[tooson9010-spec]

@Ralith
Thanks for taking the time to review this. Once it's released, would it be alright if I file a RustSec advisory so users on older versions are notified?

[Ralith]

Sure, if you like. It's difficult to imagine a realistic scenario in which this could be deliberately triggered and present an actual security hazard, so the severity should be recorded accordingly, but it's certainly not technically impossible.