-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix integer overflow in BlobVec::push for ZST #10799
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how this resolves the bug mentioned in the PR description.
The Rust standard library also has an unchecked addition in Vec::push. The corresponding grow method for RawVec also makes explicit checks to see if it's handling a ZST, which this PR is changing to be an unconditional assertion instead of a soft check.
The problem in the PR description is valid. We should not allow BlobVec's len to overflow, but I don't think the changes here are appropriate for addressing it.
Attempt to call
Because
This "soft check" is converted to panic in
I'm sorry, I don't understand.
Correct. We should panic when attempting to add more than
The interface of
Is it possible you misunderstood this PR? Otherwise, what do you suggest? |
f8a7724
to
e009be0
Compare
Added a test. |
e009be0
to
1541b7f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for clarifying how you were approaching this. I think I see what you were going for here. This also removes yet another low level unsafe function from bevy_ecs, so it generally seems like a win to me. Only nit is that this is reliant on the current BlobVec initialization for ZSTs and that implicit reliance is not documented.
d3e6769
to
b2a642b
Compare
Added more comments. |
b2a642b
to
3b30cec
Compare
Rephrased comments. |
reserve_exact
is no-op for ZST becauseself.item_layout.size() > 0
is alwaysfalse
.bevy/crates/bevy_ecs/src/storage/blob_vec.rs
Lines 112 to 120 in daa8bf2
Then in
push
we just increase.len
ignoring integer overflow.bevy/crates/bevy_ecs/src/storage/blob_vec.rs
Lines 232 to 237 in daa8bf2