MemoryBuffer: Add a missing error-check to getOpenFileImpl

[labath]

Summary

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.

[labath]

PS: I discovered this when accidentally trying to read a directory, but it is not possible to write a portable test for that, as some BSDs will actually allow you to do that, so the test is written via a write-only file handle instead.

[dblaikie]

Seems reasonable

[stella.stamenova]

It looks like this changed caused 30 or so of the tests to fail on the Windows bot. It was already broken, so you wouldn't have gotten an email:

http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/7851

[labath]

In D66224#1631549, @stella.stamenova wrote:

It looks like this changed caused 30 or so of the tests to fail on the Windows bot. It was already broken, so you wouldn't have gotten an email:

http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/7851

Oh my... I was doing this to fix a different LLDB bug :/. Lemme boot up windows to see what's happening here...

[labath]

Ok, I have an idea what's going on. There's a difference in the behavior of readNativeFileSlice on windows&posix, and neither of them really do what MemoryBuffer expects here. I don't have time to fix this today so I'll revert and get back to this tomorrow. Thanks for the heads up.

lib/Support/MemoryBuffer.cpp

getOpenFileImpl(sys::fs::file_t FD, const Twine &Filename, uint64_t FileSize,
 
   auto Buf = WritableMemoryBuffer::getNewUninitMemBuffer(MapSize, Filename);
   if (!Buf) {
     // Failed to create a buffer. The only way it can fail is if
     // new(std::nothrow) returns 0.
     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);
 }
 
 ErrorOr<std::unique_ptr<MemoryBuffer>>
 MemoryBuffer::getOpenFile(sys::fs::file_t FD, const Twine &Filename, uint64_t FileSize,
                           bool RequiresNullTerminator, bool IsVolatile) {
   return getOpenFileImpl<MemoryBuffer>(FD, Filename, FileSize, FileSize, 0,

unittests/Support/MemoryBufferTest.cpp

 //===- llvm/unittest/Support/MemoryBufferTest.cpp - MemoryBuffer tests ----===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file implements unit tests for the MemoryBuffer support class.
 //
 //===----------------------------------------------------------------------===//
 
 #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"
 #include "llvm/Testing/Support/Error.h"
 #include "gtest/gtest.h"
 
 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 {
 protected:
   MemoryBufferTest()
   : data("this is some data")
   { }
 
TEST_F(MemoryBufferTest, get) {
 
   // verify the original data is unmodified after deleting the buffers
   MB1.reset();
   MB2.reset();
   MB3.reset();
   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.
   int TestFD;
   SmallString<64> TestPath;
   sys::fs::createTemporaryFile("MemoryBufferTest_NullTerminator4K", "temp",
                                TestFD, TestPath);
   FileRemover Cleanup(TestPath);