Release the in-flight field when a post decoder is destroyed by HuzaifaChaudary · Pull Request #17389 · netty/netty

HuzaifaChaudary · 2026-09-05T17:44:56Z

Motivation

destroy() on both post decoders releases everything in bodyListHttpData and asks the
factory to clean up , but the field that is still being decoded is in neither .

setFinalBuffer() only calls addHttpData() once a field terminates , so a partial one never
reaches bodyListHttpData . and DefaultHttpDataFactory.createAttribute only puts it in
getList(request) on the useDisk and checkSize branches . the memory fallthrough returns a
bare MemoryAttribute that nothing tracks .

so on the ordinary path where a client goes away in the middle of a form post and the server
calls decoder.destroy() , the in flight Attribute or FileUpload leaks , along with the
retained slice of the inbound buffer it holds . netty's own leak detector fires on it .

this only bites when the factory hands back untracked memory data , which is
new DefaultHttpDataFactory(false) , the documented always in Memory mode , or a custom
factory . the default constructor , MINSIZE and useDisk=true all already reach refCnt 0 ,
i checked all three .

#11188 added that bodyListHttpData release loop saying destroy() should clean up memory
based data too , it just did not cover the item still in progress .

Modification

release currentAttribute , and currentFileUpload in the multipart decoder , at the end of
destroy() . the refCnt() > 0 guard matches the loop just above it , so a disk or mixed item
that cleanFiles() already freed is not released twice .

i did not go the other way and make the factory track memory data . that was tried in #10432
and closed as a breaking change , and #11109 was closed with MemoryAttribute is not concerned
by this method . nothing here touches the factory .

Result

no leak from a decoder destroyed mid field .

two tests added . both fail without the change with expected: <0> but was: <1> , and the leak
detector raises Possible memory leak detected on top , so they are not passing off their own
assertions .

codec-http is 8985 tests , 0 failures . checkstyle and forbiddenapis are clean on the module .

the same destroy() is on 4.1 , so this probably wants needs-cherry-pick-4.1 .

destroy() releases everything in bodyListHttpData, but a field that is still being decoded was never put there yet . with a memory only factory it is not tracked by the factory either , so cleanFiles() does not reach it and it is simply lost . this happens on the normal case where a client goes away in the middle of a form post and the server calls destroy() . release currentAttribute , and currentFileUpload in the multipart decoder , the same way the list is released .

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

normanmaurer · 2026-09-08T08:10:14Z

+        // A HttpData that is still being decoded was never added to bodyListHttpData, and a memory based
+        // one is not tracked by the factory either, so cleanFiles() above does not cover it.
+        if (currentFileUpload != null) {
+            if (currentFileUpload.refCnt() > 0) {


why do we need to check the reference count ? Same question for the other release calls

cleanFiles() runs two lines above this and it releases everything the factory is holding , without checking . the default DefaultHttpDataFactory has checkSize true , so createAttribute and createFileUpload put the in progress one on that list , and by the time destroy() gets down here the count is already 0 . so the check is not the "user might have released it" reason that the loop below has , it is the release that just happened a few lines up .

i took all three checks out and ran the class : 9 errors , every one of them IllegalReferenceCountException: refCnt: 0, decrement: 1 . seven are tests that were already there , testTooManyFormFieldsPostStandardDecoder , testFieldGreaterThanMaxBufferedBytesStandardDecoder and the four broken hex byte ones . with the checks back it is 42 of 42 .

worth saying that my two tests would not have caught that . they use new DefaultHttpDataFactory(false) , the memory only path where the factory does not track it at all . i have pushed two more that use the default factory so the tracked side is covered as well .

yawkat · 2026-09-08T16:57:55Z

Do you have an application that is buggy because of this? Reference counting in the POST decoders is a mess, and frameworks have all sorts of workarounds for it. I would be hesitant to accept a change that may break those frameworks, even if this could be considered a bug.

the three tests already here build the factory directly. the case that reaches this in a server is a memory only factory that overrides only createFileUpload, which is the shape of vert.x's NettyFileUploadDataFactory, with the buffered bytes limit tripping while a field is still being decoded. that is the path a framework takes when it catches the decoder exception and calls destroy() in response. fails without the change with expected: <0> but was: <1>, and the leak presence extension raises Possible memory leak detected on the class on top of it.

HuzaifaChaudary · 2026-09-09T17:44:26Z

no, i do not have a production application that this is breaking. what i have is the
configuration and a framework already sitting in it, so let me give you that instead of
an assertion.

who is in the leaking configuration. vert.x's NettyFileUploadDataFactory extends
DefaultHttpDataFactory with super(false) and overrides only createFileUpload. so
attributes fall through to the last branch of DefaultHttpDataFactory.createAttribute,
which returns a bare MemoryAttribute and does not put it on getList(request).
Http1ServerRequest.onData catches ErrorDataDecoderException, TooLongFormFieldException
and TooManyFormFieldsException and calls decoder.destroy() right there, while
currentAttribute is still holding its retained slice. isValidMultipartContentType
accepts application/x-www-form-urlencoded, so this is the standard decoder as well as the
multipart one.

i pushed a fourth test in that shape , memory only factory overriding only the upload
creation , buffered bytes limit tripping mid field. expected: <0> but was: <1> without the
change, and the leak presence extension raises Possible memory leak detected on the class
on top of the three assertion failures.

on breaking the workarounds. the only way the in flight item leaves the decoder is
currentPartialHttpData(), so i went through every caller i could find:

vert.x reads it in handleException and casts to its own NettyFileUpload. that class
returns refCnt() == 1 unconditionally and release() is a no op returning false, so
this change cannot be observed there at all. and on the path where they do destroy, they
null their decoder field before calling handleException, so it reads no decoder.
jooby's HttpRawPostRequestDecoder implements InterfaceHttpPostRequestDecoder
itself with its own destroy(). it does not inherit this one.
quarkus's QuarkusMultipartResponseDecoder is a vendored copy inside
resteasy-reactive, not a subclass.

the break i would worry about is a framework that keeps the in flight item past destroy()
and releases it itself, since it would now be releasing at 0. the guard here is
refCnt() > 0 on our side and the field is nulled, so a double release from netty is not
the risk , theirs after ours is. i did not find a caller that does it, and if you know of
one i would rather gate this than guess.

scope, for what it is worth: the default factory, MINSIZE and useDisk=true all already
reach 0 through cleanFiles(). the only configuration whose behaviour changes is the
untracked memory one, new DefaultHttpDataFactory(false) and custom factories built on it.

and #8045 is the same leak from the other side. the answer there was "You need to call
destroy() on the decoder", which is exactly what this makes true for the item still
being decoded.

…h refCnt Remove the item from the factory before cleanFiles(), so a tracking (mixed/disk) factory does not release it first, then release it unconditionally. The in-progress item is never handed out (current* is cleared as soon as it is added to bodyListHttpData), so the decoder is its sole owner and no refCnt check is needed. Co-Authored-By: Claude Opus 5.5 <[email protected]>

yawkat

claude found an alternative solution without refCnt

testlens-app · 2026-09-25T10:50:08Z

🚨 All tests passed but jobs failed 🚨

Failed Jobs without Test Failures

❌ Build PR / linux-x86_64-java11-awslc build
❌ Build PR / linux-x86_64-java11-pooled build
❌ Build PR / linux-x86_64-java25 build
❌ Build PR / macos-aarch64-java11-boringssl build
❌ CodeQL / Analyze (cpp)
❌ CodeQL / Analyze (java)
❌ Verify native library loading on musl / build-jars-x86_64

🏷️ Commit: 9d5b9df
▶️ Tests: 387664 executed
⚪️ Checks: 21/21 completed

Rerun Controls

Click the checkbox to trigger a rerun:

Rerun jobs

Learn more about TestLens at testlens.app/docs.

yawkat · 2026-09-25T16:06:57Z

I was wrong :( the guard is necessary.

The decoder is not the only one that can release the in-progress HttpData: it is handed out by currentPartialHttpData(), a tracking factory releases it on cleanFiles() or when the user cleans the factory directly. destroy() must not release it a second time in these cases. Co-Authored-By: Claude Opus 5.5 <[email protected]>

HuzaifaChaudary · 2026-10-10T11:41:48Z

merged 4.2 in to clear the conflict . it was only the test file , both sides had added tests at the end of HttpPostRequestDecoderTest , so both are kept . the decoder change is untouched . all 55 in that class pass on the merged tree , including the new content type ones from 4.2 . the workflows will need approving again .

Copilot AI lite review requested due to automatic review settings September 5, 2026 17:44

Copilot AI reviewed Sep 5, 2026

normanmaurer reviewed Sep 8, 2026

View reviewed changes

cover the factories that track the in progress HttpData too

b52447b

yawkat mentioned this pull request Sep 25, 2026

destroy() does not release the in-flight field of a partially decoded request netty-contrib/codec-multipart#42

Closed

yawkat and others added 2 commits September 25, 2026 12:34

Merge branch '4.2' into fix-post-decoder-destroy-leak

2eb6066

yawkat approved these changes Sep 25, 2026

View reviewed changes

yawkat mentioned this pull request Sep 25, 2026

Do not release the in-progress HttpData twice on destroy() netty-contrib/codec-multipart#66

Merged

Merge branch '4.2' into fix-post-decoder-destroy-leak

836ee4d

Navigation Menu

Repository navigation

Release the in-flight field when a post decoder is destroyed - #17389

Release the in-flight field when a post decoder is destroyed#17389
HuzaifaChaudary wants to merge 7 commits into
netty:4.2from
HuzaifaChaudary:fix-post-decoder-destroy-leak

HuzaifaChaudary commented Sep 5, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

normanmaurer Sep 8, 2026

Uh oh!

HuzaifaChaudary Sep 8, 2026

Uh oh!

yawkat commented Sep 8, 2026

Uh oh!

HuzaifaChaudary commented Sep 9, 2026

Uh oh!

yawkat left a comment

Uh oh!

testlens-app Bot commented Sep 25, 2026 •

edited

Loading

Uh oh!

yawkat commented Sep 25, 2026

Uh oh!

HuzaifaChaudary commented Oct 10, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

HuzaifaChaudary commented Sep 5, 2026

Motivation

Modification

Result

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

normanmaurer Sep 8, 2026

Choose a reason for hiding this comment

Uh oh!

HuzaifaChaudary Sep 8, 2026

Choose a reason for hiding this comment

Uh oh!

yawkat commented Sep 8, 2026

Uh oh!

HuzaifaChaudary commented Sep 9, 2026

Uh oh!

yawkat left a comment

Choose a reason for hiding this comment

Uh oh!

testlens-app Bot commented Sep 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🚨 All tests passed but jobs failed 🚨

Failed Jobs without Test Failures

Rerun Controls

Uh oh!

yawkat commented Sep 25, 2026

Uh oh!

HuzaifaChaudary commented Oct 10, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

testlens-app Bot commented Sep 25, 2026 •

edited

Loading