Fix #C-16: insert_by_ids accepts a documented-compliant short iterator - #25266
Open
kiana1kaslana wants to merge 1 commit into
Open
Fix #C-16: insert_by_ids accepts a documented-compliant short iterator#25266kiana1kaslana wants to merge 1 commit into
kiana1kaslana wants to merge 1 commit into
Conversation
…tor length `EntityWorldMut::insert_by_ids` is an `unsafe` API whose `# Safety` docs did not state that `iter_components` must yield exactly `component_ids.len()` items. Internally, `insert_dynamic_bundle` pairs the two via `zip`, which silently truncates to the shorter iterator. If the caller passes a short iterator, `Table::allocate` has already reserved space for all components but only some are written — leaving uninitialized memory that is later read as valid data (UB). Changes: 1. **`insert_by_ids` Safety docs** — add the missing contract: the iterator must yield exactly `component_ids.len()` items. Fewer → UB from uninitialized memory; more → silently ignored. 2. **`insert_dynamic_bundle` Safety docs** — state that both iterators must yield the same number of items. 3. **`insert_by_ids_internal` debug guard** — in debug builds, eagerly collect `iter_components` into a `Vec` and `debug_assert_eq!` its length against `component_ids.len()`. This catches the mismatch before any uninitialized memory is accessed. The check is removed entirely in release builds (zero overhead). 4. **Regression tests** — two `#[should_panic]` tests (debug-only) verifying that both short and long iterators trigger the assertion.
kiana1kaslana
changed the title
insert_by_ids soundness — document and debug-assert itera…
JaySpruce
added
C-Bug
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
insert_by_idsisunsafe, but its Safety docs never said the iterator has to match the length ofcomponent_ids. The internal pairing useszip, so a short iterator silently gets truncated whileTable::allocatehas already reserved all the columns. Reading them later is UB.Three changes in
crates/bevy_ecs/src/world/entity_access/:insert_by_idsto require an exact length match.insert_dynamic_bundleto say both iterators must match.insert_by_ids_internal, collect the iterator in debug builds anddebug_assert_eq!its length againstcomponent_ids.len(). Gated on#[cfg(debug_assertions)], so release builds are unchanged.Added two
#[should_panic]tests inentity_access::mod.rs:insert_by_ids_short_iterator_panics_in_debug(2 IDs, 1 component)insert_by_ids_long_iterator_panics_in_debug(1 ID, 2 components)All 71 existing
entity_accesstests still pass.cargo check -p bevy_ecsis clean in both debug and release.The other two callers of
insert_by_ids_internal(bundle::writerandentity::clone_entities) always pass matching lengths, so they're unaffected.