Skip to content
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

Soundness issue: Undefined Behavior due to writing to immutable pointer #36

Closed
RalfJung opened this issue Nov 20, 2023 · 6 comments
Closed

Comments

@RalfJung
Copy link

RalfJung commented Nov 20, 2023

I stumbled upon this code:

bnum/src/bint/endian.rs

Lines 85 to 92 in 6e26ddb

let ptr = uninit.as_ptr().cast_mut() as *mut u8;
let digit_bytes = unsafe {
slice_ptr
.add(
len - digit::$Digit::BYTES as usize
- (i << digit::$Digit::BYTE_SHIFT),
)
.copy_to_nonoverlapping(ptr, digit::$Digit::BYTES as usize);

That code has UB: uninit.as_ptr() is a read-only pointer (it derives from a shared reference), and so it must not be written to. Just casting from const to mut does not help, you must use a proper mutable pointer here.

copy_to_nonoverlapping got stabilized accidentally; there isn't actually a way to use it in const on stable in a UB-free way.

@isaacholt100
Copy link
Owner

isaacholt100 commented Nov 23, 2023

Hi @RalfJung, thank you very much for alerting me to this, I guess I'll have to make these non-const for the time being, or hopefully I can find a way of keeping it const without UB that isn't much slower.

@RalfJung
Copy link
Author

Thanks a lot!
Is there any chance of getting a semver-compatible update for bnum 0.8 that does the same thing? According to our crater results, that version occurs most often as dependency, and it would be great if people just had to do cargo update instead of having to wait for the version bump to propagate through the dependency tree.

@isaacholt100
Copy link
Owner

No worries! Sure, can do, although the only major changes from 0.8 to 0.10 are the change from #[repr(Rust)] to #[repr(transparent)] of some structs and the fixing of a small number of incorrect implementations of methods, so users upgrading to 0.8 to 0.10 shouldn't have to change any existing code. If you still think this is a good idea though, I can certainly create a compatible update for v0.8: should this version be published as v0.8.1?

@RalfJung
Copy link
Author

It's hard for me to say, I don't know the bnum ecosystem. The thing is just, the update from 0.8 to 0.10 has to be done by the author of the crate(s) that depend on 0.8 (and then the author of the next crate that depends on that, and so on). In contrast, an update to 0.8.1 can be done by anyone by just doing cargo update.

@isaacholt100
Copy link
Owner

Thanks for explaining, I have now published v0.8.1 with this fix.

@RalfJung
Copy link
Author

Thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants