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

TODO in code: replace copy_nonoverlapping() with read_unaligned() #35

Closed
Shnatsel opened this issue Jul 9, 2018 · 5 comments
Closed

Comments

@Shnatsel
Copy link
Contributor

Shnatsel commented Jul 9, 2018

There is a TODO in the code: "replace this with read_unaligned() as it stabilizes"

https://github.com/snapview/tungstenite-rs/blob/3abe419/src/protocol/frame/mask.rs#L31

read_unaligned() has been stabilized in Rust 1.17

Also of interest, to_bytes()/from_bytes() which seems to be the safe equivalent of what the unsafe code is doing here, is going to be stabilized in Rust 1.29, which is the next release.

@agalakhov
Copy link
Member

Thank you!

@bluetech
Copy link
Contributor

Just passing by, but I wonder why is read_unaligned needed? The &mask reference should be sufficiently aligned, according to the reference at least: https://doc.rust-lang.org/reference/type-layout.html

@agalakhov
Copy link
Member

agalakhov commented Jul 10, 2018

The &mask has an alignment of just u8, and the one of u32 is needed. It's about its contents, not the reference itself.

@bluetech
Copy link
Contributor

Yes, you are correct, I misread:

An array of [T; n] has a size of size_of::<T>() * n and the same alignment of T.

Maybe it's better to just read a u32 off the wire and work with that, like actix does.

@agalakhov
Copy link
Member

Working with u32 requires endianess-dependent code. The behaviour is defined by RFC for u8, not for u32. On an unusual CPU or in a critical environment one could disable unsafe optimizations and stick to straightforward apply_mask_fallback which is definitely correct.

psmit pushed a commit to psmit/tungstenite-rs that referenced this issue Nov 14, 2019
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