Ensure HttpData#addContent/setContent releases the buffer before throwing IOException (#11621)

Motivation:
When the ByteBuf size exceeds the max limit/defined size, IOException is thrown.
HttpData#addContent/setContent should release the buffer in such cases otherwise memory leak will happen.

Modification:
- Release the provided ByteBuf before throwing IOException
- Add unit tests

Result:
Fixes #11618

codec-http/src/main/java/io/netty/handler/codec/http/multipart/AbstractMemoryHttpData.java

@@ -50,8 +50,14 @@ public abstract class AbstractMemoryHttpData extends AbstractHttpData {
     public void setContent(ByteBuf buffer) throws IOException {
         requireNonNull(buffer, "buffer");
         long localsize = buffer.readableBytes();
-        checkSize(localsize);
+        try {
+            checkSize(localsize);
+        } catch (IOException e) {
+            buffer.release();
+            throw e;
+        }
         if (definedSize > 0 && definedSize < localsize) {
+            buffer.release();
             throw new IOException("Out of size: " + localsize + " > " +
                     definedSize);
         }
@@ -98,8 +104,14 @@ public abstract class AbstractMemoryHttpData extends AbstractHttpData {
             throws IOException {
         if (buffer != null) {
             long localsize = buffer.readableBytes();
-            checkSize(size + localsize);
+            try {
+                checkSize(size + localsize);
+            } catch (IOException e) {
+                buffer.release();
+                throw e;
+            }
             if (definedSize > 0 && definedSize < size + localsize) {
+                buffer.release();
                 throw new IOException("Out of size: " + (size + localsize) +
                         " > " + definedSize);
             }

codec-http/src/main/java/io/netty/handler/codec/http/multipart/DiskAttribute.java

@@ -126,7 +126,12 @@ public class DiskAttribute extends AbstractDiskHttpData implements Attribute {
     @Override
     public void addContent(ByteBuf buffer, boolean last) throws IOException {
         final long newDefinedSize = size + buffer.readableBytes();
-        checkSize(newDefinedSize);
+        try {
+            checkSize(newDefinedSize);
+        } catch (IOException e) {
+            buffer.release();
+            throw e;
+        }
         if (definedSize > 0 && definedSize < newDefinedSize) {
             definedSize = newDefinedSize;
         }

codec-http/src/main/java/io/netty/handler/codec/http/multipart/HttpData.java

@@ -43,12 +43,13 @@ public interface HttpData extends InterfaceHttpData, ByteBufHolder {
 
     /**
      * Check if the new size is not reaching the max limit allowed.
-     * The limit is always computed in term of bytes.
+     * The limit is always computed in terms of bytes.
      */
     void checkSize(long newSize) throws IOException;
 
     /**
      * Set the content from the ChannelBuffer (erase any previous data)
+     * <p>{@link ByteBuf#release()} ownership of {@code buffer} is transferred to this {@link HttpData}.
      *
      * @param buffer Must be not null.
      * @throws IOException If an IO error occurs when setting the content of this HttpData.
@@ -57,6 +58,7 @@ public interface HttpData extends InterfaceHttpData, ByteBufHolder {
 
     /**
      * Add the content from the ChannelBuffer
+     * <p>{@link ByteBuf#release()} ownership of {@code buffer} is transferred to this {@link HttpData}.
      *
      * @param buffer
      *            must be not null except if last is set to False

codec-http/src/main/java/io/netty/handler/codec/http/multipart/MemoryAttribute.java

@@ -80,7 +80,12 @@ public class MemoryAttribute extends AbstractMemoryHttpData implements Attribute
     @Override
     public void addContent(ByteBuf buffer, boolean last) throws IOException {
         int localsize = buffer.readableBytes();
-        checkSize(size + localsize);
+        try {
+            checkSize(size + localsize);
+        } catch (IOException e) {
+            buffer.release();
+            throw e;
+        }
         if (definedSize > 0 && definedSize < size + localsize) {
             definedSize = size + localsize;
         }

codec-http/src/main/java/io/netty/handler/codec/http/multipart/MixedAttribute.java

@@ -122,16 +122,21 @@ public class MixedAttribute implements Attribute {
     @Override
     public void addContent(ByteBuf buffer, boolean last) throws IOException {
         if (attribute instanceof MemoryAttribute) {
-            checkSize(attribute.length() + buffer.readableBytes());
+            try {
-            if (attribute.length() + buffer.readableBytes() > limitSize) {
+                checkSize(attribute.length() + buffer.readableBytes());
-                DiskAttribute diskAttribute = new DiskAttribute(attribute
+                if (attribute.length() + buffer.readableBytes() > limitSize) {
-                        .getName(), attribute.definedLength(), baseDir, deleteOnExit);
+                    DiskAttribute diskAttribute = new DiskAttribute(attribute
-                diskAttribute.setMaxSize(maxSize);
+                            .getName(), attribute.definedLength(), baseDir, deleteOnExit);
-                if (((MemoryAttribute) attribute).getByteBuf() != null) {
+                    diskAttribute.setMaxSize(maxSize);
-                    diskAttribute.addContent(((MemoryAttribute) attribute)
+                    if (((MemoryAttribute) attribute).getByteBuf() != null) {
-                        .getByteBuf(), false);
+                        diskAttribute.addContent(((MemoryAttribute) attribute)
+                            .getByteBuf(), false);
+                    }
+                    attribute = diskAttribute;
                 }
-                attribute = diskAttribute;
+            } catch (IOException e) {
+                buffer.release();
+                throw e;
             }
         }
         attribute.addContent(buffer, last);
@@ -199,7 +204,12 @@ public class MixedAttribute implements Attribute {
 
     @Override
     public void setContent(ByteBuf buffer) throws IOException {
-        checkSize(buffer.readableBytes());
+        try {
+            checkSize(buffer.readableBytes());
+        } catch (IOException e) {
+            buffer.release();
+            throw e;
+        }
         if (buffer.readableBytes() > limitSize) {
             if (attribute instanceof MemoryAttribute) {
                 // change to Disk

codec-http/src/main/java/io/netty/handler/codec/http/multipart/MixedFileUpload.java

@@ -83,22 +83,27 @@ public class MixedFileUpload implements FileUpload {
     public void addContent(ByteBuf buffer, boolean last)
             throws IOException {
         if (fileUpload instanceof MemoryFileUpload) {
-            checkSize(fileUpload.length() + buffer.readableBytes());
+            try {
-            if (fileUpload.length() + buffer.readableBytes() > limitSize) {
+                checkSize(fileUpload.length() + buffer.readableBytes());
-                DiskFileUpload diskFileUpload = new DiskFileUpload(fileUpload
+                if (fileUpload.length() + buffer.readableBytes() > limitSize) {
-                        .getName(), fileUpload.getFilename(), fileUpload
+                    DiskFileUpload diskFileUpload = new DiskFileUpload(fileUpload
-                        .getContentType(), fileUpload
+                            .getName(), fileUpload.getFilename(), fileUpload
-                        .getContentTransferEncoding(), fileUpload.getCharset(),
+                            .getContentType(), fileUpload
-                        definedSize, baseDir, deleteOnExit);
+                            .getContentTransferEncoding(), fileUpload.getCharset(),
-                diskFileUpload.setMaxSize(maxSize);
+                            definedSize, baseDir, deleteOnExit);
-                ByteBuf data = fileUpload.getByteBuf();
+                    diskFileUpload.setMaxSize(maxSize);
-                if (data != null && data.isReadable()) {
+                    ByteBuf data = fileUpload.getByteBuf();
-                    diskFileUpload.addContent(data.retain(), false);
+                    if (data != null && data.isReadable()) {
-                }
+                        diskFileUpload.addContent(data.retain(), false);
-                // release old upload
+                    }
-                fileUpload.release();
+                    // release old upload
+                    fileUpload.release();
 
-                fileUpload = diskFileUpload;
+                    fileUpload = diskFileUpload;
+                }
+            } catch (IOException e) {
+                buffer.release();
+                throw e;
             }
         }
         fileUpload.addContent(buffer, last);
@@ -181,7 +186,12 @@ public class MixedFileUpload implements FileUpload {
 
     @Override
     public void setContent(ByteBuf buffer) throws IOException {
-        checkSize(buffer.readableBytes());
+        try {
+            checkSize(buffer.readableBytes());
+        } catch (IOException e) {
+            buffer.release();
+            throw e;
+        }
         if (buffer.readableBytes() > limitSize) {
             if (fileUpload instanceof MemoryFileUpload) {
                 FileUpload memoryUpload = fileUpload;

codec-http/src/test/java/io/netty/handler/codec/http/multipart/HttpDataTest.java

@@ -0,0 +1,135 @@
+/*
+ * Copyright 2021 The Netty Project
+ *
+ * The Netty Project licenses this file to you under the Apache License,
+ * version 2.0 (the "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at:
+ *
+ *   https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ */
+package io.netty.handler.codec.http.multipart;
+
+import io.netty.buffer.ByteBuf;
+import io.netty.buffer.PooledByteBufAllocator;
+import io.netty.util.CharsetUtil;
+import org.assertj.core.api.ThrowableAssert;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.MethodSource;
+
+import java.io.IOException;
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+import java.util.Random;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
+
+class HttpDataTest {
+    private static final byte[] BYTES = new byte[64];
+
+    @Retention(RetentionPolicy.RUNTIME)
+    @Target(ElementType.METHOD)
+    @ParameterizedTest(name = "{displayName}({0})")
+    @MethodSource("data")
+    @interface ParameterizedHttpDataTest {
+    }
+
+    static HttpData[] data() {
+        return new HttpData[]{
+                new MemoryAttribute("test", 10),
+                new MemoryFileUpload("test", "", "text/plain", null, CharsetUtil.UTF_8, 10),
+                new MixedAttribute("test", 10, -1),
+                new MixedFileUpload("test", "", "text/plain", null, CharsetUtil.UTF_8, 10, -1),
+                new DiskAttribute("test", 10),
+                new DiskFileUpload("test", "", "text/plain", null, CharsetUtil.UTF_8, 10)
+        };
+    }
+
+    @BeforeAll
+    static void setUp() {
+        Random rndm = new Random();
+        rndm.nextBytes(BYTES);
+    }
+
+    @ParameterizedHttpDataTest
+    void testAddContentEmptyBuffer(HttpData httpData) throws IOException {
+        ByteBuf content = PooledByteBufAllocator.DEFAULT.buffer();
+        httpData.addContent(content, false);
+        assertThat(content.refCnt()).isEqualTo(0);
+    }
+
+    @Test
+    void testAddContentExceedsDefinedSizeDiskFileUpload() {
+        doTestAddContentExceedsSize(
+                new DiskFileUpload("test", "", "application/json", null, CharsetUtil.UTF_8, 10),
+                "Out of size: 64 > 10");
+    }
+
+    @Test
+    void testAddContentExceedsDefinedSizeMemoryFileUpload() {
+        doTestAddContentExceedsSize(
+                new MemoryFileUpload("test", "", "application/json", null, CharsetUtil.UTF_8, 10),
+                "Out of size: 64 > 10");
+    }
+
+    @ParameterizedHttpDataTest
+    void testAddContentExceedsMaxSize(final HttpData httpData) {
+        httpData.setMaxSize(10);
+        doTestAddContentExceedsSize(httpData, "Size exceed allowed maximum capacity");
+    }
+
+    @ParameterizedHttpDataTest
+    void testSetContentExceedsDefinedSize(final HttpData httpData) {
+        doTestSetContentExceedsSize(httpData, "Out of size: 64 > 10");
+    }
+
+    @ParameterizedHttpDataTest
+    void testSetContentExceedsMaxSize(final HttpData httpData) {
+        httpData.setMaxSize(10);
+        doTestSetContentExceedsSize(httpData, "Size exceed allowed maximum capacity");
+    }
+
+    private static void doTestAddContentExceedsSize(final HttpData httpData, String expectedMessage) {
+        final ByteBuf content = PooledByteBufAllocator.DEFAULT.buffer();
+        content.writeBytes(BYTES);
+
+        assertThatExceptionOfType(IOException.class)
+                .isThrownBy(new ThrowableAssert.ThrowingCallable() {
+
+                    @Override
+                    public void call() throws Throwable {
+                        httpData.addContent(content, false);
+                    }
+                })
+                .withMessage(expectedMessage);
+
+        assertThat(content.refCnt()).isEqualTo(0);
+    }
+
+    private static void doTestSetContentExceedsSize(final HttpData httpData, String expectedMessage) {
+        final ByteBuf content = PooledByteBufAllocator.DEFAULT.buffer();
+        content.writeBytes(BYTES);
+
+        assertThatExceptionOfType(IOException.class)
+                .isThrownBy(new ThrowableAssert.ThrowingCallable() {
+
+                    @Override
+                    public void call() throws Throwable {
+                        httpData.setContent(content);
+                    }
+                })
+                .withMessage(expectedMessage);
+
+        assertThat(content.refCnt()).isEqualTo(0);
+    }
+}