[FIX] ImapRequestFrameDecoder drops a pipelined command after a literal (3.9.x backport) - #3145
Open
gkm2164 wants to merge 1 commit into
Open
[FIX] ImapRequestFrameDecoder drops a pipelined command after a literal (3.9.x backport)#3145gkm2164 wants to merge 1 commit into
gkm2164 wants to merge 1 commit into
Conversation
When a command carrying a literal is followed by another command in the same write - two LITERAL+ APPENDs, say - the second one silently vanishes: no error, no timeout, nothing persisted. obtainReader() copies everything Netty currently holds into `pending`, so the bytes of that next command are swept in as well. parseImapMessage() then clears `pending` on a successful parse, and they are gone. Copying less is not a viable fix on its own: the byte count carried by NotEnoughDataException is `size + read + crlf`, where `read` only counts characters pulled through nextChar(), so it under-reports for a command holding several literals and cannot be trusted as an exact bound. Instead, stop consuming before we know how much was used. obtainReader() now parses against a view - `pending` followed by a slice of Netty's cumulation - and on a successful parse the cumulation is advanced by what the command actually consumed. Whatever it did not touch stays where it is and gets decoded as the next command. On failure everything is kept and the parse is simply retried once more bytes arrive, which is what keeps multi-literal commands working. NEEDED_DATA keeps its historical meaning - how many bytes must still arrive beyond what `pending` holds - because uploadToAFile()'s completion check depends on it; since the reader now counts from the start of `pending`, requestMoreData() rebases the reported size to that delta (leaving the UNKNOWN_SIZE sentinel untouched). The file path itself is unchanged. This also removes the `should_buffer` bookkeeping and the size comparison in obtainReader(): both existed only to decide when a parse was worth retrying, which the parser now decides for itself. See apache#3144 for the imaptest repro and rawlog trace this was found with. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
chibenwa
approved these changes
Aug 31, 2026
chibenwa
left a comment
chibenwa
left a comment
Contributor
There was a problem hiding this comment.
Pretty much a cherry-pick right?
Ok with me too.
chibenwa
approved these changes
Aug 31, 2026
chibenwa
left a comment
chibenwa
left a comment
Contributor
There was a problem hiding this comment.
Pretty much a cherry-pick right?
Ok with me too.
chibenwa
approved these changes
Aug 31, 2026
chibenwa
left a comment
chibenwa
left a comment
Contributor
There was a problem hiding this comment.
Pretty much a cherry-pick right?
Ok with me too.
chibenwa
approved these changes
Aug 31, 2026
chibenwa
left a comment
chibenwa
left a comment
Contributor
There was a problem hiding this comment.
Pretty much a cherry-pick right?
Ok with me too.
gkm2164
marked this pull request as ready for review
August 31, 2026 19:36
Author
|
Yeap. And also verified this also passes the Dovecot imap test |
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.
Backport of #3144 to
3.9.x, as discussed there.What this is
ImapRequestFrameDecodersilently drops a pipelined command that arrives in the same read as a preceding literal-bearing command (two LITERAL+APPENDs in one write: the second gets no response and is never persisted). Full analysis, wire captures and root cause are in #3144 — this PR carries the exact same decoder change (+76/−77, byte-identical to the master commit — I'll update this reference with the merged SHA once #3144 lands; opening as a draft until then).Worth noting: the bug was originally discovered on a 3.9.0 deployment (the
imapteststalls in #3144 were recorded against James 3.9.0), so3.9.xis confirmed affected, not just theoretically.Differences from the master PR
3.9.xand master in this file is theLeakAwareconstructor signature on a line the fix does not touch.AbstractIMAPServerTest, which does not exist on3.9.x. The same test method (secondPipelinedAppendAfterLiteralShouldNotBeLost, unchanged) therefore lives inIMAPServerTest's existingAppendNonSynchronizedLitteralsnested class, right next topartialCommandAfterNonSynchronizedLiteralShouldNotFail— the same neighborhood it occupies on master.Testing on 3.9.x
3.9.xdecoder plus only the new test,secondPipelinedAppendAfterLiteralShouldNotBeLostfails withexecution timed out after 5000 ms(bug reproduced); with the patch it passes.server/protocols/protocols-imap4module suite with the patch: 612 tests run, 0 failures (1 skipped),BUILD SUCCESS— including all 28 nested classes ofIMAPServerTest.One unrelated observation from validating this on a macOS host:
IMAPServerTestcontains nested classesSslandSSL, whose class files collide on case-insensitive filesystems. The surviving file then fails class loading (NoClassDefFoundError ... wrong name), and JUnit silently drops the whole class from discovery — a macOS developer running this module locally gets a green build with 568 of the tests never executed. Linux CI is unaffected. Happy to file that separately if useful.