Fix NPE bug caused by a failed Remap. am: 574b723a7e

Original change: https://googleplex-android-review.googlesource.com/c/platform/external/icing/+/18385601

Change-Id: Ia4fd4d2176946b29c04da5cb88f26f20bc5c6051
Signed-off-by: Automerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com>

2 files changed, 59 insertions, 15 deletions

diff --git a/icing/file/file-backed-vector_test.cc b/icing/file/file-backed-vector_test.cc
index 7c02af9..54f9ef5 100644
--- a/icing/file/file-backed-vector_test.cc
+++ b/icing/file/file-backed-vector_test.cc
@@ -14,6 +14,8 @@
 
 #include "icing/file/file-backed-vector.h"
 
+#include <unistd.h>
+
 #include <algorithm>
 #include <cerrno>
 #include <cstdint>
@@ -21,18 +23,21 @@
 #include <string_view>
 #include <vector>
 
-#include "gmock/gmock.h"
-#include "gtest/gtest.h"
-#include "icing/file/filesystem.h"
-#include "icing/file/memory-mapped-file.h"
-#include "icing/testing/common-matchers.h"
-#include "icing/testing/tmp-directory.h"
-#include "icing/util/crc32.h"
-#include "icing/util/logging.h"
+#include "knowledge/cerebra/sense/text_classifier/lib3/utils/base/status.h"
+#include "testing/base/public/gmock.h"
+#include "testing/base/public/gunit.h"
+#include "third_party/icing/file/filesystem.h"
+#include "third_party/icing/file/memory-mapped-file.h"
+#include "third_party/icing/file/mock-filesystem.h"
+#include "third_party/icing/testing/common-matchers.h"
+#include "third_party/icing/testing/tmp-directory.h"
+#include "third_party/icing/util/crc32.h"
+#include "third_party/icing/util/logging.h"
 
 using ::testing::Eq;
 using ::testing::IsTrue;
 using ::testing::Pointee;
+using ::testing::Return;
 
 namespace icing {
 namespace lib {
@@ -73,6 +78,8 @@ class FileBackedVectorTest : public testing::Test {
     return std::string_view(vector->array() + idx, expected_len);
   }
 
+  const Filesystem& filesystem() const { return filesystem_; }
+
   Filesystem filesystem_;
   std::string file_path_;
   int fd_;
@@ -637,6 +644,39 @@ TEST_F(FileBackedVectorTest, InitNormalSucceeds) {
   }
 }
 
+TEST_F(FileBackedVectorTest, RemapFailureStillValidInstance) {
+  auto mock_filesystem = std::make_unique<MockFilesystem>();
+  ICING_ASSERT_OK_AND_ASSIGN(
+      std::unique_ptr<FileBackedVector<int>> vector,
+      FileBackedVector<int>::Create(
+          *mock_filesystem, file_path_,
+          MemoryMappedFile::Strategy::READ_WRITE_AUTO_SYNC));
+
+  // 1. Write data to just before the first block resize. Running the test
+  // locally has determined that we'll first resize at 65531st entry.
+  constexpr int kResizingIndex = 16378;
+  for (int i = 0; i < kResizingIndex; ++i) {
+    ICING_ASSERT_OK(vector->Set(i, 7));
+  }
+
+  // 2. The next Set call should cause a resize and a remap. Make that remap
+  // fail.
+  int num_calls = 0;
+  auto open_lambda = [this, &num_calls](const char* file_name){
+    if (++num_calls == 2) {
+      return -1;
+    }
+    return this->filesystem().OpenForWrite(file_name);
+  };
+  ON_CALL(*mock_filesystem, OpenForWrite(_)).WillByDefault(open_lambda);
+  EXPECT_THAT(vector->Set(kResizingIndex, 7),
+              StatusIs(libtextclassifier3::StatusCode::INTERNAL));
+
+  // 3. We should still be able to call set correctly for earlier regions.
+  ICING_EXPECT_OK(vector->Set(kResizingIndex / 2, 9));
+  EXPECT_THAT(vector->Get(kResizingIndex / 2), IsOkAndHolds(Pointee(Eq(9))));
+}
+
 }  // namespace
 
 }  // namespace lib
diff --git a/icing/file/memory-mapped-file.cc b/icing/file/memory-mapped-file.cc
index bda01f2..9ff3adb 100644
--- a/icing/file/memory-mapped-file.cc
+++ b/icing/file/memory-mapped-file.cc
@@ -70,10 +70,10 @@ void MemoryMappedFile::MemoryMappedFile::Unmap() {
 
 libtextclassifier3::Status MemoryMappedFile::Remap(size_t file_offset,
                                                    size_t mmap_size) {
-  // First unmap any previously mmapped region.
-  Unmap();
-
   if (mmap_size == 0) {
+    // First unmap any previously mmapped region.
+    Unmap();
+
     // Nothing more to do.
     return libtextclassifier3::Status::OK;
   }
@@ -118,15 +118,19 @@ libtextclassifier3::Status MemoryMappedFile::Remap(size_t file_offset,
         "Unable to open file meant to be mmapped: ", file_path_));
   }
 
-  mmap_result_ = mmap(nullptr, adjusted_mmap_size, protection_flags, mmap_flags,
-                      fd.get(), aligned_offset);
+  void* mmap_result = mmap(nullptr, adjusted_mmap_size, protection_flags,
+                           mmap_flags, fd.get(), aligned_offset);
 
-  if (mmap_result_ == MAP_FAILED) {
-    mmap_result_ = nullptr;
+  if (mmap_result == MAP_FAILED) {
     return absl_ports::InternalError(absl_ports::StrCat(
         "Failed to mmap region due to error: ", strerror(errno)));
   }
 
+  // Now we know that we have successfully created a new mapping. We can free
+  // the old one and switch to the new one.
+  Unmap();
+
+  mmap_result_ = mmap_result;
   file_offset_ = file_offset;
   region_ = reinterpret_cast<char*>(mmap_result_) + alignment_adjustment;
   region_size_ = mmap_size;