[llvm] r369370 - Recommit "MemoryBuffer: Add a missing error-check to getOpenFileImpl"

[Pavel Labath via llvm-commits]

Author: labath
Date: Tue Aug 20 05:08:52 2019
New Revision: 369370

URL: http://llvm.org/viewvc/llvm-project?rev=369370&view=rev
Log:
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

Modified:
    llvm/trunk/lib/Support/MemoryBuffer.cpp
    llvm/trunk/unittests/Support/MemoryBufferTest.cpp

Modified: llvm/trunk/lib/Support/MemoryBuffer.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/MemoryBuffer.cpp?rev=369370&r1=369369&r2=369370&view=diff
==============================================================================
--- llvm/trunk/lib/Support/MemoryBuffer.cpp (original)
+++ llvm/trunk/lib/Support/MemoryBuffer.cpp Tue Aug 20 05:08:52 2019
@@ -458,7 +458,9 @@ getOpenFileImpl(sys::fs::file_t FD, cons
     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);
 }

Modified: llvm/trunk/unittests/Support/MemoryBufferTest.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Support/MemoryBufferTest.cpp?rev=369370&r1=369369&r2=369370&view=diff
==============================================================================
--- llvm/trunk/unittests/Support/MemoryBufferTest.cpp (original)
+++ llvm/trunk/unittests/Support/MemoryBufferTest.cpp Tue Aug 20 05:08:52 2019
@@ -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.