Fix CR LF handling in FramingParser#830
Open
danieleggert wants to merge 3 commits into
Open
Conversation
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.
Fix a remotely-triggerable crash in
FramingParserwhen a bareCRends one inbound buffer and a bareLFterminates the next.Motivation:
FramingParserframes client commands server-side (FrameDecoder→IMAPServerHandler). When a buffer ends on a bareCR, it completes that frame but enters the.ignoreFirststrategy, since the partnerLFmay arrive in the next read.That strategy was only cleared if the next byte was actually a leading
LF. If the next buffer instead began with content terminated by a bareLF(e.g.A1 NOOP\rthenA2 NOOP\n), the stale strategy meant the trailingLFreachedprocessLineBreakByte_statewithframeLength > 1and trippedprecondition(self.frameLength == 1), aborting the whole process. AsFrameDecodersits ahead of any auth handler, any connecting client could trigger this.This is the command-side counterpart to the response-side split-CRLF fix in #812:
FramingParseralready handled split CRLF via.ignoreFirst; this repairs a latent bug in that machinery.Modifications:
In
readByte_state_normalTraversal, reset.ignoreFirstto.includeInFramewhen a non-LFbyte is consumed — a fresh frame has begun, so a laterLFis a genuine terminator. This is the fix.As defense-in-depth, replace the
preconditionwiththrow InvalidFrame(); the.normalTraversal/.insideQuotedcall sites catch it, reset state, and emit a recoverable.invalidframe. With the fix above this path is unreachable in practice. Add tests: targetedFramingParsercases for every bare-CRcontinuation, plus integration tests asserting a well-formed stream parses to the sameCommandStreamParts — and never crashes — at every single- and pair-of-byte split.Result:
A bare
CRat a buffer boundary followed by bare-LF-terminated content now frames cleanly instead of crashing. Well-formed input parses identically regardless of how it's split, and non-conforming bare-CR/ bare-LFterminators are tolerated. No previously-successful parse changes, and there's no API/ABI change — the throwing additions are confined toprivatemethods.