Indices::extend (#16023)

# Objective

There's integer overflow in `Mesh::merge` in branches like this:


405fa3e8ea/crates/bevy_mesh/src/mesh.rs (L857-L859)

we truncate `u32` to `u16` and ignore integer overflow on `u16`. This
may lead to unexpected results when the number of vertices exceeds
`u16::MAX`.

## Solution

Convert indices storage to `u32` when necessary.

## Testing

- Unit test added for `extend` function
- For changes in `Mesh`, I presume it is already tested elsewhere
This commit is contained in:
Stepan Koltsov 2024-10-31 16:39:32 +00:00 committed by GitHub
parent db4a74be76
commit 928dee830e
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
2 changed files with 52 additions and 25 deletions

View file

@ -103,19 +103,36 @@ impl Indices {
/// Add an index. If the index is greater than `u16::MAX`, /// Add an index. If the index is greater than `u16::MAX`,
/// the storage will be converted to `u32`. /// the storage will be converted to `u32`.
pub fn push(&mut self, index: u32) { pub fn push(&mut self, index: u32) {
self.extend([index]);
}
}
/// Extend the indices with indices from an iterator.
/// Semantically equivalent to calling [`push`](Indices::push) for each element in the iterator,
/// but more efficient.
impl Extend<u32> for Indices {
fn extend<T: IntoIterator<Item = u32>>(&mut self, iter: T) {
let mut iter = iter.into_iter();
match self { match self {
Indices::U32(vec) => vec.push(index), Indices::U32(indices) => indices.extend(iter),
Indices::U16(vec) => match u16::try_from(index) { Indices::U16(indices) => {
Ok(index) => vec.push(index), indices.reserve(iter.size_hint().0);
Err(_) => { while let Some(index) = iter.next() {
let new_vec = vec match u16::try_from(index) {
.iter() Ok(index) => indices.push(index),
.map(|&index| u32::from(index)) Err(_) => {
.chain(iter::once(index)) let new_vec = indices
.collect::<Vec<u32>>(); .iter()
*self = Indices::U32(new_vec); .map(|&index| u32::from(index))
.chain(iter::once(index))
.chain(iter)
.collect::<Vec<u32>>();
*self = Indices::U32(new_vec);
break;
}
}
} }
}, }
} }
} }
} }
@ -181,4 +198,27 @@ mod tests {
indices.iter().collect::<Vec<_>>() indices.iter().collect::<Vec<_>>()
); );
} }
#[test]
fn test_indices_extend() {
let mut indices = Indices::U16(Vec::new());
indices.extend([10, 11]);
assert_eq!(IndexFormat::Uint16, IndexFormat::from(&indices));
assert_eq!(vec![10, 11], indices.iter().collect::<Vec<_>>());
// Add a value that is too large for `u16` so the storage should be converted to `U32`.
indices.extend([12, 0x10013, 0x10014]);
assert_eq!(IndexFormat::Uint32, IndexFormat::from(&indices));
assert_eq!(
vec![10, 11, 12, 0x10013, 0x10014],
indices.iter().collect::<Vec<_>>()
);
indices.extend([15, 0x10016]);
assert_eq!(IndexFormat::Uint32, IndexFormat::from(&indices));
assert_eq!(
vec![10, 11, 12, 0x10013, 0x10014, 15, 0x10016],
indices.iter().collect::<Vec<_>>()
);
}
} }

View file

@ -842,20 +842,7 @@ impl Mesh {
// Extend indices of `self` with indices of `other`. // Extend indices of `self` with indices of `other`.
if let (Some(indices), Some(other_indices)) = (self.indices_mut(), other.indices()) { if let (Some(indices), Some(other_indices)) = (self.indices_mut(), other.indices()) {
match (indices, other_indices) { indices.extend(other_indices.iter().map(|i| (i + index_offset) as u32));
(Indices::U16(i1), Indices::U16(i2)) => {
i1.extend(i2.iter().map(|i| *i + index_offset as u16));
}
(Indices::U32(i1), Indices::U32(i2)) => {
i1.extend(i2.iter().map(|i| *i + index_offset as u32));
}
(Indices::U16(i1), Indices::U32(i2)) => {
i1.extend(i2.iter().map(|i| *i as u16 + index_offset as u16));
}
(Indices::U32(i1), Indices::U16(i2)) => {
i1.extend(i2.iter().map(|i| *i as u32 + index_offset as u32));
}
}
} }
} }