feat(io): fix ancillary API to avoid UB#737
feat(io): fix ancillary API to avoid UB#737fantix wants to merge 11 commits intocompio-rs:masterfrom
Conversation
|
We need to be cautious to allocations here. The previous Somehow I would like to avoid the box. Maybe make the |
|
Good point on allocation! This change was partially to eliminate the now-unnecessary |
AncillaryBuf::build() API
There was a problem hiding this comment.
Pull request overview
Adds a new AncillaryBuf::builder()-style construction API for ancillary/control messages in compio-io, updates downstream usage (QUIC socket + tests), and preserves the deprecated compio-net::CMsgBuilder API via a compatibility type.
Changes:
- Refactors
AncillaryBuilderto build into anAncillaryBuf<N>and addsAncillaryBuf::builder(). - Introduces a deprecated-compat
CMsgBuilderand updatescompio-net’s deprecated alias to point to it. - Adds docs/example + a new
ancillary_space<T>()helper intended for const-generic sizing; adjusts tests and Cargo test gating.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| compio-quic/src/socket.rs | Switches send-side control-message construction to AncillaryBuf::builder() and removes manual finish/set_len. |
| compio-net/src/lib.rs | Updates deprecated CMsgBuilder alias and deprecation note to reflect new API path. |
| compio-io/tests/ancillary.rs | Migrates tests to AncillaryBuf-based builder and uses compat CMsgBuilder for alignment panic test. |
| compio-io/src/ancillary/windows.rs | Makes wsa_cmsg_space pub(crate) for reuse. |
| compio-io/src/ancillary/mod.rs | Implements new builder API, adds compat CMsgBuilder, adds docs/example, and adds ancillary_space<T>(). |
| compio-io/Cargo.toml | Removes aligned-array dev-dep and gates the ancillary integration test behind the ancillary feature. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
AncillaryBuf::build() API|
@AsakuraMizu I would love to have your review on this change too! |
Berrysoft
left a comment
There was a problem hiding this comment.
I think it LGTM currently, but would like @George-Miao to review.
| builder.try_push(libc::IPPROTO_IP, libc::IP_TOS, ecn as libc::c_int); | ||
| builder | ||
| .push(libc::IPPROTO_IP, libc::IP_TOS, &(ecn as libc::c_int)) | ||
| .expect("cmsg push"); |
There was a problem hiding this comment.
We should be tolerant of push failures.
There was a problem hiding this comment.
Ahh, good catch! This is a change-of-behavior indeed.
push() now fails when:
- Insufficient capacity in the buffer
encode()fails with custom error (not currently used)
while try_push() was returning None when:
- Insufficient capacity in the buffer
- Data type is not aligned to cmsg (no longer used)
So, changing expect("cmsg push") to ok() would preserve the old behavior.
However, given that the buffer size was set to a fixed number of 128 bytes, my question is like, is 128 guaranteed to contain the worst-case CMSG_SPACE() sum of all three:
- ECN
- pktinfo / destination address
- GSO
If yes, I think we should keep the panic; otherwise, it would be ideal to compute a proper total size to replace the magic number.
... unless, it was by design that, if any of the 3 control messages go beyond 128 bytes, we silently ignore the issue, skip the rest, and send with whatever cmsg we can?
This PR was initally created to enhance the
AncillaryBuilderAPI to be like:This is not a breaking change because:
AncillaryBuilderis never releasedCMsgBuilderpreserves old deprecated behaviorUPDATE
During review, @George-Miao spotted an API design defect that allows users to trigger undefined behaviors sending/receiving control messages using structs that have paddings between fields.
This PR is then repurposed to fix this issue in a way that:
AncillaryDatathat is semantically safe to encode/decode any types of value to/from control message data payload;bytemuckdependency to automatically implementAncillaryDatafor basic types;compio-ionow implementsAncillaryDatafor networking types required bycompio-quic.This PR also changed the interface to copy data during encoding/decoding, with a side-effect of no longer requiring data alignment.
Now it looks like:
And
AncillaryRef.data()returnsResult<T, CodecError>instead of&T.Refs #730 #734