From dba08ba26f75d550019fe129a2f722687cf710a4 Mon Sep 17 00:00:00 2001
From: Ayush Ranjan <ayushranjan@google.com>
Date: Thu, 30 Jun 2022 16:41:50 -0700
Subject: [PATCH] LISAFS client file read must return io.EOF when appropriate.

This is consistent with POSIX semantics and with 9P client file.
Some VFS callers like splice really depend on such a non-nil
error being returned from FD.Read(). Otherwise, sendfile will
hang forever, spaming FD.Read() for more data.

Added a regression test. This test reproduces the issue when:
- LISAFS is enabled.
- sendfile(2) called with input file for which we don't have
  cached mapping (so will need to read from gofer). Can be
  simulated with shared gofer mode.
- Host FD is not available for input file to perform read. Tested
  internally.
Hence this test will be more meaningful when lisafs is enabled
by default in all tests.

PiperOrigin-RevId: 458340830
---
 pkg/lisafs/client_file.go       | 15 ++++++++++++++-
 test/syscalls/linux/sendfile.cc | 27 +++++++++++++++++++++++++++
 2 files changed, 41 insertions(+), 1 deletion(-)

diff --git a/pkg/lisafs/client_file.go b/pkg/lisafs/client_file.go
index 2065919b8..596d85afa 100644
--- a/pkg/lisafs/client_file.go
+++ b/pkg/lisafs/client_file.go
@@ -16,6 +16,7 @@ package lisafs
 
 import (
 	"fmt"
+	"io"
 
 	"golang.org/x/sys/unix"
 	"gvisor.dev/gvisor/pkg/abi/linux"
@@ -174,7 +175,19 @@ func (f *ClientFD) Read(ctx context.Context, dst []byte, offset uint64) (uint64,
 		ctx.UninterruptibleSleepStart(false)
 		err := f.client.SndRcvMessage(PRead, uint32(req.SizeBytes()), req.MarshalUnsafe, resp.CheckedUnmarshal, nil, req.String, resp.String)
 		ctx.UninterruptibleSleepFinish(false)
-		return uint64(resp.NumBytes), err
+		if err != nil {
+			return 0, err
+		}
+
+		// io.EOF is not an error that a lisafs server can return. Use POSIX
+		// semantics to return io.EOF manually: zero bytes were returned and a
+		// non-zero buffer was used.
+		// NOTE(b/237442794): Some callers like splice really depend on a non-nil
+		// error being returned in such a case. This is consistent with P9.
+		if resp.NumBytes == 0 && len(buf) > 0 {
+			return 0, io.EOF
+		}
+		return uint64(resp.NumBytes), nil
 	})
 }
 
diff --git a/test/syscalls/linux/sendfile.cc b/test/syscalls/linux/sendfile.cc
index 6fa746221..d00356399 100644
--- a/test/syscalls/linux/sendfile.cc
+++ b/test/syscalls/linux/sendfile.cc
@@ -18,6 +18,8 @@
 #include <sys/sendfile.h>
 #include <unistd.h>
 
+#include <string_view>
+
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include "absl/strings/string_view.h"
@@ -627,6 +629,31 @@ TEST(SendFileTest, SendFileToSelf) {
               SyscallSucceedsWithValue(kSendfileSize));
 }
 
+// NOTE(b/237442794): Regression test. Make sure sendfile works with a count
+// larger than input file size.
+TEST(SendFileTest, LargeCount) {
+  // Create input file with some wisdom. It is imperative to use a
+  // Shakespearean quote, consistent with the rest of this file.
+  constexpr std::string_view kData =
+      "We know what we are, but know not what we may be.";
+  const TempPath in_file = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateFileWith(
+      GetAbsoluteTestTmpdir(), kData, TempPath::kDefaultFileMode));
+
+  const TempPath out_file = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateFile());
+
+  // Open the input file as read only.
+  const FileDescriptor inf =
+      ASSERT_NO_ERRNO_AND_VALUE(Open(in_file.path(), O_RDONLY));
+
+  // Open the output file as write only.
+  const FileDescriptor outf =
+      ASSERT_NO_ERRNO_AND_VALUE(Open(out_file.path(), O_WRONLY));
+
+  // Set a count larger than kDataSize.
+  EXPECT_THAT(sendfile(outf.get(), inf.get(), nullptr, 2 * kData.size()),
+              SyscallSucceedsWithValue(kData.size()));
+}
+
 }  // namespace
 
 }  // namespace testing