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

Reading methods are not sound w.r.t alignment #47

Closed
bluss opened this issue May 2, 2016 · 7 comments
Closed

Reading methods are not sound w.r.t alignment #47

bluss opened this issue May 2, 2016 · 7 comments

Comments

@bluss
Copy link

bluss commented May 2, 2016

The byteorder crate is casting a u8 pointer to for example *const u64 and reading it. This requires that the pointer is well aligned for the new element type, otherwise this is UB.

@BurntSushi
Copy link
Owner

Could you explain a bit more about the UB? (I'm not familiar with this particular case.)

I take it that asserting the length of a &[u8] is sufficiently long is not enough?

@bluss
Copy link
Author

bluss commented May 2, 2016

Using * on a raw pointer is reading the value through the pointer.

The pointer must be well aligned for the element type of the memory access.

The typical way to do a possibly unaligned access in a well defined way is like this

https://github.com/rust-lang/rust/blob/9cc430d2cf90a38880e02bb319a5563cbe07a40c/src/libcore/hash/sip.rs#L102-L113

disclaimer: I wrote that code. But that's exactly what for example google's farmhash does in C to do a possibly unaligned load. On a platform where unaligned loads are unproblematic, the compiler generates a regular load. I don't know its performance on other platforms than x86 though, there could be better ways to do it there possibly, but this approach is at least sound.

@bluss
Copy link
Author

bluss commented May 2, 2016

If you deref a *const u64, rustc generates a load that assumes that pointer is well aligned. When it is not, that results in the emitted code overestimating the alignment.

I would not even cast a *const u8 that is not well aligned, to another pointer type.

@BurntSushi
Copy link
Owner

BurntSushi commented May 2, 2016

I see, thanks for explaining and pointing to a solution. :-) That makes sense. So switching to copy_overlapping for everything sounds like the answer here.

@DoumanAsh
Copy link

DoumanAsh commented May 5, 2016

This issue appeared because of me occasionally looking into byteorder code and writing my own vehicle for byte operations...
With some help i made simple generic conversions from\to bytes and tried to make it as safe as possible via traits:
In case if you'll find it interesting you can take a look at my repo:
https://github.com/DoumanAsh/lazy-bytes-cast

@BurntSushi
Copy link
Owner

I found this page to contain the best explanation I've seen so far: https://www.securecoding.cert.org/confluence/display/c/EXP36-C.+Do+not+cast+pointers+into+more+strictly+aligned+pointer+types

BurntSushi added a commit that referenced this issue May 21, 2016
Instead of casting pointers, we do a proper unaligned load using
copy_nonoverlapping.

Benchmarks appear unaffected on Linux x64.
BurntSushi added a commit that referenced this issue May 22, 2016
Fixes undefined behavior reported in #47.
@BurntSushi
Copy link
Owner

Fixed in #48.

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

No branches or pull requests

3 participants