Repository: netty/netty
Issue: DefaultFileRegion.transferTo with invalid count may cause hang 
Labels: defect

### Expected behavior

`DefaultFileRegion.transferTo` will end successfully or throws Exception.

### Actual behavior

Thread will be hang and CPU usage increase to 100%

### Steps to reproduce

Initialize `DefaultFileRegion` with `count` larger than the actual `FileChannel ` size.

```
new DefaultFileRegion(raf.getChannel(), 0, fileLength)
```

### Minimal yet complete reproducer code (or URL to code)

https://github.com/netty/netty/blob/4.1/example/src/main/java/io/netty/example/http/file/HttpStaticFileServerHandler.java#L175-L193

Truncate files from another process between L175 and L192


Related issue.
https://github.com/xitrum-framework/xitrum/issues/675

### Netty version
4.1

### JVM version (e.g. `java -version`)

java version "1.8.0_202"
Java(TM) SE Runtime Environment (build 1.8.0_202-b08)
Java HotSpot(TM) 64-Bit Server VM (build 25.202-b08, mixed mode)

### OS version (e.g. `uname -a`)

Darwin OshidaIMac.local 17.7.0 Darwin Kernel Version 17.7.0: Wed Oct 10 23:06:14 PDT 2018; root:xnu-4570.71.13~1/RELEASE_X86_64 x86_64

Issue comments:
@georgeOsdDev what you suggest we should do here ? You are saying we should check in the constructor that you do not construct something "invalid" ?
I'm not sure to check `count` in the constructor.
`sun.nio.ch.FileChannelImpl` does not throws Exception even if `count ` is larger than real file size.
https://docs.oracle.com/javase/8/docs/api/java/nio/channels/FileChannel.html#transferTo-long-long-java.nio.channels.WritableByteChannel-

---
When CPU usage increase, It seems `NioSocketChannel.doWrite` is called repeatedly. So it seems to be a matter of eventLoop handling
```
io.netty.channel.socket.nio.NioSocketChannel.doWrite(NioSocketChannel.java:375)
io.netty.channel.AbstractChannel$AbstractUnsafe.flush0(AbstractChannel.java:934)
io.netty.channel.nio.AbstractNioChannel$AbstractNioUnsafe.forceFlush(AbstractNioChannel.java:367)
io.netty.channel.nio.NioEventLoop.processSelectedKey(NioEventLoop.java:639)
io.netty.channel.nio.NioEventLoop.processSelectedKeysOptimized(NioEventLoop.java:580)
io.netty.channel.nio.NioEventLoop.processSelectedKeys(NioEventLoop.java:497)
io.netty.channel.nio.NioEventLoop.run(NioEventLoop.java:459)
io.netty.util.concurrent.SingleThreadEventExecutor$5.run(SingleThreadEventExecutor.java:886)
io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
```

When `count` is *not* larger than realSize,  `in.remove();` is called and doWrite is called just once.
https://github.com/netty/netty/blob/4.1/transport/src/main/java/io/netty/channel/nio/AbstractNioByteChannel.java#L238
Looking into it
@georgeOsdDev PTAL https://github.com/netty/netty/pull/8885

Pull request review comments:

Patch that resolved the issue:
diff --git a/transport-native-epoll/src/main/java/io/netty/channel/epoll/AbstractEpollStreamChannel.java b/transport-native-epoll/src/main/java/io/netty/channel/epoll/AbstractEpollStreamChannel.java
index 8d993b48e74..e2f2c88cb72 100644
--- a/transport-native-epoll/src/main/java/io/netty/channel/epoll/AbstractEpollStreamChannel.java
+++ b/transport-native-epoll/src/main/java/io/netty/channel/epoll/AbstractEpollStreamChannel.java
@@ -367,13 +367,13 @@ private int writeBytesMultiple(
      * </ul>
      */
     private int writeDefaultFileRegion(ChannelOutboundBuffer in, DefaultFileRegion region) throws Exception {
+        final long offset = region.transferred();
         final long regionCount = region.count();
-        if (region.transferred() >= regionCount) {
+        if (offset >= regionCount) {
             in.remove();
             return 0;
         }
 
-        final long offset = region.transferred();
         final long flushedAmount = socket.sendFile(region, region.position(), offset, regionCount - offset);
         if (flushedAmount > 0) {
             in.progress(flushedAmount);
@@ -381,6 +381,8 @@ private int writeDefaultFileRegion(ChannelOutboundBuffer in, DefaultFileRegion r
                 in.remove();
             }
             return 1;
+        } else if (flushedAmount == 0) {
+            validateFileRegion(region, offset);
         }
         return WRITE_STATUS_SNDBUF_FULL;
     }
diff --git a/transport-native-kqueue/src/main/java/io/netty/channel/kqueue/AbstractKQueueStreamChannel.java b/transport-native-kqueue/src/main/java/io/netty/channel/kqueue/AbstractKQueueStreamChannel.java
index 7d604f150ac..4b7f4e06e5b 100644
--- a/transport-native-kqueue/src/main/java/io/netty/channel/kqueue/AbstractKQueueStreamChannel.java
+++ b/transport-native-kqueue/src/main/java/io/netty/channel/kqueue/AbstractKQueueStreamChannel.java
@@ -210,12 +210,13 @@ private int writeBytesMultiple(
      */
     private int writeDefaultFileRegion(ChannelOutboundBuffer in, DefaultFileRegion region) throws Exception {
         final long regionCount = region.count();
-        if (region.transferred() >= regionCount) {
+        final long offset = region.transferred();
+
+        if (offset >= regionCount) {
             in.remove();
             return 0;
         }
 
-        final long offset = region.transferred();
         final long flushedAmount = socket.sendFile(region, region.position(), offset, regionCount - offset);
         if (flushedAmount > 0) {
             in.progress(flushedAmount);
@@ -223,6 +224,8 @@ private int writeDefaultFileRegion(ChannelOutboundBuffer in, DefaultFileRegion r
                 in.remove();
             }
             return 1;
+        } else if (flushedAmount == 0) {
+            validateFileRegion(region, offset);
         }
         return WRITE_STATUS_SNDBUF_FULL;
     }
diff --git a/transport/src/main/java/io/netty/channel/AbstractChannel.java b/transport/src/main/java/io/netty/channel/AbstractChannel.java
index 8f8fe2f787a..fcd596563ef 100644
--- a/transport/src/main/java/io/netty/channel/AbstractChannel.java
+++ b/transport/src/main/java/io/netty/channel/AbstractChannel.java
@@ -1149,6 +1149,10 @@ protected Object filterOutboundMessage(Object msg) throws Exception {
         return msg;
     }
 
+    protected void validateFileRegion(DefaultFileRegion region, long position) throws IOException {
+        DefaultFileRegion.validate(region, position);
+    }
+
     static final class CloseFuture extends DefaultChannelPromise {
 
         CloseFuture(AbstractChannel ch) {
diff --git a/transport/src/main/java/io/netty/channel/DefaultFileRegion.java b/transport/src/main/java/io/netty/channel/DefaultFileRegion.java
index 2435b20a313..2f6bea95a1c 100644
--- a/transport/src/main/java/io/netty/channel/DefaultFileRegion.java
+++ b/transport/src/main/java/io/netty/channel/DefaultFileRegion.java
@@ -139,6 +139,12 @@ public long transferTo(WritableByteChannel target, long position) throws IOExcep
         long written = file.transferTo(this.position + position, count, target);
         if (written > 0) {
             transferred += written;
+        } else if (written == 0) {
+            // If the amount of written data is 0 we need to check if the requested count is bigger then the
+            // actual file itself as it may have been truncated on disk.
+            //
+            // See https://github.com/netty/netty/issues/8868
+            validate(this, position);
         }
         return written;
     }
@@ -182,4 +188,16 @@ public FileRegion touch() {
     public FileRegion touch(Object hint) {
         return this;
     }
+
+    static void validate(DefaultFileRegion region, long position) throws IOException {
+        // If the amount of written data is 0 we need to check if the requested count is bigger then the
+        // actual file itself as it may have been truncated on disk.
+        //
+        // See https://github.com/netty/netty/issues/8868
+        long size = region.file.size();
+        long count = region.count - position;
+        if (region.position + count + position > size) {
+            throw new IOException("Underlying file size " + size + " smaller then requested count " + region.count);
+        }
+    }
 }


Test patch:
diff --git a/testsuite/src/main/java/io/netty/testsuite/transport/socket/SocketFileRegionTest.java b/testsuite/src/main/java/io/netty/testsuite/transport/socket/SocketFileRegionTest.java
index 53deb6c6743..881acd1bcff 100644
--- a/testsuite/src/main/java/io/netty/testsuite/transport/socket/SocketFileRegionTest.java
+++ b/testsuite/src/main/java/io/netty/testsuite/transport/socket/SocketFileRegionTest.java
@@ -22,11 +22,13 @@
 import io.netty.channel.Channel;
 import io.netty.channel.ChannelHandlerContext;
 import io.netty.channel.ChannelInboundHandler;
+import io.netty.channel.ChannelInboundHandlerAdapter;
 import io.netty.channel.ChannelOption;
 import io.netty.channel.DefaultFileRegion;
 import io.netty.channel.FileRegion;
 import io.netty.channel.SimpleChannelInboundHandler;
 import io.netty.util.internal.PlatformDependent;
+import org.hamcrest.CoreMatchers;
 import org.junit.Test;
 
 import java.io.File;
@@ -73,6 +75,11 @@ public void testFileRegionVoidPromiseNotAutoRead() throws Throwable {
         run();
     }
 
+    @Test
+    public void testFileRegionCountLargerThenFile() throws Throwable {
+        run();
+    }
+
     public void testFileRegion(ServerBootstrap sb, Bootstrap cb) throws Throwable {
         testFileRegion0(sb, cb, false, true, true);
     }
@@ -93,6 +100,34 @@ public void testFileRegionVoidPromiseNotAutoRead(ServerBootstrap sb, Bootstrap c
         testFileRegion0(sb, cb, true, false, true);
     }
 
+    public void testFileRegionCountLargerThenFile(ServerBootstrap sb, Bootstrap cb) throws Throwable {
+        File file = File.createTempFile("netty-", ".tmp");
+        file.deleteOnExit();
+
+        final FileOutputStream out = new FileOutputStream(file);
+        out.write(data);
+        out.close();
+
+        sb.childHandler(new SimpleChannelInboundHandler<ByteBuf>() {
+            @Override
+            protected void channelRead0(ChannelHandlerContext ctx, ByteBuf msg) {
+                // Just drop the message.
+            }
+        });
+        cb.handler(new ChannelInboundHandlerAdapter());
+
+        Channel sc = sb.bind().sync().channel();
+        Channel cc = cb.connect(sc.localAddress()).sync().channel();
+
+        // Request file region which is bigger then the underlying file.
+        FileRegion region = new DefaultFileRegion(
+                new FileInputStream(file).getChannel(), 0, data.length + 1024);
+
+        assertThat(cc.writeAndFlush(region).await().cause(), CoreMatchers.<Throwable>instanceOf(IOException.class));
+        cc.close().sync();
+        sc.close().sync();
+    }
+
     private static void testFileRegion0(
             ServerBootstrap sb, Bootstrap cb, boolean voidPromise, final boolean autoRead, boolean defaultFileRegion)
             throws Throwable {
diff --git a/transport/src/test/java/io/netty/channel/DefaultFileRegionTest.java b/transport/src/test/java/io/netty/channel/DefaultFileRegionTest.java
new file mode 100644
index 00000000000..e416bccba59
--- /dev/null
+++ b/transport/src/test/java/io/netty/channel/DefaultFileRegionTest.java
@@ -0,0 +1,120 @@
+/*
+ * Copyright 2019 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.channel;
+
+import io.netty.util.internal.PlatformDependent;
+import org.junit.Test;
+
+import java.io.ByteArrayOutputStream;
+import java.io.File;
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.io.RandomAccessFile;
+import java.nio.channels.Channels;
+import java.nio.channels.WritableByteChannel;
+
+import static org.junit.Assert.assertArrayEquals;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.fail;
+
+public class DefaultFileRegionTest {
+
+    private static final byte[] data = new byte[1048576 * 10];
+
+    static {
+        PlatformDependent.threadLocalRandom().nextBytes(data);
+    }
+
+    private static File newFile() throws IOException {
+        File file = File.createTempFile("netty-", ".tmp");
+        file.deleteOnExit();
+
+        final FileOutputStream out = new FileOutputStream(file);
+        out.write(data);
+        out.close();
+        return file;
+    }
+
+    @Test
+    public void testCreateFromFile() throws IOException  {
+        File file = newFile();
+        try {
+            testFileRegion(new DefaultFileRegion(file, 0, data.length));
+        } finally {
+            file.delete();
+        }
+    }
+
+    @Test
+    public void testCreateFromFileChannel() throws IOException  {
+        File file = newFile();
+        RandomAccessFile randomAccessFile = new RandomAccessFile(file, "r");
+        try {
+            testFileRegion(new DefaultFileRegion(randomAccessFile.getChannel(), 0, data.length));
+        } finally {
+            randomAccessFile.close();
+            file.delete();
+        }
+    }
+
+    private static void testFileRegion(FileRegion region) throws IOException  {
+        ByteArrayOutputStream outputStream = new ByteArrayOutputStream();
+        WritableByteChannel channel = Channels.newChannel(outputStream);
+
+        try {
+            assertEquals(data.length, region.count());
+            assertEquals(0, region.transferred());
+            assertEquals(data.length, region.transferTo(channel, 0));
+            assertEquals(data.length, region.count());
+            assertEquals(data.length, region.transferred());
+            assertArrayEquals(data, outputStream.toByteArray());
+        } finally {
+            channel.close();
+        }
+    }
+
+    @Test
+    public void testTruncated() throws IOException  {
+        File file = newFile();
+        ByteArrayOutputStream outputStream = new ByteArrayOutputStream();
+        WritableByteChannel channel = Channels.newChannel(outputStream);
+        RandomAccessFile randomAccessFile = new RandomAccessFile(file, "rw");
+
+        try {
+            FileRegion region = new DefaultFileRegion(randomAccessFile.getChannel(), 0, data.length);
+
+            randomAccessFile.getChannel().truncate(data.length - 1024);
+
+            assertEquals(data.length, region.count());
+            assertEquals(0, region.transferred());
+
+            assertEquals(data.length - 1024, region.transferTo(channel, 0));
+            assertEquals(data.length, region.count());
+            assertEquals(data.length - 1024, region.transferred());
+            try {
+                region.transferTo(channel, data.length - 1024);
+                fail();
+            } catch (IOException expected) {
+                // expected
+            }
+        } finally {
+            channel.close();
+
+            randomAccessFile.close();
+            file.delete();
+        }
+    }
+}