Skip to content

fix(udp): use unaligned cmsg payload access for musl - #775

Open
ypopivniak wants to merge 1 commit into
n0-computer:mainfrom
ypopivniak:fix-musl-cmsg-alignment
Open

fix(udp): use unaligned cmsg payload access for musl#775
ypopivniak wants to merge 1 commit into
n0-computer:mainfrom
ypopivniak:fix-musl-cmsg-alignment

Conversation

@ypopivniak

Copy link
Copy Markdown

Description

Fixes #774.

Drops the two alignment asserts in cmsg/mod.rs and uses unaligned access:

  • decode() ptr::readptr::read_unaligned. This is the reachable bug.
  • Encoder::push() ptr::writeptr::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 failed
After: 9 passed / 0 failed.

Breaking Changes

None.

Notes & open questions

  • Encoder::push is 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.
  • Alternative fix: keep an assert but compare against the 8-byte alignment Aligned<T> actually enforces, rather than align_of::<C>(). I went with read_unaligned since it stops depending on that invariant, but I am happy to switch.
  • Should CI gain a musl target? Note a cargo check job would not catch this, because it is a runtime panic. It needs cargo test --target x86_64-unknown-linux-musl, which runs natively on the Ubuntu runners with musl-tools installed. Glad to add it here or as a follow-up.

Change checklist

  • Self-review.
  • Documentation updates following the style guide, if relevant.
  • Tests if relevant.
  • All breaking changes documented.
  • This PR was created by a human that thought critically about the proposed change and wrote an as clear and concise description as they could.
  • This PR isn't slop, and is carefully crafted to do have the intented effect.
  • cargo make passes locally.

@n0bot n0bot Bot added this to iroh Jul 26, 2026
@github-project-automation github-project-automation Bot moved this to 🚑 Needs Triage in iroh Jul 26, 2026
@flub

flub commented Jul 27, 2026

Copy link
Copy Markdown
Collaborator

Should CI gain a musl target? Note a cargo check job would not catch this, because it is a runtime panic. It needs cargo test --target x86_64-unknown-linux-musl, which runs natively on the Ubuntu runners with musl-tools installed. Glad to add it here or as a follow-up.

Yes, clearly we need to add this! Would be appreciated if you could do that here as well.

@flub

flub commented Jul 27, 2026

Copy link
Copy Markdown
Collaborator

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 cmsg::LEN constant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: 🚑 Needs Triage

Development

Successfully merging this pull request may close these issues.

noq-udp panics on musl targets: assertion failed: align_of::<T>() <= align_of::<C>()

2 participants