Fix ASCII case-fold collision between distinct tchar punctuation bytes - #3713
Open
shoemoney wants to merge 1 commit into
Open
Fix ASCII case-fold collision between distinct tchar punctuation bytes#3713shoemoney wants to merge 1 commit into
shoemoney wants to merge 1 commit into
Conversation
Motivation:
compareCaseInsensitiveASCIIBytes(to:) in ByteCollectionUtils.swift folds
case by masking every byte with & 0xdf, clearing bit 0x20. That bit is
what separates an ASCII lowercase letter from its uppercase form, but it
is also the only bit separating five unrelated punctuation byte pairs
from one another: ^(0x5e)/~(0x7e), [(0x5b)/{(0x7b), ](0x5d)/}(0x7d),
\(0x5c)/|(0x7c), and @(0x40)/`(0x60). Masking a byte from any of the
right-hand column produces the same value as its left-hand partner, so
the comparison treats them as identical even though every one of these
ten bytes is a legal tchar character in an HTTP header field name per
RFC 7230 3.2.6, and is accepted as such by NIOHTTP1's own header name
validator. In practice this means two genuinely distinct header names,
e.g. X-Foo^Bar and X-Foo~Bar, compare equal, and the collision reaches
every HTTPHeaders API built on this comparison: first(name:), the
subscript family (including canonicalForm:), contains(name:), and
remove(name:).
Modifications:
Replace the unconditional & 0xdf mask with a small helper that only
clears bit 0x20 when the byte is actually an ASCII lowercase letter,
leaving every other byte untouched. The helper is a single UInt8(ascii:)
range switch, matching the idiom already used elsewhere in NIOHTTP1
(HTTPHeaderValidator.swift, HTTPHeaders+Validation.swift), so both the
fast withContiguousStorageIfAvailable loop and the elementsEqual
fallback stay branch-light.
Result:
Header names differing only by one of these five punctuation pairs are
no longer treated as equal, while ASCII letter case-folding is
unaffected. Added a byte-level test covering all five colliding pairs
plus same-byte and letter-case sanity checks, and an HTTPHeaders-level
test exercising the bug through first(name:), contains(name:),
subscript(canonicalForm:), and remove(name:). Both fail against the old
implementation and pass with the fix; the full NIOHTTP1Tests suite
passes.
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
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.
Motivation:
ByteCollectionUtils.swift'scompareCaseInsensitiveASCIIBytesfolds case bymasking every byte with
& 0xdf(clearing bit0x20). That bit is whatdistinguishes an ASCII lowercase letter from its uppercase form, but it is
also the only bit separating several unrelated punctuation bytes from one
another:
& 0xdf& 0xdf^~[{]}\|@`Every byte in the right-hand column masks down to the same value as its
left-hand partner, so the current implementation treats them as identical.
All ten of these bytes are legal
tcharcharacters in an HTTP header fieldname per RFC 7230 §3.2.6, and are accepted as such by NIO's own header-name
validator (
HTTPHeaders+Validation.swift). The result: two genuinelydifferent header names collide.
Because
compareCaseInsensitiveASCIIBytesbacksString.isEqualCaseInsensitiveASCIIBytes,which backs
HTTPHeaders's name comparisons, this collision propagates to:HTTPHeaders.first(name:)HTTPHeaders.subscript(_:)/subscript(canonicalForm:)HTTPHeaders.contains(name:)HTTPHeaders.remove(name:)Modifications:
Replace the unconditional
& 0xdfmask with a helper that only clears bit0x20when the byte is actually an ASCII lowercase letter (a...z),leaving every other byte — punctuation included — untouched:
This is a single range check (matches the
UInt8(ascii:)range-switch idiomalready used elsewhere in
NIOHTTP1, e.g.HTTPHeaderValidator.swiftandHTTPHeaders+Validation.swift), so it stays branch-light and does notregress the hot path both the fast (
withContiguousStorageIfAvailable) andfallback (
elementsEqual) comparison loops sit on.Result:
Header names that differ only by one of these five punctuation pairs are no
longer treated as equal, while ASCII letter case-folding (the actual purpose
of the function) is unaffected.
Tests:
Added a test to
Tests/NIOHTTP1Tests/ByteBufferUtilsTest.swiftexercisingcompareCaseInsensitiveASCIIBytesdirectly against all five colliding pairs(plus a same-byte and letter-case sanity check), and a test to
Tests/NIOHTTP1Tests/HTTPHeadersTest.swiftexercising the bug through thepublic
HTTPHeadersAPI (first(name:),contains(name:),subscript(canonicalForm:),remove(name:)) with the^/~pair.Confirmed both new tests fail against the old implementation (5 assertion
failures in the byte-level test, 4 in the
HTTPHeaders-level test) and passafter the fix, with the full
NIOHTTP1Testssuite green.