Skip to content

Commit

Permalink
Fix buffer leak in HttpObjectEncoder when message has no content (#12828
Browse files Browse the repository at this point in the history
)

Motivation:

When the _HttpObjectEncoder.encodeAndClose_ method encodes a response which content is _always empty_ (as described in [_HttpObjectEncoder. isContentAlwaysEmpty](https://github.com/netty/netty/blob/main/codec-http/src/main/java/io/netty5/handler/codec/http/HttpObjectEncoder.java#L240) ), then the empty last http content or the empty FullHttpResponse is not closed.

An http response content is always empty for example when status code is INFORMATIONAL, is a 204, 205, 304 ... [See for example here](https://github.com/netty/netty/blob/main/codec-http/src/main/java/io/netty5/handler/codec/http/HttpResponseEncoder.java#L67)

Example scenario: 

1) a _DefaultHttpResponse_ is first encoded with status 204, in this case, the _HttpObjectEncoder.encodeAndClose_ method will initialize its state to **ST_CONTENT_ALWAYS_EMPTY**, [see here](https://github.com/netty/netty/blob/main/codec-http/src/main/java/io/netty5/handler/codec/http/HttpObjectEncoder.java#L96).

2) next, an _EmptyLastHttpContent_ is encoded. So, the _HttpObjectEncoder.encodeAndClose_ state being in _ST_CONTENT_ALWAYS_EMPTY_, then the encodeAndClose method switch statement will proceed [here](https://github.com/netty/netty/blob/main/codec-http/src/main/java/io/netty5/handler/codec/http/HttpObjectEncoder.java#L149) directly, which is not closing the _EmptyLastHttpContent_

Modification:

Move the code which was disposing the message [from here](https://github.com/netty/netty/blob/main/codec-http/src/main/java/io/netty5/handler/codec/http/HttpObjectEncoder.java#L144) to the case on ST_CONTENT_ALWAYS_EMPTY [here](https://github.com/netty/netty/blob/main/codec-http/src/main/java/io/netty5/handler/codec/http/HttpObjectEncoder.java#L150).
Also added some test cases in _HttpResponseEncoderTest_.

Result:

Disposing the message to the case on _ST_CONTENT_ALWAYS_EMPTY_ will continue to ensure that the message is closed when the state is _ST_CONTENT_NON_CHUNK_ and when content length is zero (because the case fall-through, [see here](https://github.com/netty/netty/blob/main/codec-http/src/main/java/io/netty5/handler/codec/http/HttpObjectEncoder.java#L145)), and also ensures that any HttpContent is closed when state is _ST_CONTENT_ALWAYS_EMPTY_.
  • Loading branch information
pderop committed Sep 21, 2022
1 parent 57f472a commit 50b1efb
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -141,13 +141,13 @@ protected void encodeAndClose(ChannelHandlerContext ctx, Object msg, List<Object

break;
} else {
Resource.dispose(msg);
// do not break, let's fall-through
}

// fall-through!
case ST_CONTENT_ALWAYS_EMPTY:

Resource.dispose(msg);
if (buf != null) {
// We allocated a buffer so add it now.
out.add(buf);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -396,4 +396,32 @@ private static void testStatusResetContentTransferContentLength0(CharSequence he
assertEquals(responseText, written.toString());
assertFalse(channel.finish());
}

@Test
void testResponseNoContentWithEmptyLastContentClosed() {
testResponseNoContentWithLastContentClosed(new EmptyLastHttpContent(preferredAllocator()));
}

@Test
void testResponseNoContentWithDefaultLastHttpContentClosed() {
testResponseNoContentWithLastContentClosed(new DefaultLastHttpContent(preferredAllocator().allocate(0)));
}

private void testResponseNoContentWithLastContentClosed(LastHttpContent<?> lastHttpContent) {
EmbeddedChannel channel = new EmbeddedChannel(new HttpResponseEncoder());
HttpResponse response = new DefaultHttpResponse(HttpVersion.HTTP_1_1, HttpResponseStatus.NO_CONTENT);
assertTrue(channel.writeOutbound(response, lastHttpContent));
assertTrue(channel.finishAndReleaseAll());
assertFalse(lastHttpContent.isAccessible());
}

@Test
void testFullResponseNoContentClosed() {
EmbeddedChannel channel = new EmbeddedChannel(new HttpResponseEncoder());
FullHttpResponse response = new DefaultFullHttpResponse(HttpVersion.HTTP_1_1, HttpResponseStatus.NO_CONTENT,
preferredAllocator().allocate(0));
assertTrue(channel.writeOutbound(response));
assertTrue(channel.finishAndReleaseAll());
assertFalse(response.isAccessible());
}
}

0 comments on commit 50b1efb

Please sign in to comment.