Auto-port 5.0: HttpObjectEncoder / DefaultHttp2FrameWriter: fix buffer leak when a Throwable is thrown during header encoding - #17179
Merged
Conversation
… a `Throwable` is thrown during header encoding (#17089) ### Motivation Fixes #17088. Same class of bug as the `SslHandler` leak fixed in #17059: a header buffer is allocated, then a `Throwable` (typically `OutOfMemoryError`) is thrown before the buffer is handed off, leaving it unreleased. #6729 reported the exact symptom on `HttpObjectEncoder.encodeHeaders` back in 2017 but couldn't be reproduced on demand and was closed; the root cause was never fixed. Affected paths: - HTTP/1 — `encodeInitHttpMessage()` and `encodeFullHttpMessage()` in `HttpObjectEncoder`: `buf` from `ctx.alloc().buffer(...)` leaks if `encodeHeaders()` throws before it is added to `out`. - HTTP/2 — `writeHeadersInternal()`, `writePushPromise()` and `writeContinuationFrames()` in `DefaultHttp2FrameWriter`: the retained `fragment` / frame-header buffer leaks if a later allocation throws after the fragment is sliced off the header block. With pooled direct buffers this leaks off-heap memory the GC cannot reclaim, so repeated OOME on these paths ends in `OutOfDirectMemoryError` / process death. ### Modification - `HttpObjectEncoder`: guard the header buffer with a success/handed-off flag and release it in a `finally` unless ownership was transferred. In `encodeFullHttpMessage()` the flag is set immediately before `encodeByteBufHttpContent()` so the chunked path — where `buf` is already added to `out` before `encodeChunkedHttpContent()` can throw — does not double-release. - `DefaultHttp2FrameWriter`: hoist `fragment` to method scope, null it right after `ctx.write(fragment, ...)`, and release it in `finally` if non-null. `writeContinuationFrames()` additionally guards the reused frame-header buffer with a per-fragment flag. - Add tests to both modules using a tracking allocator that injects an `OutOfMemoryError` on a targeted allocation, asserting every tracked buffer reaches `refCnt() == 0` after the failure. ### Result No buffer leak when a `Throwable` is thrown mid header encoding. The normal path is unchanged. Measured with the tracking allocator (each OOME on these paths leaks exactly one header buffer, so the leak grows linearly with the number of affected requests): | path | leak / request | before | after | |---|---|---|---| | `HttpObjectEncoder` init / full | 256 B | `refCnt == 1` | `0` | | Http2 `writeHeaders` / `writePushPromise` | 256 B | `refCnt == 1` | `0` | | Http2 `writeContinuationFrames` (large headers) | 64 KiB | `refCnt == 1` | `0` | At scale on the 256 B paths that is ~244 MiB leaked per 1M affected requests; on the CONTINUATION path (large headers) ~61 GiB per 1M. After the fix the leak is `0` regardless of request count. (cherry picked from commit 10e24f9)
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.
Auto-port of #17089 to 5.0
Cherry-picked commit: 10e24f9
Motivation
Fixes #17088. Same class of bug as the
SslHandlerleak fixed in #17059: a header buffer is allocated, then aThrowable(typicallyOutOfMemoryError) is thrown before the buffer is handed off, leaving it unreleased. #6729 reported the exact symptom onHttpObjectEncoder.encodeHeadersback in 2017 but couldn't be reproduced on demand and was closed; the root cause was never fixed.Affected paths:
encodeInitHttpMessage()andencodeFullHttpMessage()inHttpObjectEncoder:buffromctx.alloc().buffer(...)leaks ifencodeHeaders()throws before it is added toout.writeHeadersInternal(),writePushPromise()andwriteContinuationFrames()inDefaultHttp2FrameWriter: the retainedfragment/ frame-header buffer leaks if a later allocation throws after the fragment is sliced off the header block.With pooled direct buffers this leaks off-heap memory the GC cannot reclaim, so repeated OOME on these paths ends in
OutOfDirectMemoryError/ process death.Modification
HttpObjectEncoder: guard the header buffer with a success/handed-off flag and release it in afinallyunless ownership was transferred. InencodeFullHttpMessage()the flag is set immediately beforeencodeByteBufHttpContent()so the chunked path — wherebufis already added tooutbeforeencodeChunkedHttpContent()can throw — does not double-release.DefaultHttp2FrameWriter: hoistfragmentto method scope, null it right afterctx.write(fragment, ...), and release it infinallyif non-null.writeContinuationFrames()additionally guards the reused frame-header buffer with a per-fragment flag.OutOfMemoryErroron a targeted allocation, asserting every tracked buffer reachesrefCnt() == 0after the failure.Result
No buffer leak when a
Throwableis thrown mid header encoding. The normal path is unchanged.Measured with the tracking allocator (each OOME on these paths leaks exactly one header buffer, so the leak grows linearly with the number of affected requests):
HttpObjectEncoderinit / fullrefCnt == 10writeHeaders/writePushPromiserefCnt == 10writeContinuationFrames(large headers)refCnt == 10At scale on the 256 B paths that is ~244 MiB leaked per 1M affected requests; on the CONTINUATION path (large headers) ~61 GiB per 1M. After the fix the leak is
0regardless of request count.