Recommit "MemoryBuffer: Add a missing error-check to getOpenFileImpl"

This recommits r368977, which was reverted in r369027 due to test
failures in lldb. The cause of this was different behavior of
readNativeFileSlice on windows and unix. These have been addressed in
r369269.

The original commit message was:
In case the function was called with a desired read size *and* the file
was not an "mmap()" candidate, the function was falling back to a
"pread()", but it was failing to check the result of that system call.
This meant that the function would return "success" even though the read
operation failed, and it returned a buffer full of uninitialized memory.

Reviewers: rnk, dblaikie

Subscribers: kristina, llvm-commits

Tags: #llvm

Differential Revision: https://reviews.llvm.org/D66224

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@369370 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/lib/Support/MemoryBuffer.cpp b/lib/Support/MemoryBuffer.cpp
index d0e5bb154c1aee706c97af93575e66ea2730dd9b..2f31f059ddbd7c2c37517779a24e3b096099dc63 100644 (file)
--- a/lib/Support/MemoryBuffer.cpp
+++ b/lib/Support/MemoryBuffer.cpp
@@ -458,7 +458,9 @@ getOpenFileImpl(sys::fs::file_t FD, const Twine &Filename, uint64_t FileSize,
     return make_error_code(errc::not_enough_memory);
   }
 
-  sys::fs::readNativeFileSlice(FD, Buf->getBuffer(), Offset);
+  if (std::error_code EC =
+          sys::fs::readNativeFileSlice(FD, Buf->getBuffer(), Offset))
+    return EC;
 
   return std::move(Buf);
 }

diff --git a/unittests/Support/MemoryBufferTest.cpp b/unittests/Support/MemoryBufferTest.cpp
index 2f9664308dc4d18b0b4dc2025e8624d30a063253..629b94d7843229370f9ea2756390deb8840f345a 100644 (file)
--- a/unittests/Support/MemoryBufferTest.cpp
+++ b/unittests/Support/MemoryBufferTest.cpp
@@ -11,6 +11,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Support/MemoryBuffer.h"
+#include "llvm/ADT/ScopeExit.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/FileUtilities.h"
 #include "llvm/Support/raw_ostream.h"
@@ -19,6 +20,25 @@
 
 using namespace llvm;
 
+#define ASSERT_NO_ERROR(x)                                                     \
+  if (std::error_code ASSERT_NO_ERROR_ec = x) {                                \
+    SmallString<128> MessageStorage;                                           \
+    raw_svector_ostream Message(MessageStorage);                               \
+    Message << #x ": did not return errc::success.\n"                          \
+            << "error number: " << ASSERT_NO_ERROR_ec.value() << "\n"          \
+            << "error message: " << ASSERT_NO_ERROR_ec.message() << "\n";      \
+    GTEST_FATAL_FAILURE_(MessageStorage.c_str());                              \
+  } else {                                                                     \
+  }
+
+#define ASSERT_ERROR(x)                                                        \
+  if (!x) {                                                                    \
+    SmallString<128> MessageStorage;                                           \
+    raw_svector_ostream Message(MessageStorage);                               \
+    Message << #x ": did not return a failure error code.\n";                  \
+    GTEST_FATAL_FAILURE_(MessageStorage.c_str());                              \
+  }
+
 namespace {
 
 class MemoryBufferTest : public testing::Test {
@@ -65,6 +85,37 @@ TEST_F(MemoryBufferTest, get) {
   EXPECT_EQ("this is some data", data);
 }
 
+TEST_F(MemoryBufferTest, getOpenFile) {
+  int FD;
+  SmallString<64> TestPath;
+  ASSERT_EQ(sys::fs::createTemporaryFile("MemoryBufferTest_getOpenFile", "temp",
+                                         FD, TestPath),
+            std::error_code());
+
+  FileRemover Cleanup(TestPath);
+  raw_fd_ostream OF(FD, /*shouldClose*/ true);
+  OF << "12345678";
+  OF.close();
+
+  {
+    Expected<sys::fs::file_t> File = sys::fs::openNativeFileForRead(TestPath);
+    ASSERT_THAT_EXPECTED(File, Succeeded());
+    auto OnExit =
+        make_scope_exit([&] { ASSERT_NO_ERROR(sys::fs::closeFile(*File)); });
+    ErrorOr<OwningBuffer> MB = MemoryBuffer::getOpenFile(*File, TestPath, 6);
+    ASSERT_NO_ERROR(MB.getError());
+    EXPECT_EQ("123456", MB.get()->getBuffer());
+  }
+  {
+    Expected<sys::fs::file_t> File = sys::fs::openNativeFileForWrite(
+        TestPath, sys::fs::CD_OpenExisting, sys::fs::OF_None);
+    ASSERT_THAT_EXPECTED(File, Succeeded());
+    auto OnExit =
+        make_scope_exit([&] { ASSERT_NO_ERROR(sys::fs::closeFile(*File)); });
+    ASSERT_ERROR(MemoryBuffer::getOpenFile(*File, TestPath, 6).getError());
+  }
+}
+
 TEST_F(MemoryBufferTest, NullTerminator4K) {
   // Test that a file with size that is a multiple of the page size can be null
   // terminated correctly by MemoryBuffer.