From 5a0dcd8114afa3b0f93bf77a6c989f539555186c Mon Sep 17 00:00:00 2001 From: Tim Barron Date: Thu, 26 May 2022 18:19:40 +0000 Subject: Fix NPE caused by handling GetFileSize return value. FileBackedVector wasn't properly checking the return value of GetFileSize when deciding whether or not it needed to grow the underlying file and remap. If GetFileSize failed due to an IO error, then FileBackedVector would conclude (incorrectly) that growing wasn't necessary and would attempt to access the mmapping at the requested index - which could result in either an NPE (if the vector was empty) or an invalid memory access (if the requested index exceeded the actual size of the mmapping). See b/232273174#comment44 for more details. This is a cherrypick of cl/451040413. Test: Builds Change-Id: I7ae0e3119126483c23a85dae3bd7a84712357209 --- icing/file/file-backed-vector.h | 5 ++++- icing/file/file-backed-vector_test.cc | 21 +++++++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/icing/file/file-backed-vector.h b/icing/file/file-backed-vector.h index 00bdc7e..7e42e32 100644 --- a/icing/file/file-backed-vector.h +++ b/icing/file/file-backed-vector.h @@ -586,8 +586,11 @@ libtextclassifier3::Status FileBackedVector::GrowIfNecessary( } int64_t current_file_size = filesystem_->GetFileSize(file_path_.c_str()); - int64_t least_file_size_needed = sizeof(Header) + num_elements * sizeof(T); + if (current_file_size == Filesystem::kBadFileSize) { + return absl_ports::InternalError("Unable to retrieve file size."); + } + int64_t least_file_size_needed = sizeof(Header) + num_elements * sizeof(T); if (least_file_size_needed <= current_file_size) { // Our underlying file can hold the target num_elements cause we've grown // before diff --git a/icing/file/file-backed-vector_test.cc b/icing/file/file-backed-vector_test.cc index 54f9ef5..ed94fa5 100644 --- a/icing/file/file-backed-vector_test.cc +++ b/icing/file/file-backed-vector_test.cc @@ -677,6 +677,27 @@ TEST_F(FileBackedVectorTest, RemapFailureStillValidInstance) { EXPECT_THAT(vector->Get(kResizingIndex / 2), IsOkAndHolds(Pointee(Eq(9)))); } +TEST_F(FileBackedVectorTest, BadFileSizeDuringGrowReturnsError) { + auto mock_filesystem = std::make_unique(); + ICING_ASSERT_OK_AND_ASSIGN( + std::unique_ptr> vector, + FileBackedVector::Create( + *mock_filesystem, file_path_, + MemoryMappedFile::Strategy::READ_WRITE_AUTO_SYNC)); + + // At first, the vector is empty and has no mapping established. The first Set + // call will cause a Grow. + // During Grow, we will attempt to check the underlying file size to see if + // growing is actually necessary. Return an error on the call to GetFileSize. + ON_CALL(*mock_filesystem, GetFileSize(A())) + .WillByDefault(Return(Filesystem::kBadFileSize)); + + // We should fail gracefully and return an INTERNAL error to indicate that + // there was an issue retrieving the file size. + EXPECT_THAT(vector->Set(0, 7), + StatusIs(libtextclassifier3::StatusCode::INTERNAL)); +} + } // namespace } // namespace lib -- cgit v1.2.3