fix(udp/fast-apple): ignore empty cmsghdr #2154
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mxinden
commented
Feb 12, 2025
|
For the record, here is a Mozilla CI run, testing this patch on MacOS 10.15: https://treeherder.mozilla.org/jobs?repo=try&revision=cc4cbf5f7bd0a48dcab741bb09c0c99cc94fa6da |
|
mxinden
changed the title
larseggert
reviewed
Feb 12, 2025
On MacOS < 14, with `fast-apple-datapath` feature, calls to
`libc::CMSG_NXTHDR` might continuously return empty (i.e. all zero)
`libc::cmsghdr` instead of a null pointer. This results in a busy loop
in `decode_recv`:
``` rust
let cmsg_iter = unsafe { cmsg::Iter::new(hdr) };
for cmsg in cmsg_iter {
match (cmsg.cmsg_level, cmsg.cmsg_type) {
```
https://github.com/quinn-rs/quinn/blob/b4378bb39dab4b58a1e6a3fea4fff9f87033dab6/quinn-udp/src/unix.rs#L685C1-L687C50
This commit fixes the above, returning a `null_mut()` pointer on an
empty `libc::cmsgdhr`, thus terminating the `cmsg_iter`.
See also mozilla/neqo#2427 for details.
All green 🎉 With quinn-udp unit tests succeeding and and Mozilla CI succeeding, this pull request is ready for review. |
djc
approved these changes
Feb 12, 2025
Sounds good to me! |
Ralith
approved these changes
Feb 13, 2025
There was a problem hiding this comment.
What is the likelihood that other BSDs have a similar bug? Should we perhaps guard against this in the Iterator implementation?
quinn/quinn-udp/src/cmsg/mod.rs
Lines 113 to 115 in 9bcd53d
Thus I assume this bug is strictly related to how Would you agree @thomaseizinger? |
Makes sense to me! |
|
@mxinden I have invited you as a crates.io owner for quinn-udp. Want to take care of merging/publishing? |
|
That is great news. Thank you @djc. I will merge, tag and publish now. |
thomaseizinger
added a commit
to thomaseizinger/quinn
that referenced
this pull request
Apr 15, 2025
In quinn-rs#2154, we added a piece of logic to `cmsg_nxt_hdr` that - within the `apple-fast-datapath` code inspects the `cmsg_len` property and checks if we can actually fit another `cmsghdr` into it. This however only makes sense when we are _reading_ a buffer of cmsgs that has been initialised by the kernel. When we are _sending_ datagrams, the cmsg buffer is initialised with all zeros and for every cmsg we want to attach to a datagram, we advance the buffer using CMSG_NXTHDR. In this case, all the fields within `cmsghdr` are 0 initialised and it doesn't make sense to check their length. In fact, doing so results in a false-positive panic triggered by `Encoder::push` because it thinks that the cmsg buffer is too short to accomodate the message it wants to write.
thomaseizinger
added a commit
to thomaseizinger/quinn
that referenced
this pull request
Apr 15, 2025
In quinn-rs#2154, we added a piece of logic to `cmsg_nxt_hdr` that - within the `apple-fast-datapath` code inspects the `cmsg_len` property and checks if we can actually fit another `cmsghdr` into it. This however only makes sense when we are _reading_ a buffer of cmsgs that has been initialised by the kernel. When we are _sending_ datagrams, the cmsg buffer is initialised with all zeros and for every cmsg we want to attach to a datagram, we advance the buffer using CMSG_NXTHDR. In this case, all the fields within `cmsghdr` are 0 initialised and it doesn't make sense to check their length. In fact, doing so results in a false-positive panic triggered by `Encoder::push` because it thinks that the cmsg buffer is too short to accomodate the message it wants to write. To fix this issue, we split the `cmsg_nxt_hdr` API into two: - One for decoding the next cmsg - One for simply advancing it when we are in the process of writing `cmsghdr`s.
thomaseizinger
added a commit
to thomaseizinger/quinn
that referenced
this pull request
Apr 15, 2025
In quinn-rs#2154, we added a piece of logic to `cmsg_nxt_hdr` that - within the `apple-fast-datapath` code - inspects the `cmsg_len` property and checks if we can actually fit another `cmsghdr` into it. This however only makes sense when we are _reading_ a buffer of cmsgs that has been initialised by the kernel. When we are _sending_ datagrams, the cmsg buffer is initialised with all zeros and for every cmsg we want to attach to a datagram, we advance the buffer using CMSG_NXTHDR. In this case, all the fields within `cmsghdr` are 0 initialised and it doesn't make sense to check their length. In fact, doing so results in a false-positive panic triggered by `Encoder::push` because it thinks that the cmsg buffer is too short to accomodate the message it wants to write. To fix this issue, we split the `cmsg_nxt_hdr` API into two: - One for decoding the next cmsg - One for simply advancing it when we are in the process of writing `cmsghdr`s.
thomaseizinger
added a commit
to thomaseizinger/quinn
that referenced
this pull request
Apr 15, 2025
In quinn-rs#2154, we added a piece of logic to `cmsg_nxt_hdr` that - within the `apple-fast-datapath` code inspects the `cmsg_len` property and checks if we can actually fit another `cmsghdr` into it. This however only makes sense when we are _reading_ a buffer of cmsgs that has been initialised by the kernel. When we are _sending_ datagrams, the cmsg buffer is initialised with all zeros and for every cmsg we want to attach to a datagram, we advance the buffer using CMSG_NXTHDR. In this case, all the fields within `cmsghdr` are 0 initialised and it doesn't make sense to check their length. In fact, doing so results in a false-positive panic triggered by `Encoder::push` because it thinks that the cmsg buffer is too short to accomodate the message it wants to write. To fix this issue, we move the check to the `Iterator` implementation, guarded by the `apple_fast` cfg. This allows us to use the `cmsg_nxt_hdr` function for both reading and writing cmsgs.
thomaseizinger
added a commit
to thomaseizinger/quinn
that referenced
this pull request
Apr 15, 2025
In quinn-rs#2154, we added a piece of logic to `cmsg_nxt_hdr` that - within the `apple-fast-datapath` code - inspects the `cmsg_len` property and checks if we can actually fit another `cmsghdr` into it. This however only makes sense when we are _reading_ a buffer of cmsgs that has been initialised by the kernel. When we are _sending_ datagrams, the cmsg buffer is initialised with all zeros and for every cmsg we want to attach to a datagram, we advance the buffer using CMSG_NXTHDR. In this case, all the fields within `cmsghdr` are 0 initialised and it doesn't make sense to check their length. In fact, doing so results in a false-positive panic triggered by `Encoder::push` because it thinks that the cmsg buffer is too short to accomodate the message it wants to write. To fix this issue, we split the `cmsg_nxt_hdr` API into two: - One for decoding the next cmsg - One for simply advancing it when we are in the process of writing `cmsghdr`s.
thomaseizinger
added a commit
to thomaseizinger/quinn
that referenced
this pull request
Apr 15, 2025
In quinn-rs#2154, we added a piece of logic to `cmsg_nxt_hdr` that - within the `apple-fast-datapath` code inspects the `cmsg_len` property and checks if we can actually fit another `cmsghdr` into it. This however only makes sense when we are _reading_ a buffer of cmsgs that has been initialised by the kernel. When we are _sending_ datagrams, the cmsg buffer is initialised with all zeros and for every cmsg we want to attach to a datagram, we advance the buffer using CMSG_NXTHDR. In this case, all the fields within `cmsghdr` are 0 initialised and it doesn't make sense to check their length. In fact, doing so results in a false-positive panic triggered by `Encoder::push` because it thinks that the cmsg buffer is too short to accomodate the message it wants to write. To fix this issue, we move the check to the `Iterator` implementation, guarded by the `apple_fast` cfg. This allows us to use the `cmsg_nxt_hdr` function for both reading and writing cmsgs.
thomaseizinger
added a commit
to thomaseizinger/quinn
that referenced
this pull request
Apr 15, 2025
In quinn-rs#2154, we added a piece of logic to `cmsg_nxt_hdr` that - within the `apple-fast-datapath` code inspects the `cmsg_len` property and checks if we can actually fit another `cmsghdr` into it. This however only makes sense when we are _reading_ a buffer of cmsgs that has been initialised by the kernel. When we are _sending_ datagrams, the cmsg buffer is initialised with all zeros and for every cmsg we want to attach to a datagram, we advance the buffer using CMSG_NXTHDR. In this case, all the fields within `cmsghdr` are 0 initialised and it doesn't make sense to check their length. In fact, doing so results in a false-positive panic triggered by `Encoder::push` because it thinks that the cmsg buffer is too short to accomodate the message it wants to write. To fix this issue, we move the check to the `Iterator` implementation, guarded by the `apple_fast` cfg. This allows us to use the `cmsg_nxt_hdr` function for both reading and writing cmsgs.
thomaseizinger
added a commit
to thomaseizinger/quinn
that referenced
this pull request
Apr 15, 2025
In quinn-rs#2154, we added a piece of logic to `cmsg_nxt_hdr` that - within the `apple-fast-datapath` code inspects the `cmsg_len` property and checks if we can actually fit another `cmsghdr` into it. This however only makes sense when we are _reading_ a buffer of cmsgs that has been initialised by the kernel. When we are _sending_ datagrams, the cmsg buffer is initialised with all zeros and for every cmsg we want to attach to a datagram, we advance the buffer using CMSG_NXTHDR. In this case, all the fields within `cmsghdr` are 0 initialised and it doesn't make sense to check their length. In fact, doing so results in a false-positive panic triggered by `Encoder::push` because it thinks that the cmsg buffer is too short to accomodate the message it wants to write. To fix this issue, we move the check to the `Iterator` implementation, guarded by the `apple_fast` cfg. This allows us to use the `cmsg_nxt_hdr` function for both reading and writing cmsgs.
thomaseizinger
added a commit
to thomaseizinger/quinn
that referenced
this pull request
Apr 15, 2025
In quinn-rs#2154, we added a piece of logic to `cmsg_nxt_hdr` that - within the `apple-fast-datapath` code inspects the `cmsg_len` property and checks if we can actually fit another `cmsghdr` into it. This however only makes sense when we are _reading_ a buffer of cmsgs that has been initialised by the kernel. When we are _sending_ datagrams, the cmsg buffer is initialised with all zeros and for every cmsg we want to attach to a datagram, we advance the buffer using CMSG_NXTHDR. In this case, all the fields within `cmsghdr` are 0 initialised and it doesn't make sense to check their length. In fact, doing so results in a false-positive panic triggered by `Encoder::push` because it thinks that the cmsg buffer is too short to accomodate the message it wants to write. To fix this issue, we move the check to the `Iterator` implementation, guarded by the `apple_fast` cfg. This allows us to use the `cmsg_nxt_hdr` function for both reading and writing cmsgs.
thomaseizinger
added a commit
to thomaseizinger/quinn
that referenced
this pull request
Apr 15, 2025
In quinn-rs#2154, we added a piece of logic to `cmsg_nxt_hdr` that - within the `apple-fast-datapath` code - inspects the `cmsg_len` property and checks if we can actually fit another `cmsghdr` into it. This however only makes sense when we are _reading_ a buffer of cmsgs that has been initialised by the kernel. When we are _sending_ datagrams, the cmsg buffer is initialised with all zeros and for every cmsg we want to attach to a datagram, we advance the buffer using CMSG_NXTHDR. In this case, all the fields within `cmsghdr` are 0 initialised and it doesn't make sense to check their length. In fact, doing so results in a false-positive panic triggered by `Encoder::push` because it thinks that the cmsg buffer is too short to accomodate the message it wants to write. To fix this issue, we move the check to the `Iterator` implementation, guarded by the `apple_fast` cfg. This allows us to use the `cmsg_nxt_hdr` function for both reading and writing cmsgs.
thomaseizinger
added a commit
to thomaseizinger/quinn
that referenced
this pull request
Apr 16, 2025
In quinn-rs#2154, we added a piece of logic to `cmsg_nxt_hdr` that - within the `apple-fast-datapath` code - inspects the `cmsg_len` property and checks if we can actually fit another `cmsghdr` into it. This however only makes sense when we are _reading_ a buffer of cmsgs that has been initialised by the kernel. When we are _sending_ datagrams, the cmsg buffer is initialised with all zeros and for every cmsg we want to attach to a datagram, we advance the buffer using CMSG_NXTHDR. In this case, all the fields within `cmsghdr` are 0 initialised and it doesn't make sense to check their length. In fact, doing so results in a false-positive panic triggered by `Encoder::push` because it thinks that the cmsg buffer is too short to accomodate the message it wants to write. To fix this issue, we move the check to the `Iterator` implementation, guarded by the `apple_fast` cfg. This allows us to use the `cmsg_nxt_hdr` function for both reading and writing cmsgs.
thomaseizinger
added a commit
to thomaseizinger/quinn
that referenced
this pull request
Apr 16, 2025
In quinn-rs#2154, we added a piece of logic to `cmsg_nxt_hdr` that - within the `apple-fast-datapath` code - inspects the `cmsg_len` property and checks if we can actually fit another `cmsghdr` into it. This however only makes sense when we are _reading_ a buffer of cmsgs that has been initialised by the kernel. When we are _sending_ datagrams, the cmsg buffer is initialised with all zeros and for every cmsg we want to attach to a datagram, we advance the buffer using CMSG_NXTHDR. In this case, all the fields within `cmsghdr` are 0 initialised and it doesn't make sense to check their length. In fact, doing so results in a false-positive panic triggered by `Encoder::push` because it thinks that the cmsg buffer is too short to accomodate the message it wants to write. To fix this issue, we move the check to the `Iterator` implementation, guarded by the `apple_fast` cfg. This allows us to use the `cmsg_nxt_hdr` function for both reading and writing cmsgs.
thomaseizinger
added a commit
to thomaseizinger/quinn
that referenced
this pull request
Apr 16, 2025
In quinn-rs#2154, we added a piece of logic to `cmsg_nxt_hdr` that - within the `apple-fast-datapath` code - inspects the `cmsg_len` property and checks if we can actually fit another `cmsghdr` into it. This however only makes sense when we are _reading_ a buffer of cmsgs that has been initialised by the kernel. When we are _sending_ datagrams, the cmsg buffer is initialised with all zeros and for every cmsg we want to attach to a datagram, we advance the buffer using CMSG_NXTHDR. In this case, all the fields within `cmsghdr` are 0 initialised and it doesn't make sense to check their length. In fact, doing so results in a false-positive panic triggered by `Encoder::push` because it thinks that the cmsg buffer is too short to accomodate the message it wants to write. To fix this issue, we move the check to the `Iterator` implementation, guarded by the `apple_fast` cfg. This allows us to use the `cmsg_nxt_hdr` function for both reading and writing cmsgs.
github-merge-queue bot
pushed a commit
that referenced
this pull request
Apr 18, 2025
In #2154, we added a piece of logic to `cmsg_nxt_hdr` that - within the `apple-fast-datapath` code - inspects the `cmsg_len` property and checks if we can actually fit another `cmsghdr` into it. This however only makes sense when we are _reading_ a buffer of cmsgs that has been initialised by the kernel. When we are _sending_ datagrams, the cmsg buffer is initialised with all zeros and for every cmsg we want to attach to a datagram, we advance the buffer using CMSG_NXTHDR. In this case, all the fields within `cmsghdr` are 0 initialised and it doesn't make sense to check their length. In fact, doing so results in a false-positive panic triggered by `Encoder::push` because it thinks that the cmsg buffer is too short to accomodate the message it wants to write. To fix this issue, we move the check to the `Iterator` implementation, guarded by the `apple_fast` cfg. This allows us to use the `cmsg_nxt_hdr` function for both reading and writing cmsgs.
mxinden
added a commit
to mxinden/neqo
that referenced
this pull request
May 16, 2025
The `quinn-udp` `fast-apple-datapath` Cargo feature allows Neqo to use the private MacOS multi-message IO functions `sendmsg_x` and `recvmsg_x`. We have seen issues with `fast-apple-datapath`: - quinn-rs/quinn#2214 - quinn-rs/quinn#2154 To reduce the risk of the upcoming larger roll out of the Fast UDP for Firefox project to Firefox Release, disable `fast-apple-datapath` for now. Enabling it can then be a separate project afterwards.
mxinden
added a commit
to mxinden/neqo
that referenced
this pull request
May 16, 2025
The `quinn-udp` `fast-apple-datapath` Cargo feature allows Neqo to use the private MacOS multi-message IO functions `sendmsg_x` and `recvmsg_x`. We have seen issues with `fast-apple-datapath`: - quinn-rs/quinn#2214 - quinn-rs/quinn#2154 To reduce the risk of the upcoming larger roll out of the Fast UDP for Firefox project to Firefox Release, disable `fast-apple-datapath` for now. Enabling it can then be a separate project afterwards.
github-merge-queue bot
pushed a commit
to mozilla/neqo
that referenced
this pull request
May 16, 2025
The `quinn-udp` `fast-apple-datapath` Cargo feature allows Neqo to use the private MacOS multi-message IO functions `sendmsg_x` and `recvmsg_x`. We have seen issues with `fast-apple-datapath`: - quinn-rs/quinn#2214 - quinn-rs/quinn#2154 To reduce the risk of the upcoming larger roll out of the Fast UDP for Firefox project to Firefox Release, disable `fast-apple-datapath` for now. Enabling it can then be a separate project afterwards.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
On MacOS < 14, with
fast-apple-datapathfeature, calls tolibc::CMSG_NXTHDRmight continuously return empty (i.e. all zero)libc::cmsghdrinstead of a null pointer. This results in a busy loop indecode_recv:https://github.com/quinn-rs/quinn/blob/b4378bb39dab4b58a1e6a3fea4fff9f87033dab6/quinn-udp/src/unix.rs#L685C1-L687C50
This commit fixes the above, returning a
null_mut()pointer on an emptylibc::cmsgdhr, thus terminating thecmsg_iter.See also mozilla/neqo#2427 for details.