Fix unsafe access of component metadata (change ticks) in bevy_ecs::storage::Table

[nils-mathieu]

Objective

This pull requests simply changes three functions in bevy_ecs/src/storage/table/mod.rs which currently may incorrectly access an out of bound element.

Currently those functions use the following pattern to guard against invalid TableRow:

/// Get the specific [`change tick`](Tick) of the component matching `component_id` in `row`.
pub fn get_changed_tick(
  &self,
  component_id: ComponentId,
  row: TableRow,
) -> Option<&UnsafeCell<Tick>> {
  (row.index_u32() < self.entity_count()).then_some(
    // SAFETY: `row.as_usize()` < `len`
    unsafe {
      self.get_column(component_id)?
        .changed_ticks
        .get_unchecked(row.index())
    },
  )
}

The usage of then_some is invalid here because it takes its input by value, which evaluates the get_unchecked call instantly even if row.index_u32() < self.entity_count() is false.

Calling get_unchecked with an invalid index triggers instant undefined behavior even if the value is not used.

Solution

We can't really use then instead of then_some because self.get_column can fail by returning None, so I opted for a regular if guard like this:

/// Get the specific [`change tick`](Tick) of the component matching `component_id` in `row`.
pub fn get_changed_tick(
  &self,
  component_id: ComponentId,
  row: TableRow,
) -> Option<&UnsafeCell<Tick>> {
  if row.index_u32() >= self.entity_count() {
    return None;
  }

  // SAFETY: `row.as_usize()` < `len`
  self.get_column(component_id)
    .map(|col| unsafe { col.changed_ticks.get_unchecked(row.index()) })
}

Testing

I tried to run the CI on the whole thing but my version of clippy (nightly) is triggered by other parts of the code and I was too lazy to install Rust stable and re-compile everything again. I will fix anything that fails in CI here and re-check everything locally if it turns out to fail.

Other Notes

I'm not familiar with the codebase (well, I've been reading an poking around for a bit but I don't know everything), so there might be other places in the code with the same issue. I grep'ed then_some and found no other instance of the same problem, so at least it doesn't appear anywhere else in this form.

[github-actions]

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

[nils-mathieu]

Not sure why the macos build times out. Is that an issue on the runner's end?

[kfc35]

Nice work! Appreciate the easy-to-understand PR description. Thanks for your first contribution 🙏

[mockersf]

Could this have an impact on performances? Maybe not run the whole benchmarks, but could you check the code generated for costly differences?

[nils-mathieu]

@mockersf

Could this have an impact on performances? Maybe not run the whole benchmarks, but could you check the code generated for costly differences?

I would naively expect the "success" case to have the same performance as it does the same things (check row index + access), and the "failure" case the have better performance because we're no longer performing the access at all in that case.

I'm not sure how to check the generated code though.

That being said, I suspect that the compiler was actually doing the following:

1. Check if the table has a column for component_id. If not bail out correctly.
2. See the get_unchecked which requires row < len to be sound.
3. Conclude that row < len must always be true and that therefore row.index_u32() < self.entity_count() is redundant.
4. Remove row.index_u32() < self.entity_count() entirely and only keep self.get_column(component_id).map(|col| unsafe { col.changed_ticks.get_unchecked(row.index()) })

If it was doing that, then this would technically be a regression performance-wise? I would be surprised if it's even measurable