You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@james.apache.org by "gkm2164 (via GitHub)" <gi...@apache.org> on 2026/08/31 03:15:11 UTC
[PR] Fix ImapRequestFrameDecoder dropping a pipelined command after a LITERAL+ literal [james-project]
gkm2164 opened a new pull request, #3144:
URL: https://github.com/apache/james-project/pull/3144
## Problem
When a client pipelines two commands using LITERAL+ (RFC 7888) - e.g. two
`APPEND`s back to back without waiting for a response in between, both
delivered to the server in a single TCP read - the **second command's
bytes are silently dropped**. No error, no timeout signal to the client,
no server-side log trace: the tag is simply never answered, and the
message is never persisted.
Minimal repro (raw IMAP, after `LOGIN`):
```
a1 APPEND INBOX {46+}
From: user@example.com
Subject: msg1
body1
a2 APPEND INBOX {46+}
From: user@example.com
Subject: msg2
body2
```
sent as one write. `a1 OK ... APPEND completed.` arrives; `a2` never does,
and `INBOX` only gains one new message, not two.
## Root cause
`ImapRequestFrameDecoder.obtainReader()` drains the *entire* readable
buffer into `pending` once it knows a literal's exact byte count, not just
the bytes the literal actually needs:
```java
int readableBytes = in.readableBytes(); // whole remaining buffer, incl. the next command
byte[] bytes = new byte[readableBytes];
in.readBytes(bytes);
pending.add(bytes);
```
Once the literal parses successfully, `parseImapMessage()` unconditionally
clears `pending`:
```java
if (!pending.isEmpty()) {
pending.clear(); // discards whatever of the next command was over-read
}
```
`pending` is a plain `List<byte[]>` snapshot with no notion of how much
the `ImapRequestLineReader` actually consumed from it, so any excess -
including a fully-formed next command sitting right behind the literal -
is gone.
This is a regression from 6f8bec68 (JAMES-4043, "Improve literal
handling"). Before that change, this path only ran when the literal
genuinely wasn't fully available yet (`size > in.readableBytes()`); when
enough data was already present in one read, `obtainReader()` built the
`ImapRequestLineReader` directly over Netty's own cumulation buffer `in`,
so the literal parser's own consumption naturally left the rest of `in`
(the next pipelined command) in place for `ByteToMessageDecoder`'s decode
loop to pick up on its next iteration. JAMES-4043 routed every path
through the always-buffer-into-`pending` mechanism - correct for the
slow/multi-read arrival it targeted, but it breaks the single-read case
where more than one command's worth of bytes is already available.
The same over-read pattern exists in `uploadToAFile()` (the
`inMemorySizeLimit`-exceeding path): it reads the whole buffer before
`FileChunkConsumer` caps what actually reaches the temp file, so a
pipelined command behind a large literal is read out of `in` and lost the
same way. Fixed there too.
## Fix
Only consume as many bytes as still needed to reach the literal's declared
size in both places, leaving any excess unread in `in` (or unconsumed from
the socket) for the existing decode loop to pick up as the next frame -
restoring the pre-JAMES-4043 behavior for the "already enough data"
case without touching the JAMES-4043 improvement for genuinely slow/multi-read
arrivals.
## Testing
- Added `IMAPServerTest.PipelinedLiteralAppend#secondPipelinedLiteralAppendShouldNotBeSilentlyDropped`:
two LITERAL+ `APPEND`s pipelined in one write; asserts both get a tagged
`OK ... APPEND completed` response (time-bounded, since without the fix
the second response never arrives at all).
- Manually verified against a live DynamoDB-backed James 3.9.0 deployment
where `imaptest` (Dovecot) first surfaced the drop under concurrent
load with LITERAL+ `APPEND`s carrying custom keyword flags; isolated to
this exact two-commands-in-one-read pipelining pattern via a minimal
raw-socket repro, traced to this method with instrumented local builds,
and confirmed the fix resolves both the missing response and the missing
mailbox data (`EXISTS` count) before opening this PR.
Marking as draft since this is a first-time contribution - happy to adjust
approach, add more test coverage (e.g. the `uploadToAFile` path
specifically), or split into two commits if that's preferred.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] [FIX] ImapRequestFrameDecoder drops a pipelined command after a LITERAL+ literal [james-project]
Posted by "gkm2164 (via GitHub)" <gi...@apache.org>.
gkm2164 commented on PR #3144:
URL: https://github.com/apache/james-project/pull/3144#issuecomment-5474543958
Thanks for showing attention! currently, I'm running jenkins, and I found several issues with current fix..
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] [FIX] ImapRequestFrameDecoder drops a pipelined command after a LITERAL+ literal [james-project]
Posted by "gkm2164 (via GitHub)" <gi...@apache.org>.
gkm2164 commented on PR #3144:
URL: https://github.com/apache/james-project/pull/3144#issuecomment-5482324206
Thanks for the information. I'm expecting this is only happening towards the bad clients, generally, some short-tempered clients where it just sends everything before each commands processed or not, like dovecot testing...
It's honor to make such a contribution!
I'll create another PR over the 3.9.x, as well, once this gets merged
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] [FIX] ImapRequestFrameDecoder drops a pipelined command after a LITERAL+ literal [james-project]
Posted by "gkm2164 (via GitHub)" <gi...@apache.org>.
gkm2164 commented on PR #3144:
URL: https://github.com/apache/james-project/pull/3144#issuecomment-5474531768
Closing this one: validating it against the full `protocols-imap4` suite showed the approach here is wrong, and the reworked fix is different enough that a fresh PR will be easier to review than a rewrite of this branch.
For the record, what this patch got wrong: it bounded each read to the literal size reported by `NotEnoughDataException#getNeededSize()`. That value is `size + read + crlf`, where `read` only counts characters pulled through `nextChar()` — bytes already consumed by an earlier `read(int, boolean)` on the same line are not included. So for a command carrying several literals it under-reports, and anything that trusts it precisely breaks (`IMAPServerSearchTest#passing2literalOnSameNetworkPackageWhenMoreThan16Chars` deadlocks). The existing "read everything, compare loosely" code is not sloppy — it is what makes that inaccuracy harmless, which I did not appreciate when I opened this.
The rework keeps that failure path exactly as it is today and instead fixes only the success path, which is where the actual bug lives: parse against a non-consuming view of Netty's cumulation, then advance the reader index by what the command actually used, so a pipelined follow-up command is left in the buffer instead of being dropped by `pending.clear()`. That needs no size arithmetic at all and removes the `should_buffer` bookkeeping.
I will open a new PR once it is green locally. Apologies for the CI cycles spent on this branch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] [FIX] ImapRequestFrameDecoder drops a pipelined command after a LITERAL+ literal [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on PR #3144:
URL: https://github.com/apache/james-project/pull/3144#issuecomment-5478654364
> First of all, I'm a big fan of JAMES project. Lol
THANKS!
> Has this been already a problem previously?
We did not encounter this bug of piped APPEND command yet to be honnest. But (as you might have seen) the parsing of potentially long IMAP littleral is a quangmire which do not have any easy solution.
Regarding the changeset I do agree with what is done (better cumulation managenebt rather than dropping the full BytBuff + the code extractions.
I'll wait for a :green_apple: build.
For the context that's some code that had been written in the light of `CVE-2024-37358: Denial of service through the use of IMAP literals`. I wrote that code by hand and did not account for piped litterals so it's very possible that I ended up doing a mistake.
I'll wait for a :green_apple: build, let this open for 3 day to let a chance for people in the community to react. Then we will merge this timely.
If you want the fix to be backported to `3.9.x` then fill free to open a PR against it: that way you might not need to do a big work to benefit from it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] [FIX] ImapRequestFrameDecoder drops a pipelined command after a LITERAL+ literal [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa merged PR #3144:
URL: https://github.com/apache/james-project/pull/3144
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] [FIX] ImapRequestFrameDecoder drops a pipelined command after a LITERAL+ literal [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on PR #3144:
URL: https://github.com/apache/james-project/pull/3144#issuecomment-5475044764
Ok
The existing test suite is ... Quite exhaustive !
Do not hesitate to wave a hand if you want to get some help on the topic, and thanks a lot for the contribution.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] [FIX] ImapRequestFrameDecoder drops a pipelined command after a LITERAL+ literal [james-project]
Posted by "gkm2164 (via GitHub)" <gi...@apache.org>.
gkm2164 commented on PR #3144:
URL: https://github.com/apache/james-project/pull/3144#issuecomment-5477180834
First of all, I'm a big fan of JAMES project. Lol
I found this while I was to build AWS infra version..(DynamoDB, S3, SNS/SQS usage version), and honestly, when testing the feature parity with Claude Code.
As far as I know, we use APPEND for doing Sent, or Draft update with minimal operations without full use of SMTP and this is a part of some small issue.
Has this been already a problem previously?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org