Repository: netty/netty (Java)

Issue:
FileUpload.get() created by a HttpPostMultipartRequestDecoder returns incomplete array on 4.1.51
### Expected behavior

UploadFile.get() should return the full byte array uploaded.

### Actual behavior

It appears that although `fileUpload.length()` returns the correct value, the byte array from `get()` returns only a partial array.

### Steps to reproduce

Create a `HttpPostMultipartRequestDecoder` and call offer at least twice with some request form data. The test below passes on 4.1.50 but fails on 4.1.51.

### Minimal yet complete reproducer code (or URL to code)

````java
@Test
public void fileUploadRegression() throws IOException {
    byte[] file = new byte[32000];
    Arrays.fill(file, (byte) 1);

    String body = "--861fbeab-cd20-470c-9609-d40a0f704466\n" +
        "Content-Disposition: form-data; name=\"image\"; filename=\"guangzhou.jpeg\"\n" +
        "Content-Type: image/jpeg\n" +
        "Content-Length: " + file.length + "\n" +
        "\n" + new String(file, UTF_8) + "\n" +
        "--861fbeab-cd20-470c-9609-d40a0f704466--\n";
    byte[] bodyBytes = body.getBytes(UTF_8);

    HttpRequest request = new DefaultHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.POST, "/upload");
    request.headers().set("content-type", "multipart/form-data; boundary=861fbeab-cd20-470c-9609-d40a0f704466");
    request.headers().set("content-length", bodyBytes.length);
    HttpPostMultipartRequestDecoder decoder = new HttpPostMultipartRequestDecoder(request);
    decoder.offer(new DefaultHttpContent(Unpooled.copiedBuffer(bodyBytes, 0, 8192)));
    decoder.offer(new DefaultHttpContent(Unpooled.copiedBuffer(bodyBytes, 8192, bodyBytes.length - 8192)));
    decoder.offer(new DefaultLastHttpContent());
    FileUpload data = (FileUpload) decoder.getBodyHttpDatas().get(0);
    assertThat(data.length(), is((long)file.length)); // passes
    assertThat(data.get().length, is(file.length)); // fails on 4.1.51
}
````

### Netty version

4.1.51

### JVM version (e.g. `java -version`)

Multiple versions tested (8, 10, 11, 12). 

### OS version (e.g. `uname -a`)

Windows, Linux.
Fix memory leak in AbstractDiskHttpData when CompositeByteBuf is used
Motivation:

AbstractDiskHttpData may cause a memory leak when a CompositeByteBuf is used. This happened because we may call copy() but actually never release the newly created ByteBuf.

Modifications:

- Remove copy() call and just use ByteBuf.getBytes(...) which will internally handle the writing to the FileChannel without any extra copies that need to be released later on.
- Add unit test

Result:

Fixes https://github.com/netty/netty/issues/10354
FileUpload.get() created by a HttpPostMultipartRequestDecoder returns incomplete array on 4.1.51
### Expected behavior

UploadFile.get() should return the full byte array uploaded.

### Actual behavior

It appears that although `fileUpload.length()` returns the correct value, the byte array from `get()` returns only a partial array.

### Steps to reproduce

Create a `HttpPostMultipartRequestDecoder` and call offer at least twice with some request form data. The test below passes on 4.1.50 but fails on 4.1.51.

### Minimal yet complete reproducer code (or URL to code)

````java
@Test
public void fileUploadRegression() throws IOException {
    byte[] file = new byte[32000];
    Arrays.fill(file, (byte) 1);

    String body = "--861fbeab-cd20-470c-9609-d40a0f704466\n" +
        "Content-Disposition: form-data; name=\"image\"; filename=\"guangzhou.jpeg\"\n" +
        "Content-Type: image/jpeg\n" +
        "Content-Length: " + file.length + "\n" +
        "\n" + new String(file, UTF_8) + "\n" +
        "--861fbeab-cd20-470c-9609-d40a0f704466--\n";
    byte[] bodyBytes = body.getBytes(UTF_8);

    HttpRequest request = new DefaultHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.POST, "/upload");
    request.headers().set("content-type", "multipart/form-data; boundary=861fbeab-cd20-470c-9609-d40a0f704466");
    request.headers().set("content-length", bodyBytes.length);
    HttpPostMultipartRequestDecoder decoder = new HttpPostMultipartRequestDecoder(request);
    decoder.offer(new DefaultHttpContent(Unpooled.copiedBuffer(bodyBytes, 0, 8192)));
    decoder.offer(new DefaultHttpContent(Unpooled.copiedBuffer(bodyBytes, 8192, bodyBytes.length - 8192)));
    decoder.offer(new DefaultLastHttpContent());
    FileUpload data = (FileUpload) decoder.getBodyHttpDatas().get(0);
    assertThat(data.length(), is((long)file.length)); // passes
    assertThat(data.get().length, is(file.length)); // fails on 4.1.51
}
````

### Netty version

4.1.51

### JVM version (e.g. `java -version`)

Multiple versions tested (8, 10, 11, 12). 

### OS version (e.g. `uname -a`)

Windows, Linux.


Hints:
@normanmaurer Might it be linked to recent changes, such as in https://github.com/netty/netty/blob/c4754cf7b890e7169b2438567fa2f881728715ed/codec-http/src/main/java/io/netty/handler/codec/http/multipart/AbstractDiskHttpData.java#L167

I saw a few days ago a remark on the possibility that this call might not write all buffer to filechannel (not getting the real number of bytes written) and that the buffer has its readerIndex not changed.
I don't have any access right now, except web, so I cannot investigate more, but it might be linked, wdyt?

@normanmaurer Might it be linked to recent changes, such as in https://github.com/netty/netty/blob/c4754cf7b890e7169b2438567fa2f881728715ed/codec-http/src/main/java/io/netty/handler/codec/http/multipart/AbstractDiskHttpData.java#L167

I saw a few days ago a remark on the possibility that this call might not write all buffer to filechannel (not getting the real number of bytes written) and that the buffer has its readerIndex not changed.
I don't have any access right now, except web, so I cannot investigate more, but it might be linked, wdyt?

Patch:
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/AbstractDiskHttpData.java b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/AbstractDiskHttpData.java
--- a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/AbstractDiskHttpData.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/AbstractDiskHttpData.java
@@ -156,7 +156,6 @@ public void addContent(ByteBuf buffer, boolean last)
                     throw new IOException("Out of size: " + (size + localsize) +
                             " > " + definedSize);
                 }
-                int written = 0;
                 if (file == null) {
                     file = tempFile();
                 }
@@ -164,9 +163,21 @@ public void addContent(ByteBuf buffer, boolean last)
                     RandomAccessFile accessFile = new RandomAccessFile(file, "rw");
                     fileChannel = accessFile.getChannel();
                 }
-                buffer.getBytes(buffer.readerIndex(), fileChannel, fileChannel.position(), localsize);
+                int totalWritten = 0;
+                long position = fileChannel.position();
+                int index = buffer.readerIndex();
+                while (totalWritten < localsize) {
+                    int written = buffer.getBytes(index, fileChannel, position, localsize - totalWritten);
+                    if (written < 0) {
+                        break;
+                    }
+                    totalWritten += written;
+                    position += written;
+                    index += written;
+                }
+                fileChannel.position(position);
+                buffer.readerIndex(index);
                 size += localsize;
-                buffer.readerIndex(buffer.readerIndex() + written);
             } finally {
                 // Release the buffer as it was retained before and we not need a reference to it at all
                 // See https://github.com/netty/netty/issues/1516


Test patch:
diff --git a/codec-http/src/test/java/io/netty/handler/codec/http/multipart/DiskFileUploadTest.java b/codec-http/src/test/java/io/netty/handler/codec/http/multipart/DiskFileUploadTest.java
--- a/codec-http/src/test/java/io/netty/handler/codec/http/multipart/DiskFileUploadTest.java
+++ b/codec-http/src/test/java/io/netty/handler/codec/http/multipart/DiskFileUploadTest.java
@@ -113,16 +113,19 @@ public void testNonEmptyBufferSetMultipleTimes() throws IOException {
     public void testAddContents() throws Exception {
         DiskFileUpload f1 = new DiskFileUpload("file1", "file1", "application/json", null, null, 0);
         try {
-            String json = "{\"foo\":\"bar\"}";
-            byte[] bytes = json.getBytes(CharsetUtil.UTF_8);
-            f1.addContent(Unpooled.wrappedBuffer(bytes), true);
-            assertEquals(json, f1.getString());
-            assertArrayEquals(bytes, f1.get());
+            byte[] jsonBytes = new byte[4096];
+            PlatformDependent.threadLocalRandom().nextBytes(jsonBytes);
+
+            f1.addContent(Unpooled.wrappedBuffer(jsonBytes, 0, 1024), false);
+            f1.addContent(Unpooled.wrappedBuffer(jsonBytes, 1024, jsonBytes.length - 1024), true);
+            assertArrayEquals(jsonBytes, f1.get());
+
             File file = f1.getFile();
-            assertEquals((long) bytes.length, file.length());
+            assertEquals(jsonBytes.length, file.length());
+
             FileInputStream fis = new FileInputStream(file);
             try {
-                byte[] buf = new byte[bytes.length];
+                byte[] buf = new byte[jsonBytes.length];
                 int offset = 0;
                 int read = 0;
                 int len = buf.length;
@@ -133,7 +136,7 @@ public void testAddContents() throws Exception {
                         break;
                     }
                 }
-                assertArrayEquals(bytes, buf);
+                assertArrayEquals(jsonBytes, buf);
             } finally {
                 fis.close();
             }