Fix DoQ response parsing to respect length prefix and add partial-read regression test #484
Conversation
![windsurf-bot[bot]](https://avatars.githubusercontent.com/in/1066231?s=60&v=4){kind=small}
| msgLen := int(binary.BigEndian.Uint16(lenBuf[:])) | ||
| if msgLen == 0 { | ||
| return nil, fmt.Errorf("empty response from %s", p.addr) | ||
| } else if msgLen > dns.MaxMsgSize { | ||
| return nil, fmt.Errorf("response size %d exceeds max %d from %s", msgLen, dns.MaxMsgSize, p.addr) | ||
| } |
There was a problem hiding this comment.
Important fix: The previous implementation was ignoring the length prefix and potentially processing stale data from the buffer pool. This change properly validates the message length and ensures we only unpack the actual message bytes.
| // does not support receiving multiple messages over a single connection. | ||
| m = new(dns.Msg) | ||
| err = m.Unpack(respBuf[2:]) | ||
| err = m.Unpack(respBuf) |
There was a problem hiding this comment.
Correct fix for the unpacking logic. Since we're now reading the length prefix separately and then reading exactly the message bytes into the buffer, we should unpack the entire buffer rather than skipping the first 2 bytes.
There was a problem hiding this comment.
Pull request overview
This PR fixes a subtle DoQ (DNS-over-QUIC) response parsing bug where partial reads combined with buffer pooling could cause stale data from previous queries to contaminate current responses, leading to incorrect DNS records appearing in answers.
Changes:
- Updated
readMsgto properly handle the 2-byte length prefix by usingio.ReadFullfor deterministic reading - Added length validation to ensure responses are within valid bounds
- Added comprehensive regression test that simulates partial reads with pooled buffers containing stale data
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| upstream/doq.go | Introduced quicStream interface for testability and rewrote readMsg to properly read and respect the RFC 9250 length prefix, eliminating the stale-buffer issue |
| upstream/doq_internal_test.go | Added TestUpstreamDoQ_readMsg_partialRead regression test with mock stream that simulates partial reads and pre-filled buffer pool to validate the fix |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
When the QUIC server parsed requests it passed the entire pooled buffer to dns.Msg.Unpack, even though only n bytes were read. This could let leftover bytes from previous requests influence parsing. Tighten the slice to the actual read length and add a regression test that simulates a truncated request with a prefilled pool buffer.
1 participant
This PR fixes two related DoQ parsing bugs: the client response path could mix stale pooled bytes into responses on partial reads (surfacing as rare “wrong domain” A/AAAA answers), and the QUIC server request path could parse beyond the actual read length, allowing leftover pooled bytes to influence request parsing.
What changed
upstream/doq.go: SwitchedreadMsgtoReadFullthe 2‑byte length prefix, validate it,ReadFullthe exact message bytes, and unpack only those bytes.upstream/doq_internal_test.go: AddedTestUpstreamDoQ_readMsg_partialReadto simulate 1‑byte chunk reads with a prefilled pool buffer.proxy/serverquic.go: Unpack only the bytes actually read (buf[2:n]/buf[:n]) to avoid stale pooled bytes affecting request parsing.proxy/serverquic_internal_test.go: AddedTestQuicProxy_truncatedRequestUsesOnlyReadBytesto simulate a truncated request with a prefilled pool buffer.Why this matters
DoQ messages are length‑prefixed and stream reads are not guaranteed to return a full message in one call. When combined with buffer pooling, ignoring the length or the actual read size can let leftover bytes be parsed as part of a different message, causing incorrect responses or unintended request handling. These changes make DoQ parsing deterministic and resilient to partial reads and pooled buffers.
Test
go test ./upstream -run TestUpstreamDoQ_readMsg_partialReadgo test ./proxy -run TestQuicProxy_truncatedRequestUsesOnlyReadBytes