search

FyroxEngine / Fyrox

3D and 2D game engine written in Rust
https://fyrox.rs
MIT License
7.51k stars 341 forks source link

Fixing Potential Double Free Issue #476

Closed kuzeyardabulut closed 11 months ago

kuzeyardabulut commented 11 months ago

Hi, I found a memory-safety/soundness issue in this crate while scanning Rust code for potential vulnerabilities. This PR contains a fix for the issue.

Issue Description

https://github.com/FyroxEngine/Fyrox/blob/af36a53e01d3b75c203bfc33daee6f103ed0dc49/src/scene/mesh/buffer.rs#L515-L521 If a panic happens within Vec::<u8>::from_raw_parts(), the std::mem::forget(data) will create a double free vulnerability.

In Rust, std::mem::forget does not actually free the memory, instead it simply allows the memory to leak. This can lead to double free when the data object goes out of scope and its destructor is called automatically. The issue arises because, while std::mem::forget(data) prevents the destructor of data from being called, the Vec created from the raw parts still has its own destructor. When the Vec is dropped, it will free the memory, and then when data is dropped, it will attempt to free the same memory again, causing a double free.

The modification here uses std::mem::ManuallyDrop to wrap data. This ensures that data will not be automatically dropped when it goes out of scope, thus avoiding a double free scenario. With ManuallyDrop, we explicitly state that the data variable should not be dropped, thus avoiding any potential double free issues.

Shnatsel commented 11 months ago

Would it be possible to publish a point release with the fix?

There is an open PR for a security advisory for this vulnerability: https://github.com/rustsec/advisory-db/pull/1736 I'd prefer to have a fixed version for users to upgrade to, as opposed to a non-actionable advisory with no fixed versions available.

Shnatsel commented 11 months ago

I understand that this is not an issue in practice because there is no way for Vec::from_raw_parts to panic, as pointed out by @adamreichold and confirmed by @Nilstrieb, so no need for a point release with the fix. Thank you for reviewing and merging this quickly nevertheless.