Fix byte buf leak when a `ContentTooLargeException` is raised #5227

minwoox · 2023-10-08T12:16:56Z

Motivation:
There are three bugs when the size of a request exceeds the max content length:

The ResponseHeaders that are sent to the client and recorded to RequestLog are different.
The AggregatingDecodedHttpRequest is not aborted thus ByteBufs are leaked. (When HTTP/1.1 is used)
The AggregatingDecodedHttpRequest does not get through the decorators so LoggingService doesn't log the exception.

Modifications:

Send 413 responses using HttpResponseSubscriber instead of sending them from HttpRequestDecoder by aborting the response.
- 413 is not a protocol error so we should send it from the HttpResponseSubscriber.
- If a responseHeaders is already sent, the stream is reset. (the channel is closed if HTTP/1.1)
Call ctx.fireChannelRead() if the request wasn't handed over to HttpServerHandler.
- This lets the LoggingService log the ContentTooLargeException.
- AggregatingDecodedHttpRequest is converted to StreamingDecodedHttpRequest not to raise an aggregate exception.
(misc) Raise a stream error instead of a connection error if it is stream level.

Result:

Fix When grpc streams requests over maxRequestLength, the request may not be handled #3803, LEAK: ByteBuf.release() was not called before it's garbage-collected using verison 1.25.2 #5180
No more leaks when the size of a request exceeds the max content length.

Motivation: There are three bugs when the size of a request exceeds the max content length: - The `ResponseHeaders` that is sent to the client and is recorded to `RequestLog` are different. - The `AggregatingDecodedHttpRequest` is not aborted thus `ByteBuf`s are leaked. (When HTTP/1.1 is used) - The `AggregatingDecodedHttpRequest` does not get throught the decorators so `LoggingService` doesn't log the exception. Modifications: - Send 413 response using `HttpResponseSubscriber` so that the recoreded `ResponseHaders` is actually sent - Abort `AggregatingDecodedHttpRequest` if a `ContentTooLargeException` is raised when HTTP/1.1 is used. - Convert `AggregatingDecodedHttpRequest` to `StreamingDecodedHttpRequest` and pass it to the service chain. - Raise an stream error instead of connection error if it is stream level. Result: - Fix line#5180 - No more leaks when the size of a request exceeds the max content length.

codecov · 2023-10-08T12:48:53Z

Codecov Report

Attention: 30 lines in your changes are missing coverage. Please review.

Comparison is base (25926dc) 73.94% compared to head (368b428) 73.94%.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #5227      +/-   ##
============================================
- Coverage     73.94%   73.94%   -0.01%     
- Complexity    20072    20085      +13     
============================================
  Files          1728     1728              
  Lines         74040    74092      +52     
  Branches       9438     9451      +13     
============================================
+ Hits          54752    54785      +33     
- Misses        14829    14833       +4     
- Partials       4459     4474      +15

Files	Coverage Δ
...orp/armeria/client/AbstractHttpRequestHandler.java	`80.00% <100.00%> (ø)`
...p/armeria/common/stream/DeferredStreamMessage.java	`84.29% <100.00%> (+2.28%)`	⬆️
...a/com/linecorp/armeria/common/util/Exceptions.java	`40.68% <ø> (-2.07%)`	⬇️
...rp/armeria/internal/common/Http1ObjectEncoder.java	`71.42% <ø> (-1.72%)`	⬇️
...orp/armeria/internal/common/HttpObjectEncoder.java	`63.63% <100.00%> (+9.09%)`	⬆️
...armeria/server/AbstractHttpResponseSubscriber.java	`79.83% <100.00%> (+0.59%)`	⬆️
...p/armeria/server/Http2ServerConnectionHandler.java	`87.14% <100.00%> (-0.36%)`	⬇️
.../internal/common/DefaultCancellationScheduler.java	`77.16% <50.00%> (-0.22%)`	⬇️
...rp/armeria/internal/common/Http2ObjectEncoder.java	`71.42% <50.00%> (-16.81%)`	⬇️
.../armeria/server/AggregatedHttpResponseHandler.java	`82.27% <80.00%> (-7.47%)`	⬇️
... and 9 more

... and 10 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

github-actions · 2023-10-08T13:35:43Z

🔍 Build Scan® (commit: `368b428`)

Job name	Status	Build Scan®
build-windows-latest-jdk-19	❌ (failure)	https://ge.armeria.dev/s/wof6rkatuxhyo
build-self-hosted-jdk-8	✅	https://ge.armeria.dev/s/lci7pzyor6dg4
build-self-hosted-jdk-19-snapshot-blockhound	✅	https://ge.armeria.dev/s/aruw2gm7dizt2
build-self-hosted-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/rv7f5yyueemw6
build-self-hosted-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/xdh46l37vocmg
build-self-hosted-jdk-17-leak	✅	https://ge.armeria.dev/s/zftrq6q2wqtqm
build-self-hosted-jdk-11	✅	https://ge.armeria.dev/s/b7p6jc4qnhvdy
build-macos-12-jdk-19	✅	https://ge.armeria.dev/s/r5ev2nrxri72s

jrhee17

not to raise an aggregate exception.

Do you mind explaining what points raised an aggregate exception?

Also, I think it will take too long for me to thoroughly review this PR.
Feel free to merge without my approval 🤞

core/src/main/java/com/linecorp/armeria/internal/common/stream/SubscriberUtil.java

jrhee17 · 2023-10-17T08:43:17Z

core/src/main/java/com/linecorp/armeria/server/Http2RequestDecoder.java

+                if (!req.isInitialized()) {
+                    assert req.needsAggregation();


Regardless of whether initialized or not, I think semantically it makes more sense that we fire the channel if the request isn't aggregated.

Also, if we happen to modify the pipeline later which reschedules the ctx.fireChannelRead(req); downstream, I feel like this can be a potential bug very easily

ditto for the other occurrences

Suggested change

if (!req.isInitialized()) {

assert req.needsAggregation();

if (req.needsAggregation()) {

assert !req.isInitialized();

My original fix was calling ctx.fireChannelRead(req); with the aggregating request itself (without converting to stream request) so I need to check if the fireChannelRead method is called or not using isInitialized(). But I've changed the logic so let me revert it. 😉

AggregatingDecodedHttpRequest is converted to StreamingDecodedHttpRequest not to raise an aggregate exception.

If the request is incomplete, wouldn't be clear to raise an exception when being aggregated?

AggregatingDecodedHttpRequest was implemented on the assumption that it is aggregated after being closed.
Do you mean we need to fix AggregatingDecodedHttpRequest not to raise an exception when aggregated?

core/src/main/java/com/linecorp/armeria/server/Http2RequestDecoder.java

core/src/main/java/com/linecorp/armeria/server/AggregatedHttpResponseHandler.java

jrhee17 · 2023-10-17T09:34:46Z

core/src/main/java/com/linecorp/armeria/server/Http2RequestDecoder.java

        final ClosedStreamException cause =
                new ClosedStreamException("received a RST_STREAM frame: " + Http2Error.valueOf(errorCode));
-        req.abortResponse(cause, /* cancel */ true);
+        if (req.needsAggregation() && !req.isInitialized()) {


Suggested change

if (req.needsAggregation() && !req.isInitialized()) {

if (req.needsAggregation()) {

assert !req.isInitialized();

This isn't true because the server can receive an RST_STREAM after the aggregating request is initialized.

request is initialized.

Q: Initialization seems more important to call req.toAbortedStreaming(). What do you think of using req.isInitialized() to check whether a call to req.toAbortedStreaming() is necessary for here and L335?

if (!req.isInitialized()) { ctx.fireChannelRead(req.toAbortedStreaming(inboundTrafficController, cause, false)); }

That's a good suggestion. Let me change it back. 😉

The original reason for my suggestion was because I wanted to do some testing on rescheduling the layer between channel and service event loops.

Let me just handle this separately then if needed

core/src/main/java/com/linecorp/armeria/server/Http2RequestDecoder.java

minwoox · 2023-10-17T11:19:56Z

not to raise an aggregate exception.
Do you mind explaining what points raised an aggregate exception?

An AggregatingDecodedHttpRequest must be closed to be subscribed:

armeria/core/src/main/java/com/linecorp/armeria/internal/common/stream/AggregatingStreamMessage.java

Line 94 in 871d872

    
           assert closed : getClass().getSimpleName() + " should be closed before publishing items";

However, we can just close it without fully receiving the request body.

Also, I think it will take too long for me to thoroughly review this PR.

Oh, please let me know if there're any points that you don't understand. 😉

core/src/main/java/com/linecorp/armeria/common/util/Exceptions.java

core/src/main/java/com/linecorp/armeria/server/AggregatedHttpResponseHandler.java

core/src/main/java/com/linecorp/armeria/server/DecodedHttpRequest.java

core/src/main/java/com/linecorp/armeria/server/Http2RequestDecoder.java

core/src/main/java/com/linecorp/armeria/server/StreamingDecodedHttpRequest.java

trustin

👍

ikhoon

Overall looks good.

core/src/main/java/com/linecorp/armeria/server/Http2RequestDecoder.java

ikhoon · 2023-10-20T13:43:53Z

core/src/main/java/com/linecorp/armeria/server/Http2RequestDecoder.java

        final ClosedStreamException cause =
                new ClosedStreamException("received a RST_STREAM frame: " + Http2Error.valueOf(errorCode));
-        req.abortResponse(cause, /* cancel */ true);
+        if (req.needsAggregation() && !req.isInitialized()) {


request is initialized.

Q: Initialization seems more important to call req.toAbortedStreaming(). What do you think of using req.isInitialized() to check whether a call to req.toAbortedStreaming() is necessary for here and L335?

if (!req.isInitialized()) { ctx.fireChannelRead(req.toAbortedStreaming(inboundTrafficController, cause, false)); }

ikhoon · 2023-10-23T09:26:17Z

core/src/main/java/com/linecorp/armeria/internal/common/Http2ObjectEncoder.java

+                                            boolean sendResetIfRemoteIsOpen) {
        final Http2Stream stream = encoder.connection().stream(streamId);
+
+        if (sendResetIfRemoteIsOpen && !stream.state().localSideOpen()) {


Q: Why is the local side open checked? sendResetIfRemoteIsOpen says reset if the remote is open.

I copied it from here and I think I also need to fix it. 😓
Let me run the CI a few more times.

core/src/main/java/com/linecorp/armeria/server/AbstractHttpResponseSubscriber.java

ikhoon · 2023-10-24T07:45:37Z

core/src/main/java/com/linecorp/armeria/internal/common/Http2ObjectEncoder.java

        // Send a RST_STREAM frame only for an active stream which did not send a RST_STREAM frame already.
        if (stream != null && !stream.isResetSent()) {
-            return encoder.writeRstStream(ctx, streamId, error.code(), ctx.newPromise());
+            if (!sendResetIfRemoteIsOpen || stream.state().remoteSideOpen()) {


Q: Do we need to send RST_FRAME when half-close on remote? An EOF for the stream has been received from the remote peer already.

Change not to send RST if EOF is received. 😉

ikhoon

Thanks for handling this non-trivial issue. 🙇‍♂️🙇‍♂️

jrhee17

Only left a minor question but looks good 👍 Thanks @minwoox 🙇 👍 🙇

jrhee17 · 2023-10-25T08:19:28Z

core/src/main/java/com/linecorp/armeria/server/Http1RequestDecoder.java

                    }
                    decodedReq.close();
                    if (decodedReq.needsAggregation()) {
+                        assert !decodedReq.isInitialized();


Looks like this can be removed

Suggested change

assert !decodedReq.isInitialized();

Yes, but let's leave it as is if you don't mind. 😉

core/src/main/java/com/linecorp/armeria/server/Http2RequestDecoder.java

jrhee17 · 2023-10-25T08:33:25Z

core/src/main/java/com/linecorp/armeria/server/Http2RequestDecoder.java

        final ClosedStreamException cause =
                new ClosedStreamException("received a RST_STREAM frame: " + Http2Error.valueOf(errorCode));
-        req.abortResponse(cause, /* cancel */ true);
+        if (req.needsAggregation() && !req.isInitialized()) {


The original reason for my suggestion was because I wanted to do some testing on rescheduling the layer between channel and service event loops.

Let me just handle this separately then if needed

jrhee17 · 2023-10-25T08:50:29Z

core/src/main/java/com/linecorp/armeria/server/Http1RequestDecoder.java

+                            assert decodedReq.needsAggregation();
+                            final StreamingDecodedHttpRequest streamingReq = decodedReq.toAbortedStreaming(
+                                    inboundTrafficController, httpStatusException, shouldReset);
+                            ctx.fireChannelRead(streamingReq);


Question) It seems like we don't early return when encoder is swapped to http2 and an upgrade request is received.

Is there a code point where we guard against ctx.fireChannelRead being invoked twice? (once for HTTP1 upgrade, once for HTTP2)

Sure. 😉
An UpgradeEvent is created when it receives the upgrade event:

armeria/core/src/main/java/com/linecorp/armeria/server/HttpServerUpgradeHandler.java

Line 297 in b7c14c4

final UpgradeEvent event = new UpgradeEvent(request);

Http2ConnectionHandler is added to the pipleline:

armeria/core/src/main/java/com/linecorp/armeria/server/Http2ServerUpgradeCodec.java

Line 109 in e7be74e

ctx.pipeline().addAfter(ctx.name(), null, connectionHandler);

Http1RequestDecoder receives the UpgradeEvent and calls channelRead() with the HTTP/1.1 request:

armeria/core/src/main/java/com/linecorp/armeria/server/Http1RequestDecoder.java

Line 453 in 091d193

channelRead(ctx, nettyReq);

After handling the request, Http1RequestDecoder is removed:

armeria/core/src/main/java/com/linecorp/armeria/server/Http1RequestDecoder.java

Lines 275 to 279 in 091d193

final boolean endOfStream = msg instanceof LastHttpContent;

if (endOfStream && encoder instanceof ServerHttp2ObjectEncoder) {

// An HTTP/1 connection has been upgraded to HTTP/2.

ctx.pipeline().remove(this);

}

It seems like we don't early return

My question was rather when ctx.pipeline().remove(this); is called, but the content length of the request is exceeded.

Let me do some testing in my local env. instead and follow up if needed, feel free to merge 😄

I misunderstood it. 😓
No, I didn't consider that case and I have to fix it. Thanks!

minwoox · 2023-10-26T06:01:32Z

Thanks all for the review. 😉

) Motivation: There are three bugs when the size of a request exceeds the max content length: - The `ResponseHeaders` that are sent to the client and recorded to `RequestLog` are different. - The `AggregatingDecodedHttpRequest` is not aborted thus `ByteBuf`s are leaked. (When HTTP/1.1 is used) - The `AggregatingDecodedHttpRequest` does not get through the decorators so `LoggingService` doesn't log the exception. Modifications: - Send 413 responses using `HttpResponseSubscriber` instead of sending them from `HttpRequestDecoder` by aborting the response. - 413 is not a protocol error so we should send it from the `HttpResponseSubscriber`. - If a responseHeaders is already sent, the stream is reset. (the channel is closed if HTTP/1.1) - Call `ctx.fireChannelRead()` if the request wasn't handed over to `HttpServerHandler`. - This lets the `LoggingService` log the `ContentTooLargeException`. - `AggregatingDecodedHttpRequest` is converted to `StreamingDecodedHttpRequest` not to raise an aggregate exception. - (misc) Raise a stream error instead of a connection error if it is stream level. Result: - Fix line#3803, line#5180 - No more leaks when the size of a request exceeds the max content length.

minwoox requested review from ikhoon, jrhee17 and trustin as code owners October 8, 2023 12:16

minwoox added the defect label Oct 8, 2023

minwoox added this to the 1.26.0 milestone Oct 8, 2023

minwoox mentioned this pull request Oct 8, 2023

LEAK: ByteBuf.release() was not called before it's garbage-collected using verison 1.25.2 #5180

Closed

minwoox added 3 commits October 9, 2023 10:48

Send as much as needs

9423e86

Add more tests

9515738

Use await

6d83b06

minwoox mentioned this pull request Oct 11, 2023

ServerErrorHandler behavior is not well defined when maxRequestLength is exceeded #4821

Closed

Always abort response if the size of a request exceeds the max

c0f206f

jrhee17 reviewed Oct 17, 2023

View reviewed changes

minwoox added 2 commits October 17, 2023 20:29

Address comments from @jrhee17

86d6569

Merge branch 'main' into leak_contentTooLarge

02cfb01

trustin reviewed Oct 20, 2023

View reviewed changes

Address comments from @trustin

275d256

trustin approved these changes Oct 20, 2023

View reviewed changes

minwoox added 2 commits October 20, 2023 19:40

Fix

5451d68

Fix

fc6f128

ikhoon reviewed Oct 20, 2023

View reviewed changes

minwoox added 2 commits October 23, 2023 15:41

Address comments from @ikhoon

54949d8

Reset only if remote is open

173de47

ikhoon reviewed Oct 23, 2023

View reviewed changes

core/src/main/java/com/linecorp/armeria/server/AbstractHttpResponseSubscriber.java Outdated Show resolved Hide resolved

minwoox added 2 commits October 23, 2023 20:59

Use remoteSideOpen

66da21d

Fix test

b8dc970

Fix

fc8126e

ikhoon reviewed Oct 24, 2023

View reviewed changes

minwoox added 2 commits October 24, 2023 17:39

Reset only for HTTP/2

091d193

Remove unused fields

8755481

ikhoon approved these changes Oct 24, 2023

View reviewed changes

minwoox mentioned this pull request Oct 25, 2023

Test failure: com.linecorp.armeria.server.FallbackServiceTest.[1] protocol=H1C #5044

Open

jrhee17 approved these changes Oct 25, 2023

View reviewed changes

minwoox and others added 2 commits October 25, 2023 22:45

Address the comment from @jrhee17

d57ebbc

Merge branch 'main' into leak_contentTooLarge

368b428

minwoox merged commit d398661 into line:main Oct 26, 2023

minwoox deleted the leak_contentTooLarge branch October 26, 2023 06:01

	if (req.needsAggregation() && !req.isInitialized()) {
	if (req.needsAggregation()) {
	assert !req.isInitialized();

	final boolean endOfStream = msg instanceof LastHttpContent;
	if (endOfStream && encoder instanceof ServerHttp2ObjectEncoder) {
	// An HTTP/1 connection has been upgraded to HTTP/2.
	ctx.pipeline().remove(this);
	}

Fix byte buf leak when a ContentTooLargeException is raised #5227

Fix byte buf leak when a ContentTooLargeException is raised #5227

Uh oh!

Conversation

minwoox commented Oct 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Oct 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot commented Oct 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔍 Build Scan® (commit: 368b428)

Uh oh!

jrhee17 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ikhoon Oct 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

minwoox commented Oct 17, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

trustin left a comment

Choose a reason for hiding this comment

Uh oh!

ikhoon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ikhoon Oct 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

minwoox Oct 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ikhoon left a comment

Choose a reason for hiding this comment

Uh oh!

jrhee17 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Fix byte buf leak when a `ContentTooLargeException` is raised #5227

Fix byte buf leak when a `ContentTooLargeException` is raised #5227

minwoox commented Oct 8, 2023 •

edited

Loading

codecov bot commented Oct 8, 2023 •

edited

Loading

github-actions bot commented Oct 8, 2023 •

edited

Loading

🔍 Build Scan® (commit: `368b428`)

ikhoon Oct 20, 2023 •

edited

Loading

ikhoon Oct 20, 2023 •

edited

Loading

minwoox Oct 23, 2023 •

edited

Loading

jrhee17 Oct 25, 2023 •

edited

Loading