From d84d2f63affa4dccbab3b6767f0769f49102b53f Mon Sep 17 00:00:00 2001 From: Chen Qian Date: Tue, 20 Jul 2021 16:01:03 +0800 Subject: [PATCH] fix memory leak when handing empty content request in Http2 * ignore empty content and do not pass empty content to onData() handler * add test cases of passing empty data for BaseRequestHandle#handleContent --- .../httpserver/impl/AggregationHandle.java | 8 ++--- .../httpserver/impl/BaseRequestHandle.java | 8 +++++ .../esa/httpserver/impl/Http2Handler.java | 2 +- .../impl/BaseRequestHandleTest.java | 35 +++++++++++++++++-- 4 files changed, 44 insertions(+), 9 deletions(-) diff --git a/httpserver/src/main/java/esa/httpserver/impl/AggregationHandle.java b/httpserver/src/main/java/esa/httpserver/impl/AggregationHandle.java index 5f2d192..498aa26 100644 --- a/httpserver/src/main/java/esa/httpserver/impl/AggregationHandle.java +++ b/httpserver/src/main/java/esa/httpserver/impl/AggregationHandle.java @@ -58,12 +58,10 @@ void setTrailers(HttpHeaders trailers) { } void appendPartialContent(ByteBuf partialContent) { - if (partialContent.isReadable()) { - if (content == null) { - content = ctx.alloc().compositeBuffer(MAX_COMPOSITE_BUFFER_COMPONENTS); - } - content.addComponent(true, partialContent); + if (content == null) { + content = ctx.alloc().compositeBuffer(MAX_COMPOSITE_BUFFER_COMPONENTS); } + content.addComponent(true, partialContent); } void release() { diff --git a/httpserver/src/main/java/esa/httpserver/impl/BaseRequestHandle.java b/httpserver/src/main/java/esa/httpserver/impl/BaseRequestHandle.java index cabf2f2..39be1ff 100644 --- a/httpserver/src/main/java/esa/httpserver/impl/BaseRequestHandle.java +++ b/httpserver/src/main/java/esa/httpserver/impl/BaseRequestHandle.java @@ -230,6 +230,14 @@ public boolean isEnded() { public abstract BaseResponse response(); void handleContent(ByteBuf buf) { + // ignore unreadable data + // 1. it is no need to pass empty content to multipart decoder and aggregator + // 2.keep the onData() behavior of Http1 and Htt2 consistent, because empty content in + // LastHttpContent#EMPTY_LAST_CONTENT will be ignored in Http1Handler but will be passed in Http2Handler + if (!buf.isReadable()) { + return; + } + if (multipart != null) { multipart.onData(buf.duplicate()); } diff --git a/httpserver/src/main/java/esa/httpserver/impl/Http2Handler.java b/httpserver/src/main/java/esa/httpserver/impl/Http2Handler.java index ba46a73..079c39d 100644 --- a/httpserver/src/main/java/esa/httpserver/impl/Http2Handler.java +++ b/httpserver/src/main/java/esa/httpserver/impl/Http2Handler.java @@ -184,7 +184,7 @@ public int onDataRead(ChannelHandlerContext ctx, final int readableBytes = data.readableBytes(); // The req.bytes is logically divided to positive one, negative one and zero - // 1. The positive one presenting the chunk size left, which mean we can only read num of req.bytes data + // 1. The positive one presenting the chunk size left, which means we can only read num of req.bytes data // limited by content length, and the oversize bytes will be discarded. // 2. The negative one presenting the num of bytes that we have read, which would be limited by the // ServerOptions#maxContentLength, and the exceeded bytes will be discarded. diff --git a/httpserver/src/test/java/esa/httpserver/impl/BaseRequestHandleTest.java b/httpserver/src/test/java/esa/httpserver/impl/BaseRequestHandleTest.java index 2d3c486..c083734 100644 --- a/httpserver/src/test/java/esa/httpserver/impl/BaseRequestHandleTest.java +++ b/httpserver/src/test/java/esa/httpserver/impl/BaseRequestHandleTest.java @@ -115,9 +115,11 @@ void testSetAndCallOnDataHandler() { final Req req = plainReq(); final List bufs = new LinkedList<>(); assertSame(req, req.onData(bufs::add)); + final ByteBuf buf0 = Unpooled.copiedBuffer(new byte[0]); final ByteBuf buf1 = Unpooled.copiedBuffer(new byte[]{1, 2}); final ByteBuf buf2 = Unpooled.copiedBuffer(new byte[]{3, 4}); final ByteBuf buf3 = Unpooled.copiedBuffer(new byte[]{5, 6}); + req.handleContent(buf0); req.handleContent(buf1); req.handleContent(buf2); req.handleContent(buf3); @@ -125,6 +127,7 @@ void testSetAndCallOnDataHandler() { assertSame(buf1, bufs.get(0)); assertSame(buf2, bufs.get(1)); assertSame(buf3, bufs.get(2)); + assertEquals(1, buf0.refCnt()); assertEquals(1, buf1.refCnt()); assertEquals(1, buf2.refCnt()); assertEquals(1, buf3.refCnt()); @@ -241,6 +244,8 @@ void testMultipartExpect() throws IOException { assertTrue(req.multipart().attributes().isEmpty()); assertTrue(req.multipart().uploadFiles().isEmpty()); + final String body0 = ""; + final String body1 = "--" + boundary + "\r\n" + "Content-Disposition: form-data; name=\"file\"; filename=\"tmp-0.txt\"\r\n" + @@ -254,8 +259,13 @@ void testMultipartExpect() throws IOException { "bar\r\n" + "--" + boundary + "--\r\n"; - req.handleContent(Unpooled.copiedBuffer(body1, StandardCharsets.UTF_8)); - req.handleContent(Unpooled.copiedBuffer(body2, StandardCharsets.UTF_8)); + final ByteBuf buf0 = Unpooled.copiedBuffer(body0, StandardCharsets.UTF_8); + final ByteBuf buf1 = Unpooled.copiedBuffer(body1, StandardCharsets.UTF_8); + final ByteBuf buf2 = Unpooled.copiedBuffer(body2, StandardCharsets.UTF_8); + + req.handleContent(buf0); + req.handleContent(buf1); + req.handleContent(buf2); final CompletableFuture cf = new CompletableFuture<>(); req.onEnd(p -> { @@ -275,6 +285,9 @@ void testMultipartExpect() throws IOException { cf.complete(null); assertNull(upload.getByteBuf()); + assertEquals(1, buf0.refCnt()); + assertEquals(1, buf1.refCnt()); + assertEquals(1, buf2.refCnt()); } @Test @@ -289,6 +302,7 @@ void testMultipartExpectCleared() { assertSame(req, req.multipart(true)); assertNotSame(MultipartHandle.EMPTY, req.multipart()); + final String body0 = ""; final String body1 = "--" + boundary + "\r\n" + "Content-Disposition: form-data; name=\"file\"; filename=\"tmp-0.txt\"\r\n" + @@ -296,10 +310,18 @@ void testMultipartExpectCleared() { "\r\n" + "foo" + "\r\n" + "--" + boundary; - req.handleContent(Unpooled.copiedBuffer(body1, StandardCharsets.UTF_8)); + final ByteBuf buf0 = Unpooled.copiedBuffer(body0, StandardCharsets.UTF_8); + final ByteBuf buf1 = Unpooled.copiedBuffer(body1, StandardCharsets.UTF_8); + req.handleContent(buf0); + req.handleContent(buf1); + + assertEquals(1, buf0.refCnt()); + assertEquals(1, buf1.refCnt()); req.multipart(false); assertSame(MultipartHandle.EMPTY, req.multipart()); + assertEquals(1, buf0.refCnt()); + assertEquals(1, buf1.refCnt()); } @Test @@ -333,21 +355,28 @@ void testAggregated() { return p; }); + final ByteBuf buf0 = Unpooled.copiedBuffer("".getBytes(StandardCharsets.UTF_8)); final ByteBuf buf1 = Unpooled.copiedBuffer("123".getBytes(StandardCharsets.UTF_8)); final ByteBuf buf2 = Unpooled.copiedBuffer("456".getBytes(StandardCharsets.UTF_8)); + req.handleContent(buf0); req.handleContent(buf1); req.handleContent(buf2); + final HttpHeaders trailers = new Http1HeadersImpl(); req.handleTrailer(trailers); req.handleEnd(); assertEquals("123456", req.aggregated().body().toString(StandardCharsets.UTF_8)); assertEquals(1, req.aggregated().body().refCnt()); + + // empty content will be ignored + assertEquals(1, buf0.refCnt()); // retained assertEquals(2, buf1.refCnt()); assertEquals(2, buf2.refCnt()); assertSame(trailers, req.aggregated().trailers()); end.complete(null); + assertEquals(1, buf0.refCnt()); assertEquals(1, buf1.refCnt()); assertEquals(1, buf2.refCnt()); assertSame(AggregationHandle.EMPTY, req.aggregated());