Clamp MinAckDelay to MaxAckDelay in local transport params - #6213
Merged
Gaurav Singh (gaurav2699) merged 1 commit intoAug 12, 2026
Conversation
Gaurav Singh (gaurav2699)
force-pushed
the
user/gaurasingh/fix-minackdelay-clamp
branch
from
August 10, 2026 19:53
e778c1d to
a1f3ea4
Compare
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (50.00%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #6213 +/- ##
==========================================
- Coverage 86.03% 85.58% -0.45%
==========================================
Files 60 60
Lines 18981 18986 +5
==========================================
- Hits 16330 16249 -81
- Misses 2651 2737 +86 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Gaurav Singh (gaurav2699)
force-pushed
the
user/gaurasingh/fix-minackdelay-clamp
branch
2 times, most recently
from
August 10, 2026 21:02
7bcade1 to
e76801d
Compare
Gaurav Singh (gaurav2699)
force-pushed
the
user/gaurasingh/fix-minackdelay-clamp
branch
from
August 11, 2026 10:58
e76801d to
cd3a490
Compare
Guillaume Hetier (guhetier)
previously approved these changes
Aug 11, 2026
Guillaume Hetier (guhetier)
left a comment
Guillaume Hetier (guhetier)
left a comment
Collaborator
There was a problem hiding this comment.
Consider a helper macro for this (US_TO_MS_CEIL or something similiar)
Gaurav Singh (gaurav2699)
force-pushed
the
user/gaurasingh/fix-minackdelay-clamp
branch
2 times, most recently
from
August 12, 2026 13:48
94de5e4 to
599ca3d
Compare
…arams When MaxAckDelayMs is 0 (delayed ACKs disabled), the connection's derived MaxAckDelay is 0 while MinAckDelay stays at the timer resolution, so we would advertise min_ack_delay > max_ack_delay. That trips a CXPLAT_DBG_ASSERT in QuicCryptoTlsEncodeTransportParameters in Debug and produces an invalid transport parameter (rejected by peers, including MsQuic) in Release. MinAckDelay is a hard floor (we cannot ACK faster than the timer resolution), so instead of lowering it, raise only the advertised MaxAckDelay TP to that floor. The stored MaxAckDelayMs setting is left unchanged, so the ack tracker still treats 0 as 'ack immediately'. Surfaced by SpinQuic settings coverage. Fixes #6212 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Gaurav Singh (gaurav2699)
force-pushed
the
user/gaurasingh/fix-minackdelay-clamp
branch
from
August 12, 2026 13:50
599ca3d to
2f3ce4f
Compare
Guillaume Hetier (guhetier)
approved these changes
Aug 12, 2026
Gaurav Singh (gaurav2699)
deleted the
user/gaurasingh/fix-minackdelay-clamp
branch
August 12, 2026 19:35
Masahiro Kozuka (masa-koz)
added a commit
to seera-networks/msquic
that referenced
this pull request
Aug 14, 2026
## Description Merge `main` (tracking upstream microsoft/msquic) into `seera-main`. 25 upstream commits since the last sync at `c2271990`. Notable ones for the fork: | Upstream | What it changes | |---|---| | microsoft#6217 | Path migration UAF — guards path promotion with an `InUse` check | | microsoft#6213 | Clamps the advertised `MaxAckDelay` to `MinAckDelay` | | microsoft#6047 | Discards client Initial keys when Handshake write keys are installed; adds `QUIC_PACKET_BUILDER::InitialKeysDiscarded` so a flush that frees allowance reschedules | | microsoft#6167 | Validates the Version Negotiation packet's Source CID against our Destination CID | | microsoft#6181 | Handles zero-length datagrams in epoll and io_uring | | microsoft#6198 | Upgrades the XDP dependency to v1.4 | | microsoft#6199 | Removes the obsolete Linux raw datapath (`datapath_raw_linux.c`, `datapath_raw_socket_linux.c`) and their clog artifacts | | microsoft#6155 | Shifts Linux OpenSSL CI to Ubuntu 26.04, and adds `NOLINTNEXTLINE` clang-tidy suppressions across core | Submodules move with it: googletest `fa005b2`→`c0afb7f`, openssl `1d6826b`→`3d9ddf4`, xdp-for-windows to v1.4. ### Conflicts Four, all resolved keeping the fork's behaviour and taking upstream's change where the two were independent. **`src/core/bbr.c`** — upstream added a `NOLINTNEXTLINE(clang-analyzer-security.ArrayBound)` above `QuicPathGetDatagramPayloadSize(&Connection->Paths[0])`. The fork reads `PathID->Path` there instead, since congestion control is per path ID under multipath, and has no `Connection` local at all. Kept the fork's expression with upstream's suppression above it. **`src/core/crypto.c`** — upstream added the same kind of suppression above `Connection->State.HandshakeConfirmed = TRUE`; the fork had added an early return when it is already set. Independent, so both are kept, guard first. Both suppressions are needed, and the one in `crypto.c` had to move. The analyzer reports at the *first* access to the containing record, which in the fork's `crypto.c` is the guard, not the assignment upstream put their comment above; `NOLINTBEGIN`/`NOLINTEND` now covers both statements so it does not have to be revisited if the reported line shifts. Verified by running the CI's own toolchain — clang-tidy 21.1.8, from `ghcr.io/microsoft/msquic/linux-build-xcomp:ubuntu-26.04-cross` — over every source file this fork changes relative to upstream: all 26 pass, and removing either suppression reproduces the exact CI error. **`.github/workflows/test.yml`, `.github/workflows/stress.yml`** — upstream bumped the Windows XDP jobs from `xdp-v1.1` to `xdp-v1.4` and, in the same region, added `WinServerPrerelease` matrix entries. Took the XDP bump; did not reintroduce `WinServerPrerelease`, which #14 removed from this fork because those jobs need Microsoft's self-hosted `1ES.Pool=1es-msquic-pool`. The fork's `windows-2025` substitutes for the two `Full.Verbose` schannel jobs are kept. Upstream's deletion of the openssl+XDP recvfuzz entry is taken as a plain deletion. After the merge no `xdp-v1.1` reference and no `WinServerPrerelease` matrix entry remains. ## Testing | Sweep | Result | |---|---| | `*Multipath*:*Path*:*Migration*:*UnconnectedSocket*:*KeepAlive*` | 109 tests pass | | `*Basic*:*Datagram*:*Receive*:*Recv*:*Handshake*` | 986 tests pass | Build is clean, no compiler warnings. The first push failed the six `-CodeCheck` jobs on a single diagnostic at `crypto.c:490`, fixed in the follow-up commit. Local clang-tidy is 18 and does not reproduce it at all, which is why the merge commit shipped with the suppression in the wrong place; the CI container was used for verification afterwards. Spot-checked that the fork's own work survived the merge intact: the per-path keep alive (`Path->LastSendTimeUs` stamped in `QuicPacketBuilderSendBatch`, read by `QuicConnPathKeepAliveTimerUpdate`), and that microsoft#6217's active-path promotion guard sits inside the existing `!MultipathNegotiated` branch so it does not affect multipath connections. ## Documentation No documentation change of its own. `docs/Settings.md`, `docs/XDP.md`, `docs/Release.md` and `docs/TEST.md` move with upstream. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Masahiro Kozuka (masa-koz)
added a commit
to seera-networks/msquic
that referenced
this pull request
Aug 14, 2026
Brings in #76 (per-path keep alive timer) and #77 (upstream main sync, 25 commits). Conflicts: scripts/build.rs qmux-01 added iOS/Apple support around the link search path; seera-main replaced the single "lib" path with a lib/lib64 probe (upstream microsoft#6168). Independent: took the probe, kept the `|| apple_ios` condition and the Apple framework branch. `path_extra` is gone with the single path it fed. src/core/connection.c QuicConnGenerateLocalTransportParameters. qmux-01 wrapped the UDP-only transport parameters in !QuicConnIsQMux; seera-main added the MinAckDelay clamp (upstream microsoft#6213). Put the clamp inside the non-QMux block, next to the MaxAckDelay/MinAckDelay it constrains -- QMux sets neither. src/core/send.c QuicSendCanSendFlagsNow. qmux-01 wrapped the check in !QuicConnIsQMux; seera-main added a clang-tidy ArrayBound suppression above it (upstream microsoft#6155). Kept both, with the suppression on the line it was written for. That alone was not enough. QuicConnIsQMux is now the first dereference of Connection, and being QUIC_INLINE the analyzer reports inside the helper in connection.h rather than at the call site, where no NOLINT in send.c can reach it. Suppressed inside QuicConnIsQMux instead, matching what upstream microsoft#6155 already does for QuicConnIsClosed one function below. Verified with the CI toolchain rather than the local one, which is too old to see these diagnostics: a full -CodeCheck build (clang-tidy 21, Debug, linux, x64, quictls) inside ghcr.io/microsoft/msquic/linux-build-xcomp: ubuntu-26.04-cross completes clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Masahiro Kozuka (masa-koz)
added a commit
to masa-koz/msquic-async-rs
that referenced
this pull request
Aug 14, 2026
## Summary Bumps the `seera-msquic` submodule `5b21c4f` → `146714f`. Unlike the recent bumps this is not a fork change: it is `seera-main` taking **25 commits of upstream microsoft/msquic** ([seera#77](seera-networks/msquic@146714f)), plus one fork commit adjusting a static-analysis suppression the merge displaced. 106 files, +544/-5355 — most of the deletions are obsolete Linux raw datapath files and Azure pipeline definitions upstream removed. **Nothing in this crate needed changing.** `src/rs` is untouched, so the Rust bindings and every signature this crate calls are unchanged; the only public header change is one line in `msquic.hpp`, which is C++ and unused here. ## What is worth knowing Upstream fixes that reach this crate's users: - **Path migration use-after-free** ([#6217](microsoft/msquic#6217)). `QuicConnRecvPostProcessing` promoted a path on a non-probing frame without checking `(*Path)->InUse`, so a packet on a stale path entry could promote freed state. A one-line guard — and it is in the path handling this project exercises through `Connection::add_path()`. - Zero length datagrams are handled in the epoll and iouring datapaths ([#6181](microsoft/msquic#6181)). - `MinAckDelay` is clamped to `MaxAckDelay` in the local transport parameters ([#6213](microsoft/msquic#6213)). - Client Initial keys are discarded when Handshake write keys are installed ([#6047](microsoft/msquic#6047)), and VN packet source CIDs are validated ([#6167](microsoft/msquic#6167)). - `build.rs` no longer fails to link on lib64 distros ([#6168](microsoft/msquic#6168)) — about building this crate rather than running it. The fork commit `f465ff0` moves a clang-analyzer `ArrayBound` suppression in `crypto.c` onto the fork's early return, which is where the analyzer now reports because that guard reads `Connection` one statement before upstream's assignment. Build-only. ## Nested submodules The merge moves seera-msquic's own submodules: **openssl** `1d6826b` → `3d9ddf4`, googletest `fa005b2` → `c0afb7f`, xdp-for-windows `ccd59c2` → `d372b52`. A checkout that only updates the top-level submodule will build the new source against the *old* openssl, so this bump wants `git submodule update --init --recursive` rather than a plain `git submodule update`. Both were updated here before building. ## Test plan - `cargo test -p msquic-async --lib --no-default-features --features tokio,msquic-seera` — 43 passed, on a full rebuild of the vendored C library and the updated openssl, not a cached one 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
When MaxAckDelayMs is 0 (delayed ACKs disabled), MaxAckDelay resolves to 0 while MinAckDelay is derived independently from the timer resolution and stays more than 0. This makes MsQuic advertise min_ack_delay > max_ack_delay, which trips a CXPLAT_DBG_ASSERT in QuicCryptoTlsEncodeTransportParameters in Debug builds and produces an invalid transport parameter (rejected by peers, including MsQuic) in Release builds.
Clamp MinAckDelay so it never exceeds MaxAckDelay. Surfaced by SpinQuic settings randomization coverage.
Fixes: #6212