From 4ce6774336319e920d2fdbb2629978cad5b8a7da Mon Sep 17 00:00:00 2001
From: Violeta Georgieva <violetag@vmware.com>
Date: Fri, 14 Aug 2020 11:54:43 +0300
Subject: [PATCH] =?UTF-8?q?AbstractDiskHttpData#getChunk=20closes=20fileCh?=
 =?UTF-8?q?annel=20only=20if=20everything=20w=E2=80=A6=20(#10481)?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Motivation:
AbstractDiskHttpData#getChunk opens and closes fileChannel every time when it is invoked,
as a result the uploaded file is corrupted. This is a regression caused by #10270.

Modifications:

- Close the fileChannel only if everything was read or an exception is thrown
- Add unit test

Result:
AbstractDiskHttpData#getChunk closes fileChannel only if everything was read or an exception is thrown
---
 .../http/multipart/AbstractDiskHttpData.java  |   5 +-
 .../multipart/AbstractDiskHttpDataTest.java   | 128 ++++++++++++++++++
 2 files changed, 132 insertions(+), 1 deletion(-)
 create mode 100644 codec-http/src/test/java/io/netty/handler/codec/http/multipart/AbstractDiskHttpDataTest.java

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
index 25d13cd26a..61397967f5 100644
--- 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
@@ -307,13 +307,16 @@ public abstract class AbstractDiskHttpData extends AbstractHttpData {
             while (read < length) {
                 int readnow = fileChannel.read(byteBuffer);
                 if (readnow == -1) {
+                    fileChannel.close();
+                    fileChannel = null;
                     break;
                 }
                 read += readnow;
             }
-        } finally {
+        } catch (IOException e) {
             fileChannel.close();
             fileChannel = null;
+            throw e;
         }
         if (read == 0) {
             return EMPTY_BUFFER;
diff --git a/codec-http/src/test/java/io/netty/handler/codec/http/multipart/AbstractDiskHttpDataTest.java b/codec-http/src/test/java/io/netty/handler/codec/http/multipart/AbstractDiskHttpDataTest.java
new file mode 100644
index 0000000000..07303425ba
--- /dev/null
+++ b/codec-http/src/test/java/io/netty/handler/codec/http/multipart/AbstractDiskHttpDataTest.java
@@ -0,0 +1,128 @@
+/*
+ * Copyright 2020 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:
+ *
+ *   http://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.ByteBufUtil;
+import org.junit.Test;
+
+import java.io.File;
+import java.io.FileOutputStream;
+import java.nio.charset.Charset;
+import java.util.Arrays;
+import java.util.UUID;
+import java.util.concurrent.ThreadLocalRandom;
+
+import static io.netty.util.CharsetUtil.UTF_8;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+
+/**
+ * {@link AbstractDiskHttpData} test cases
+ */
+public class AbstractDiskHttpDataTest {
+
+    @Test
+    public void testGetChunk() throws Exception {
+        TestHttpData test = new TestHttpData("test", UTF_8, 0);
+        try {
+            File tmpFile = File.createTempFile(UUID.randomUUID().toString(), ".tmp");
+            tmpFile.deleteOnExit();
+            FileOutputStream fos = new FileOutputStream(tmpFile);
+            byte[] bytes = new byte[4096];
+            ThreadLocalRandom.current().nextBytes(bytes);
+            try {
+                fos.write(bytes);
+                fos.flush();
+            } finally {
+                fos.close();
+            }
+            test.setContent(tmpFile);
+            ByteBuf buf1 = test.getChunk(1024);
+            assertEquals(buf1.readerIndex(), 0);
+            assertEquals(buf1.writerIndex(), 1024);
+            ByteBuf buf2 = test.getChunk(1024);
+            assertEquals(buf2.readerIndex(), 0);
+            assertEquals(buf2.writerIndex(), 1024);
+            assertFalse("Arrays should not be equal",
+                    Arrays.equals(ByteBufUtil.getBytes(buf1), ByteBufUtil.getBytes(buf2)));
+        } finally {
+            test.delete();
+        }
+    }
+
+    private static final class TestHttpData extends AbstractDiskHttpData {
+
+        private TestHttpData(String name, Charset charset, long size) {
+            super(name, charset, size);
+        }
+
+        @Override
+        protected String getDiskFilename() {
+            return null;
+        }
+
+        @Override
+        protected String getPrefix() {
+            return null;
+        }
+
+        @Override
+        protected String getBaseDirectory() {
+            return null;
+        }
+
+        @Override
+        protected String getPostfix() {
+            return null;
+        }
+
+        @Override
+        protected boolean deleteOnExit() {
+            return false;
+        }
+
+        @Override
+        public HttpData copy() {
+            return null;
+        }
+
+        @Override
+        public HttpData duplicate() {
+            return null;
+        }
+
+        @Override
+        public HttpData retainedDuplicate() {
+            return null;
+        }
+
+        @Override
+        public HttpData replace(ByteBuf content) {
+            return null;
+        }
+
+        @Override
+        public HttpDataType getHttpDataType() {
+            return null;
+        }
+
+        @Override
+        public int compareTo(InterfaceHttpData o) {
+            return 0;
+        }
+    }
+}