Skip to content

Commit

Permalink
Ensure HttpData#addContent/setContent releases the buffer before thro…
Browse files Browse the repository at this point in the history
…wing IOException (netty#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 netty#11618
  • Loading branch information
violetagg authored and laosijikaichele committed Dec 16, 2021
1 parent 86ac45d commit 99552cf
Show file tree
Hide file tree
Showing 7 changed files with 210 additions and 31 deletions.
Expand Up @@ -50,8 +50,14 @@ protected AbstractMemoryHttpData(String name, Charset charset, long size) {
public void setContent(ByteBuf buffer) throws IOException {
ObjectUtil.checkNotNull(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);
}
Expand Down Expand Up @@ -99,8 +105,14 @@ public void addContent(ByteBuf buffer, boolean last)
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);
}
Expand Down
Expand Up @@ -126,7 +126,12 @@ public void setValue(String value) throws IOException {
@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;
}
Expand Down
Expand Up @@ -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
Expand All @@ -58,6 +59,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
Expand Down
Expand Up @@ -80,7 +80,12 @@ public void setValue(String value) throws IOException {
@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;
}
Expand Down
Expand Up @@ -122,16 +122,21 @@ public void checkSize(long newSize) throws IOException {
@Override
public void addContent(ByteBuf buffer, boolean last) throws IOException {
if (attribute instanceof MemoryAttribute) {
checkSize(attribute.length() + buffer.readableBytes());
if (attribute.length() + buffer.readableBytes() > limitSize) {
DiskAttribute diskAttribute = new DiskAttribute(attribute
.getName(), attribute.definedLength(), baseDir, deleteOnExit);
diskAttribute.setMaxSize(maxSize);
if (((MemoryAttribute) attribute).getByteBuf() != null) {
diskAttribute.addContent(((MemoryAttribute) attribute)
.getByteBuf(), false);
try {
checkSize(attribute.length() + buffer.readableBytes());
if (attribute.length() + buffer.readableBytes() > limitSize) {
DiskAttribute diskAttribute = new DiskAttribute(attribute
.getName(), attribute.definedLength(), baseDir, deleteOnExit);
diskAttribute.setMaxSize(maxSize);
if (((MemoryAttribute) attribute).getByteBuf() != null) {
diskAttribute.addContent(((MemoryAttribute) attribute)
.getByteBuf(), false);
}
attribute = diskAttribute;
}
attribute = diskAttribute;
} catch (IOException e) {
buffer.release();
throw e;
}
}
attribute.addContent(buffer, last);
Expand Down Expand Up @@ -199,7 +204,12 @@ public void setCharset(Charset charset) {

@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
Expand Down
Expand Up @@ -83,22 +83,27 @@ public void checkSize(long newSize) throws IOException {
public void addContent(ByteBuf buffer, boolean last)
throws IOException {
if (fileUpload instanceof MemoryFileUpload) {
checkSize(fileUpload.length() + buffer.readableBytes());
if (fileUpload.length() + buffer.readableBytes() > limitSize) {
DiskFileUpload diskFileUpload = new DiskFileUpload(fileUpload
.getName(), fileUpload.getFilename(), fileUpload
.getContentType(), fileUpload
.getContentTransferEncoding(), fileUpload.getCharset(),
definedSize, baseDir, deleteOnExit);
diskFileUpload.setMaxSize(maxSize);
ByteBuf data = fileUpload.getByteBuf();
if (data != null && data.isReadable()) {
diskFileUpload.addContent(data.retain(), false);
try {
checkSize(fileUpload.length() + buffer.readableBytes());
if (fileUpload.length() + buffer.readableBytes() > limitSize) {
DiskFileUpload diskFileUpload = new DiskFileUpload(fileUpload
.getName(), fileUpload.getFilename(), fileUpload
.getContentType(), fileUpload
.getContentTransferEncoding(), fileUpload.getCharset(),
definedSize, baseDir, deleteOnExit);
diskFileUpload.setMaxSize(maxSize);
ByteBuf data = fileUpload.getByteBuf();
if (data != null && data.isReadable()) {
diskFileUpload.addContent(data.retain(), false);
}
// release old upload
fileUpload.release();

fileUpload = diskFileUpload;
}
// release old upload
fileUpload.release();

fileUpload = diskFileUpload;
} catch (IOException e) {
buffer.release();
throw e;
}
}
fileUpload.addContent(buffer, last);
Expand Down Expand Up @@ -181,7 +186,12 @@ public void setCharset(Charset charset) {

@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;
Expand Down
@@ -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);
}
}

0 comments on commit 99552cf

Please sign in to comment.