fix(udp): use unaligned cmsg payload access for musl - #775
Conversation
Yes, clearly we need to add this! Would be appreciated if you could do that here as well. |
|
I'm afraid that an unaligned write is probably just another runtime panic in disguise. It furthers the current approach of hoping that the alignment we use is correct. Which was always a bit optimistic. I think the correct approach it to actually allocate this on the right alignment. I've once written this up in https://devork.be/blog/2023/11/modern-linux-sockets/ which may finally become a useful reference. I'm not sure we can avoid the explicit allocation call, ideally we'd be able to amortise it so we only need to do it once. But I'm not immediately sure how to do that yet, we may have to store this allocation somewhere. Maybe the UdpSocketState but maybe there are better places. As for the size of the allocation, the current code makes it a bit hard to figure out all the options that we might want to push. Maybe we can draw a list of all the ones we might use per-platform. But that seems like we'd end up with having to keep two places, the allocation and the filling, of cmsgs manually in sync which is brittle. Maybe there are some cleverer tricks we can do with the encoder itself to have a separate method to push each message and that way we can manage to fix this at compile time as well? It could be worth trying to figure this out rather than go back to relying on the |
Description
Fixes #774.
Drops the two alignment asserts in
cmsg/mod.rsand uses unaligned access:decode()ptr::read→ptr::read_unaligned. This is the reachable bug.Encoder::push()ptr::write→ptr::write_unaligned, same assert removed.On x86-64 and aarch64 these lower to the same instructions when the address is aligned, so there is no cost.
Verified on
aarch64-unknown-linux-musl.Before:
0 passed / 9 failedAfter:
9 passed / 0 failed.Breaking Changes
None.
Notes & open questions
Encoder::pushis latent, not reachable today - every pushed type is align ≤ 4, so its assert never fires. Fixed for symmetry. Happy to drop it if you would rather keep the diff to the live bug.Aligned<T>actually enforces, rather thanalign_of::<C>(). I went withread_unalignedsince it stops depending on that invariant, but I am happy to switch.cargo checkjob would not catch this, because it is a runtime panic. It needscargo test --target x86_64-unknown-linux-musl, which runs natively on the Ubuntu runners withmusl-toolsinstalled. Glad to add it here or as a follow-up.Change checklist
cargo makepasses locally.