Preserve full width of SETTINGS frame identifiers - #13627
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new unit test frees its MIOBuffer before destroying the frame that deallocates the associated IOBufferReader, which can trigger UAF/null deref in the frame destructor.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes an HTTP/3 SETTINGS parsing correctness issue by preserving the full-width QUIC variable-length SETTINGS identifier during parsing, preventing unknown identifiers from aliasing known ones via truncation.
Changes:
- Widen decoded SETTINGS identifiers in
Http3SettingsFrame::_parse()fromuint16_ttouint64_t. - Add a regression unit test ensuring a wide unknown SETTINGS identifier (e.g.,
0x10001) is ignored rather than misinterpreted asHEADER_TABLE_SIZE.
File summaries
| File | Description |
|---|---|
| src/proxy/http3/Http3Frame.cc | Preserve full-width decoded SETTINGS identifier to avoid truncation-based aliasing. |
| src/proxy/http3/test/test_Http3Frame.cc | Add regression test covering wide unknown SETTINGS identifier handling. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Http3SettingsFrame::_parse() decoded each SETTINGS identifier with read_QUICVariableInt(), which returns uint64_t, but stored the result in a uint16_t. An identifier outside the known set whose low 16 bits happen to match a known ID was therefore misread as that setting. Widen the local to uint64_t so the comparison against the known-ID set happens at full variable-length integer width.
aaf483a to
9c6d9ce
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The updated test correctly orders teardown to avoid reader deallocation after freeing the backing MIOBuffer, but other existing tests in the same file still free buffers while frames/readers are alive, risking use-after-free and test flakiness.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
maskit
left a comment
maskit
left a comment
There was a problem hiding this comment.
Three changes requested — see inline.
RFC 9114, Section 7.2.4.1 reserves setting identifiers of the form 0x1f * N + 0x21 to exercise the requirement that unknown identifiers be ignored, and says endpoints should include at least one such setting in their SETTINGS frame. Use one of those instead of an arbitrary value, so the test covers an identifier that is actually sent on the wire: 0x1f * 33824 + 0x21 == 0x100001 shares its low 16 bits with HEADER_TABLE_SIZE (0x01). The varint encoding, the frame length and the assertions are unchanged. Also drop the extra Catch2 tag, so the case is tagged [http3] like the rest of the file. Release the frames before the MIOBuffer they read from in "Load DATA Frame" and "Load SETTINGS Frame". ~Http3Frame calls IOBufferReader:: dealloc(), which goes through the reader's MIOBuffer, and free_MIOBuffer() returns that buffer to the ioAllocator freelist, so the existing order touches the buffer after it is released.
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
The other two load-path teardowns carry a note explaining why the frames are released before free_MIOBuffer(); add the same one here so all three read alike.
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
|
[approve ci autest 1 2] |
|
[approve ci autest 1] |
Http3SettingsFrame::_parse()decodes each SETTINGS identifier withQUICIntUtil::read_QUICVariableInt(), which returnsuint64_t, but stored theresult in a
uint16_tlocal (src/proxy/http3/Http3Frame.cc:459). RFC 9114defines SETTINGS identifiers as variable-length integers and requires
unrecognized identifiers to be ignored; narrowing the decoded value before the
comparison against the known-ID set means an identifier outside that set can
alias a known one on its low 16 bits.
This widens the local to
uint64_t. Nothing else changes -- the surroundingparse loop and the known-ID comparison are untouched.
Test
Adds
Load SETTINGS Frame ignores reserved identifiertotest_Http3Frame.cc,tagged
[http3]. RFC 9114, Section 7.2.4.1 reserves identifiers of the form0x1f * N + 0x21to exercise the requirement that unknown identifiers beignored, and expects endpoints to send one. The test feeds a SETTINGS frame
whose identifier is the 4-byte varint encoding of
0x1f * 33824 + 0x21 == 0x100001-- sharing its low 16 bits withHEADER_TABLE_SIZE(0x01) -- and asserts the frame parses as valid whilenot containing
HEADER_TABLE_SIZE.Confirmed this is a regression test and not a tautology: with the
uint16_trestored it fails on
CHECK_FALSE(contains(HEADER_TABLE_SIZE))(expansion!true); with the fix it passes, 4 assertions.Also run: full
test_http3(138 assertions / 16 cases, up from 134/15 onmaster) and the
h3_proxy_verifierautest.Load DATA FrameandLoad SETTINGS Framenow release their frames beforefree_MIOBuffer().~Http3Framedeallocates itsIOBufferReaderthrough theMIOBufferthat reader came from, so the previous order let the reader outliveits buffer.