From 70ccb76751ae7077a8ae2ab8b10b1e496fb15372 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Peter=20Bostr=C3=B6m?= Date: Mon, 21 Oct 2024 14:52:29 +1100 Subject: [PATCH] Accept longer Settings::Data structs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This attempts to be somewhat forward-compatible with upcoming additions to the Data struct, most importantly to not lose the client ID if we ever need to downgrade / read data from a future crashpad version. Bug: 42310127 Change-Id: Ic3914fdd8460f4f41e5bb523d5c52361767880dd Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/5915193 Reviewed-by: Mark Mentovai Reviewed-by: Jesse McKenna Commit-Queue: Peter Boström --- client/crash_report_database_test.cc | 44 +++++++ client/settings.cc | 34 +++++- util/file/file_io.cc | 91 +++++++++------ util/file/file_io.h | 79 +++++++------ util/file/file_io_test.cc | 165 ++++++++++++++------------- util/file/file_reader.cc | 34 ++---- util/file/file_reader_test.cc | 24 ++-- 7 files changed, 277 insertions(+), 194 deletions(-) diff --git a/client/crash_report_database_test.cc b/client/crash_report_database_test.cc index a5425fdb..eecd8f0c 100644 --- a/client/crash_report_database_test.cc +++ b/client/crash_report_database_test.cc @@ -979,6 +979,50 @@ TEST_F(CrashReportDatabaseTest, GetReportSize_RightSizeWithAttachments) { sizeof(attachment_2_data)); } +TEST_F(CrashReportDatabaseTest, InitializeFromLargerFileRetainsClientId) { + // Initialize the database for the first time, creating it. + ASSERT_TRUE(db()); + + const base::FilePath settings_path = + path().Append(FILE_PATH_LITERAL("settings.dat")); + EXPECT_FALSE(FileExists(settings_path)); + + Settings* settings = db()->GetSettings(); + ASSERT_TRUE(settings); + EXPECT_TRUE(FileExists(settings_path)); + + UUID client_id; + ASSERT_TRUE(settings->GetClientID(&client_id)); + EXPECT_NE(client_id, UUID()); + + // Close and reopen the database at the same path. + ResetDatabase(); + EXPECT_FALSE(db()); + EXPECT_TRUE(FileExists(settings_path)); + + // Append some data, to ensure that we can open settings even if a future + // version has added additional field to Settings (forwards compatible). + FileWriter settings_writer; + ASSERT_TRUE(settings_writer.Open( + settings_path, FileWriteMode::kReuseOrFail, FilePermissions::kOwnerOnly)); + ASSERT_NE(settings_writer.Seek(0, SEEK_END), 0u); + constexpr uint64_t extra_garbage = 0xBADF00D; + ASSERT_TRUE(settings_writer.Write(&extra_garbage, sizeof(extra_garbage))); + settings_writer.Close(); + + auto db = CrashReportDatabase::InitializeWithoutCreating(path()); + ASSERT_TRUE(db); + + settings = db->GetSettings(); + ASSERT_TRUE(settings); + + // Make sure that the reopened settings retained the original client id and + // wasn't recreated. + UUID reopened_client_id; + ASSERT_TRUE(settings->GetClientID(&reopened_client_id)); + EXPECT_EQ(client_id, reopened_client_id); +} + } // namespace } // namespace test } // namespace crashpad diff --git a/client/settings.cc b/client/settings.cc index 0174c62b..76d77c86 100644 --- a/client/settings.cc +++ b/client/settings.cc @@ -15,6 +15,7 @@ #include "client/settings.h" #include +#include #include @@ -117,6 +118,10 @@ void ScopedLockedFileHandleTraits::Free(FileHandle handle) { struct Settings::Data { static constexpr uint32_t kSettingsMagic = 'CPds'; + + // Version number only used for incompatible changes to Data. Do not change + // this when adding additional fields at the end. Modifying `kSettingsVersion` + // will wipe away the entire struct when reading from other versions. static constexpr uint32_t kSettingsVersion = 1; enum Options : uint32_t { @@ -389,16 +394,33 @@ Settings::ScopedLockedFileHandle Settings::OpenForWritingAndReadSettings( bool Settings::ReadSettings(FileHandle handle, Data* out_data, bool log_read_error) { - if (LoggingSeekFile(handle, 0, SEEK_SET) != 0) + if (LoggingSeekFile(handle, 0, SEEK_SET) != 0) { return false; + } - bool read_result = - log_read_error - ? LoggingReadFileExactly(handle, out_data, sizeof(*out_data)) - : ReadFileExactly(handle, out_data, sizeof(*out_data)); + // This clears `out_data` so that any bytes not read from disk are zero + // initialized. This is expected when reading from an older settings file with + // fewer fields. + memset(out_data, 0, sizeof(*out_data)); - if (!read_result) + const FileOperationResult read_result = + log_read_error ? LoggingReadFileUntil(handle, out_data, sizeof(*out_data)) + : ReadFileUntil(handle, out_data, sizeof(*out_data)); + + if (read_result <= 0) { return false; + } + + // Newer versions of crashpad may add fields to Data, but all versions have + // the data members up to `client_id`. Do not attempt to understand a smaller + // struct read. + const size_t min_size = + offsetof(Data, client_id) + sizeof(out_data->client_id); + if (static_cast(read_result) < min_size) { + LOG(ERROR) << "Settings file too small: minimum " << min_size + << ", observed " << read_result; + return false; + } if (out_data->magic != Data::kSettingsMagic) { LOG(ERROR) << "Settings magic is not " << Data::kSettingsMagic; diff --git a/util/file/file_io.cc b/util/file/file_io.cc index 3e8a6169..c8219f95 100644 --- a/util/file/file_io.cc +++ b/util/file/file_io.cc @@ -14,6 +14,8 @@ #include "util/file/file_io.h" +#include + #include "base/check_op.h" #include "base/logging.h" #include "base/numerics/safe_conversions.h" @@ -22,29 +24,17 @@ namespace crashpad { namespace { -class FileIOReadExactly final : public internal::ReadExactlyInternal { - public: - explicit FileIOReadExactly(FileHandle file) - : ReadExactlyInternal(), file_(file) {} - - FileIOReadExactly(const FileIOReadExactly&) = delete; - FileIOReadExactly& operator=(const FileIOReadExactly&) = delete; - - ~FileIOReadExactly() {} - - private: - // ReadExactlyInternal: - FileOperationResult Read(void* buffer, size_t size, bool can_log) override { - FileOperationResult rv = ReadFile(file_, buffer, size); - if (rv < 0) { - PLOG_IF(ERROR, can_log) << internal::kNativeReadFunctionName; - return -1; - } - return rv; +FileOperationResult FileIORead(FileHandle file, + bool can_log, + void* buffer, + size_t size) { + FileOperationResult rv = ReadFile(file, buffer, size); + if (rv < 0) { + PLOG_IF(ERROR, can_log) << internal::kNativeReadFunctionName; + return -1; } - - FileHandle file_; -}; + return rv; +} class FileIOWriteAll final : public internal::WriteAllInternal { public: @@ -64,19 +54,21 @@ class FileIOWriteAll final : public internal::WriteAllInternal { FileHandle file_; }; -} // namespace - -namespace internal { - -bool ReadExactlyInternal::ReadExactly(void* buffer, size_t size, bool can_log) { +FileOperationResult ReadUntil( + std::function read_function, + void* buffer, + size_t size) { + // Ensure bytes read fit within int32_t::max to make sure that they also fit + // into FileOperationResult on all platforms. + DCHECK_LE(size, size_t{std::numeric_limits::max()}); uintptr_t buffer_int = reinterpret_cast(buffer); size_t total_bytes = 0; size_t remaining = size; while (remaining > 0) { - FileOperationResult bytes_read = - Read(reinterpret_cast(buffer_int), remaining, can_log); + const FileOperationResult bytes_read = + read_function(reinterpret_cast(buffer_int), remaining); if (bytes_read < 0) { - return false; + return bytes_read; } DCHECK_LE(static_cast(bytes_read), remaining); @@ -89,10 +81,27 @@ bool ReadExactlyInternal::ReadExactly(void* buffer, size_t size, bool can_log) { remaining -= bytes_read; total_bytes += bytes_read; } + return total_bytes; +} - if (total_bytes != size) { +} // namespace + +namespace internal { + +bool ReadExactly( + std::function read_function, + bool can_log, + void* buffer, + size_t size) { + const FileOperationResult result = + ReadUntil(std::bind_front(read_function, can_log), buffer, size); + if (result < 0) { + return false; + } + + if (static_cast(result) != size) { LOG_IF(ERROR, can_log) << "ReadExactly: expected " << size << ", observed " - << total_bytes; + << result; return false; } @@ -121,13 +130,23 @@ bool WriteAllInternal::WriteAll(const void* buffer, size_t size) { } // namespace internal bool ReadFileExactly(FileHandle file, void* buffer, size_t size) { - FileIOReadExactly read_exactly(file); - return read_exactly.ReadExactly(buffer, size, false); + return internal::ReadExactly( + std::bind_front(&FileIORead, file), false, buffer, size); +} + +FileOperationResult ReadFileUntil(FileHandle file, void* buffer, size_t size) { + return ReadUntil(std::bind_front(&FileIORead, file, false), buffer, size); } bool LoggingReadFileExactly(FileHandle file, void* buffer, size_t size) { - FileIOReadExactly read_exactly(file); - return read_exactly.ReadExactly(buffer, size, true); + return internal::ReadExactly( + std::bind_front(&FileIORead, file), true, buffer, size); +} + +FileOperationResult LoggingReadFileUntil(FileHandle file, + void* buffer, + size_t size) { + return ReadUntil(std::bind_front(&FileIORead, file, true), buffer, size); } bool WriteFile(FileHandle file, const void* buffer, size_t size) { diff --git a/util/file/file_io.h b/util/file/file_io.h index 607cd1eb..c0dc9e16 100644 --- a/util/file/file_io.h +++ b/util/file/file_io.h @@ -17,6 +17,7 @@ #include +#include #include #include "build/build_config.h" @@ -162,30 +163,11 @@ constexpr char kNativeWriteFunctionName[] = "WriteFile"; //! intended to be used more generally. Use ReadFileExactly(), //! LoggingReadFileExactly(), CheckedReadFileExactly(), or //! FileReaderInterface::ReadExactly() instead. -class ReadExactlyInternal { - public: - ReadExactlyInternal(const ReadExactlyInternal&) = delete; - ReadExactlyInternal& operator=(const ReadExactlyInternal&) = delete; - - //! \brief Calls Read(), retrying following a short read, ensuring that - //! exactly \a size bytes are read. - //! - //! \return `true` on success. `false` if the underlying Read() fails or if - //! fewer than \a size bytes were read. When returning `false`, if \a - //! can_log is `true`, logs a message. - bool ReadExactly(void* buffer, size_t size, bool can_log); - - protected: - ReadExactlyInternal() {} - ~ReadExactlyInternal() {} - - private: - //! \brief Wraps a read operation, such as ReadFile(). - //! - //! \return The number of bytes read and placed into \a buffer, or `-1` on - //! error. When returning `-1`, if \a can_log is `true`, logs a message. - virtual FileOperationResult Read(void* buffer, size_t size, bool can_log) = 0; -}; +bool ReadExactly( + std::function read_function, + bool can_log, + void* buffer, + size_t size); //! \brief The internal implementation of WriteFile() and its wrappers. //! @@ -252,7 +234,9 @@ FileOperationResult NativeWriteFile(FileHandle file, //! //! \sa WriteFile //! \sa ReadFileExactly +//! \sa ReadFileUntil //! \sa LoggingReadFileExactly +//! \sa LoggingReadFileUntil //! \sa CheckedReadFileExactly //! \sa CheckedReadFileAtEOF FileOperationResult ReadFile(FileHandle file, void* buffer, size_t size); @@ -273,33 +257,62 @@ FileOperationResult ReadFile(FileHandle file, void* buffer, size_t size); bool WriteFile(FileHandle file, const void* buffer, size_t size); //! \brief Wraps ReadFile(), retrying following a short read, ensuring that -//! exactly \a size bytes are read. +//! exactly \a size bytes are read. Does not log on failure. //! -//! \return `true` on success. If the underlying ReadFile() fails, or if fewer -//! than \a size bytes were read, this function logs a message and -//! returns `false`. +//! \return `true` on success. Returns `false` if the underlying ReadFile() +//! fails or if fewer than \a size bytes were read. //! //! \sa LoggingWriteFile //! \sa ReadFile +//! \sa ReadFileUntil //! \sa LoggingReadFileExactly +//! \sa LoggingReadFileUntil //! \sa CheckedReadFileExactly //! \sa CheckedReadFileAtEOF bool ReadFileExactly(FileHandle file, void* buffer, size_t size); -//! \brief Wraps ReadFile(), retrying following a short read, ensuring that -//! exactly \a size bytes are read. +//! \brief Wraps ReadFile(), retrying following a short read. Does not log on +//! failure. //! -//! \return `true` on success. If the underlying ReadFile() fails, or if fewer -//! than \a size bytes were read, this function logs a message and -//! returns `false`. +//! \returns The number of bytes read or `-1` if the underlying ReadFile() +//! fails. +//! +//! \sa ReadFile +//! \sa ReadFileExactly +//! \sa LoggingReadFileExactly +//! \sa LoggingReadFileUntil +FileOperationResult ReadFileUntil(FileHandle file, void* buffer, size_t size); + +//! \brief Wraps ReadFile(), retrying following a short read, ensuring that +//! exactly \a size bytes are read. Logs an error on failure. +//! +//! \return `true` on success. Returns `false` if the underlying ReadFile() +//! fails or if fewer than \a size bytes were read. //! //! \sa LoggingWriteFile //! \sa ReadFile //! \sa ReadFileExactly +//! \sa ReadFileUntil +//! \sa LoggingReadFileUntil //! \sa CheckedReadFileExactly //! \sa CheckedReadFileAtEOF bool LoggingReadFileExactly(FileHandle file, void* buffer, size_t size); +//! \brief Wraps ReadFile(), retrying following a short read. Logs an error on +//! failure. +//! +//! \returns The number of bytes read or `-1` if the underlying ReadFile() +//! fails. +//! +//! \sa ReadFileExactly +//! \sa ReadFileUntil +//! \sa LoggingReadFileExactly +//! \sa CheckedReadFileExactly +//! \sa CheckedReadFileAtEOF +FileOperationResult LoggingReadFileUntil(FileHandle file, + void* buffer, + size_t size); + //! \brief Wraps WriteFile(), ensuring that exactly \a size bytes are written. //! //! \return `true` on success. If the underlying WriteFile() fails, or if fewer diff --git a/util/file/file_io_test.cc b/util/file/file_io_test.cc index 253f6a03..a0c79f9a 100644 --- a/util/file/file_io_test.cc +++ b/util/file/file_io_test.cc @@ -16,6 +16,7 @@ #include +#include #include #include #include @@ -39,202 +40,204 @@ using testing::_; using testing::InSequence; using testing::Return; -class MockReadExactly : public internal::ReadExactlyInternal { +class MockReadExactly { public: - MockReadExactly() : ReadExactlyInternal() {} + MockReadExactly() = default; MockReadExactly(const MockReadExactly&) = delete; MockReadExactly& operator=(const MockReadExactly&) = delete; - ~MockReadExactly() {} + ~MockReadExactly() = default; // Since it’s more convenient for the test to use uintptr_t than void*, // ReadExactlyInt() and ReadInt() adapt the types. - bool ReadExactlyInt(uintptr_t data, size_t size, bool can_log) { - return ReadExactly(reinterpret_cast(data), size, can_log); + bool ReadExactlyInt(bool can_log, uintptr_t data, size_t size) { + return internal::ReadExactly(std::bind_front(&MockReadExactly::Read, this), + can_log, + reinterpret_cast(data), + size); } - MOCK_METHOD(FileOperationResult, ReadInt, (uintptr_t, size_t, bool)); + MOCK_METHOD(FileOperationResult, ReadInt, (bool, uintptr_t, size_t)); - // ReadExactlyInternal: - FileOperationResult Read(void* data, size_t size, bool can_log) { - return ReadInt(reinterpret_cast(data), size, can_log); + FileOperationResult Read(bool can_log, void* data, size_t size) { + return ReadInt(can_log, reinterpret_cast(data), size); } }; TEST(FileIO, ReadExactly_Zero) { MockReadExactly read_exactly; InSequence in_sequence; - EXPECT_CALL(read_exactly, ReadInt(_, _, false)).Times(0); - EXPECT_TRUE(read_exactly.ReadExactlyInt(100, 0, false)); + EXPECT_CALL(read_exactly, ReadInt(false, _, _)).Times(0); + EXPECT_TRUE(read_exactly.ReadExactlyInt(false, 100, 0)); } TEST(FileIO, ReadExactly_SingleSmallSuccess) { MockReadExactly read_exactly; InSequence in_sequence; - EXPECT_CALL(read_exactly, ReadInt(1000, 1, false)).WillOnce(Return(1)); - EXPECT_TRUE(read_exactly.ReadExactlyInt(1000, 1, false)); + EXPECT_CALL(read_exactly, ReadInt(false, 1000, 1)).WillOnce(Return(1)); + EXPECT_TRUE(read_exactly.ReadExactlyInt(false, 1000, 1)); } TEST(FileIO, ReadExactly_SingleSmallSuccessCanLog) { MockReadExactly read_exactly; InSequence in_sequence; - EXPECT_CALL(read_exactly, ReadInt(1000, 1, true)).WillOnce(Return(1)); - EXPECT_TRUE(read_exactly.ReadExactlyInt(1000, 1, true)); + EXPECT_CALL(read_exactly, ReadInt(true, 1000, 1)).WillOnce(Return(1)); + EXPECT_TRUE(read_exactly.ReadExactlyInt(true, 1000, 1)); } TEST(FileIO, ReadExactly_SingleSmallFailure) { MockReadExactly read_exactly; InSequence in_sequence; - EXPECT_CALL(read_exactly, ReadInt(1000, 1, false)).WillOnce(Return(-1)); - EXPECT_FALSE(read_exactly.ReadExactlyInt(1000, 1, false)); + EXPECT_CALL(read_exactly, ReadInt(false, 1000, 1)).WillOnce(Return(-1)); + EXPECT_FALSE(read_exactly.ReadExactlyInt(false, 1000, 1)); } TEST(FileIO, ReadExactly_SingleSmallFailureCanLog) { MockReadExactly read_exactly; InSequence in_sequence; - EXPECT_CALL(read_exactly, ReadInt(1000, 1, true)).WillOnce(Return(-1)); - EXPECT_FALSE(read_exactly.ReadExactlyInt(1000, 1, true)); + EXPECT_CALL(read_exactly, ReadInt(true, 1000, 1)).WillOnce(Return(-1)); + EXPECT_FALSE(read_exactly.ReadExactlyInt(true, 1000, 1)); } TEST(FileIO, ReadExactly_DoubleSmallSuccess) { MockReadExactly read_exactly; InSequence in_sequence; - EXPECT_CALL(read_exactly, ReadInt(0x1000, 2, false)).WillOnce(Return(1)); - EXPECT_CALL(read_exactly, ReadInt(0x1001, 1, false)).WillOnce(Return(1)); - EXPECT_TRUE(read_exactly.ReadExactlyInt(0x1000, 2, false)); + EXPECT_CALL(read_exactly, ReadInt(false, 0x1000, 2)).WillOnce(Return(1)); + EXPECT_CALL(read_exactly, ReadInt(false, 0x1001, 1)).WillOnce(Return(1)); + EXPECT_TRUE(read_exactly.ReadExactlyInt(false, 0x1000, 2)); } TEST(FileIO, ReadExactly_DoubleSmallShort) { MockReadExactly read_exactly; InSequence in_sequence; - EXPECT_CALL(read_exactly, ReadInt(0x20000, 2, false)).WillOnce(Return(1)); - EXPECT_CALL(read_exactly, ReadInt(0x20001, 1, false)).WillOnce(Return(0)); - EXPECT_FALSE(read_exactly.ReadExactlyInt(0x20000, 2, false)); + EXPECT_CALL(read_exactly, ReadInt(false, 0x20000, 2)).WillOnce(Return(1)); + EXPECT_CALL(read_exactly, ReadInt(false, 0x20001, 1)).WillOnce(Return(0)); + EXPECT_FALSE(read_exactly.ReadExactlyInt(false, 0x20000, 2)); } TEST(FileIO, ReadExactly_DoubleSmallShortCanLog) { MockReadExactly read_exactly; InSequence in_sequence; - EXPECT_CALL(read_exactly, ReadInt(0x20000, 2, true)).WillOnce(Return(1)); - EXPECT_CALL(read_exactly, ReadInt(0x20001, 1, true)).WillOnce(Return(0)); - EXPECT_FALSE(read_exactly.ReadExactlyInt(0x20000, 2, true)); + EXPECT_CALL(read_exactly, ReadInt(true, 0x20000, 2)).WillOnce(Return(1)); + EXPECT_CALL(read_exactly, ReadInt(true, 0x20001, 1)).WillOnce(Return(0)); + EXPECT_FALSE(read_exactly.ReadExactlyInt(true, 0x20000, 2)); } TEST(FileIO, ReadExactly_Medium) { MockReadExactly read_exactly; InSequence in_sequence; - EXPECT_CALL(read_exactly, ReadInt(0x80000000, 0x20000000, false)) + EXPECT_CALL(read_exactly, ReadInt(false, 0x80000000, 0x20000000)) .WillOnce(Return(0x10000000)); - EXPECT_CALL(read_exactly, ReadInt(0x90000000, 0x10000000, false)) + EXPECT_CALL(read_exactly, ReadInt(false, 0x90000000, 0x10000000)) .WillOnce(Return(0x8000000)); - EXPECT_CALL(read_exactly, ReadInt(0x98000000, 0x8000000, false)) + EXPECT_CALL(read_exactly, ReadInt(false, 0x98000000, 0x8000000)) .WillOnce(Return(0x4000000)); - EXPECT_CALL(read_exactly, ReadInt(0x9c000000, 0x4000000, false)) + EXPECT_CALL(read_exactly, ReadInt(false, 0x9c000000, 0x4000000)) .WillOnce(Return(0x2000000)); - EXPECT_CALL(read_exactly, ReadInt(0x9e000000, 0x2000000, false)) + EXPECT_CALL(read_exactly, ReadInt(false, 0x9e000000, 0x2000000)) .WillOnce(Return(0x1000000)); - EXPECT_CALL(read_exactly, ReadInt(0x9f000000, 0x1000000, false)) + EXPECT_CALL(read_exactly, ReadInt(false, 0x9f000000, 0x1000000)) .WillOnce(Return(0x800000)); - EXPECT_CALL(read_exactly, ReadInt(0x9f800000, 0x800000, false)) + EXPECT_CALL(read_exactly, ReadInt(false, 0x9f800000, 0x800000)) .WillOnce(Return(0x400000)); - EXPECT_CALL(read_exactly, ReadInt(0x9fc00000, 0x400000, false)) + EXPECT_CALL(read_exactly, ReadInt(false, 0x9fc00000, 0x400000)) .WillOnce(Return(0x200000)); - EXPECT_CALL(read_exactly, ReadInt(0x9fe00000, 0x200000, false)) + EXPECT_CALL(read_exactly, ReadInt(false, 0x9fe00000, 0x200000)) .WillOnce(Return(0x100000)); - EXPECT_CALL(read_exactly, ReadInt(0x9ff00000, 0x100000, false)) + EXPECT_CALL(read_exactly, ReadInt(false, 0x9ff00000, 0x100000)) .WillOnce(Return(0x80000)); - EXPECT_CALL(read_exactly, ReadInt(0x9ff80000, 0x80000, false)) + EXPECT_CALL(read_exactly, ReadInt(false, 0x9ff80000, 0x80000)) .WillOnce(Return(0x40000)); - EXPECT_CALL(read_exactly, ReadInt(0x9ffc0000, 0x40000, false)) + EXPECT_CALL(read_exactly, ReadInt(false, 0x9ffc0000, 0x40000)) .WillOnce(Return(0x20000)); - EXPECT_CALL(read_exactly, ReadInt(0x9ffe0000, 0x20000, false)) + EXPECT_CALL(read_exactly, ReadInt(false, 0x9ffe0000, 0x20000)) .WillOnce(Return(0x10000)); - EXPECT_CALL(read_exactly, ReadInt(0x9fff0000, 0x10000, false)) + EXPECT_CALL(read_exactly, ReadInt(false, 0x9fff0000, 0x10000)) .WillOnce(Return(0x8000)); - EXPECT_CALL(read_exactly, ReadInt(0x9fff8000, 0x8000, false)) + EXPECT_CALL(read_exactly, ReadInt(false, 0x9fff8000, 0x8000)) .WillOnce(Return(0x4000)); - EXPECT_CALL(read_exactly, ReadInt(0x9fffc000, 0x4000, false)) + EXPECT_CALL(read_exactly, ReadInt(false, 0x9fffc000, 0x4000)) .WillOnce(Return(0x2000)); - EXPECT_CALL(read_exactly, ReadInt(0x9fffe000, 0x2000, false)) + EXPECT_CALL(read_exactly, ReadInt(false, 0x9fffe000, 0x2000)) .WillOnce(Return(0x1000)); - EXPECT_CALL(read_exactly, ReadInt(0x9ffff000, 0x1000, false)) + EXPECT_CALL(read_exactly, ReadInt(false, 0x9ffff000, 0x1000)) .WillOnce(Return(0x800)); - EXPECT_CALL(read_exactly, ReadInt(0x9ffff800, 0x800, false)) + EXPECT_CALL(read_exactly, ReadInt(false, 0x9ffff800, 0x800)) .WillOnce(Return(0x400)); - EXPECT_CALL(read_exactly, ReadInt(0x9ffffc00, 0x400, false)) + EXPECT_CALL(read_exactly, ReadInt(false, 0x9ffffc00, 0x400)) .WillOnce(Return(0x200)); - EXPECT_CALL(read_exactly, ReadInt(0x9ffffe00, 0x200, false)) + EXPECT_CALL(read_exactly, ReadInt(false, 0x9ffffe00, 0x200)) .WillOnce(Return(0x100)); - EXPECT_CALL(read_exactly, ReadInt(0x9fffff00, 0x100, false)) + EXPECT_CALL(read_exactly, ReadInt(false, 0x9fffff00, 0x100)) .WillOnce(Return(0x80)); - EXPECT_CALL(read_exactly, ReadInt(0x9fffff80, 0x80, false)) + EXPECT_CALL(read_exactly, ReadInt(false, 0x9fffff80, 0x80)) .WillOnce(Return(0x40)); - EXPECT_CALL(read_exactly, ReadInt(0x9fffffc0, 0x40, false)) + EXPECT_CALL(read_exactly, ReadInt(false, 0x9fffffc0, 0x40)) .WillOnce(Return(0x20)); - EXPECT_CALL(read_exactly, ReadInt(0x9fffffe0, 0x20, false)) + EXPECT_CALL(read_exactly, ReadInt(false, 0x9fffffe0, 0x20)) .WillOnce(Return(0x10)); - EXPECT_CALL(read_exactly, ReadInt(0x9ffffff0, 0x10, false)) + EXPECT_CALL(read_exactly, ReadInt(false, 0x9ffffff0, 0x10)) .WillOnce(Return(0x8)); - EXPECT_CALL(read_exactly, ReadInt(0x9ffffff8, 0x8, false)) + EXPECT_CALL(read_exactly, ReadInt(false, 0x9ffffff8, 0x8)) .WillOnce(Return(0x4)); - EXPECT_CALL(read_exactly, ReadInt(0x9ffffffc, 0x4, false)) + EXPECT_CALL(read_exactly, ReadInt(false, 0x9ffffffc, 0x4)) .WillOnce(Return(0x2)); - EXPECT_CALL(read_exactly, ReadInt(0x9ffffffe, 0x2, false)) + EXPECT_CALL(read_exactly, ReadInt(false, 0x9ffffffe, 0x2)) .WillOnce(Return(0x1)); - EXPECT_CALL(read_exactly, ReadInt(0x9fffffff, 0x1, false)) + EXPECT_CALL(read_exactly, ReadInt(false, 0x9fffffff, 0x1)) .WillOnce(Return(0x1)); - EXPECT_TRUE(read_exactly.ReadExactlyInt(0x80000000, 0x20000000, false)); + EXPECT_TRUE(read_exactly.ReadExactlyInt(false, 0x80000000, 0x20000000)); } TEST(FileIO, ReadExactly_LargeSuccess) { MockReadExactly read_exactly; InSequence in_sequence; - constexpr size_t max = std::numeric_limits::max(); - constexpr size_t increment = std::numeric_limits::max(); - EXPECT_CALL(read_exactly, ReadInt(0, max, false)).WillOnce(Return(increment)); - EXPECT_CALL(read_exactly, ReadInt(increment, max - increment, false)) + constexpr size_t max = std::numeric_limits::max(); + constexpr size_t increment = max / 2; + EXPECT_CALL(read_exactly, ReadInt(false, 0, max)).WillOnce(Return(increment)); + EXPECT_CALL(read_exactly, ReadInt(false, increment, max - increment)) .WillOnce(Return(increment)); - EXPECT_CALL(read_exactly, ReadInt(2 * increment, 1, false)) + EXPECT_CALL(read_exactly, ReadInt(false, 2 * increment, 1)) .WillOnce(Return(1)); - EXPECT_TRUE(read_exactly.ReadExactlyInt(0, max, false)); + EXPECT_TRUE(read_exactly.ReadExactlyInt(false, 0, max)); } TEST(FileIO, ReadExactly_LargeShort) { MockReadExactly read_exactly; InSequence in_sequence; - EXPECT_CALL(read_exactly, ReadInt(0, 0xffffffff, false)) - .WillOnce(Return(0x7fffffff)); - EXPECT_CALL(read_exactly, ReadInt(0x7fffffff, 0x80000000, false)) + EXPECT_CALL(read_exactly, ReadInt(false, 0, 0x7fffffff)) + .WillOnce(Return(0x3fffffff)); + EXPECT_CALL(read_exactly, ReadInt(false, 0x3fffffff, 0x40000000)) .WillOnce(Return(0x10000000)); - EXPECT_CALL(read_exactly, ReadInt(0x8fffffff, 0x70000000, false)) + EXPECT_CALL(read_exactly, ReadInt(false, 0x4fffffff, 0x30000000)) .WillOnce(Return(0)); - EXPECT_FALSE(read_exactly.ReadExactlyInt(0, 0xffffffff, false)); + EXPECT_FALSE(read_exactly.ReadExactlyInt(false, 0, 0x7fffffff)); } TEST(FileIO, ReadExactly_LargeFailure) { MockReadExactly read_exactly; InSequence in_sequence; - EXPECT_CALL(read_exactly, ReadInt(0, 0xffffffff, false)) - .WillOnce(Return(0x7fffffff)); - EXPECT_CALL(read_exactly, ReadInt(0x7fffffff, 0x80000000, false)) + EXPECT_CALL(read_exactly, ReadInt(false, 0, 0x7fffffff)) + .WillOnce(Return(0x3fffffff)); + EXPECT_CALL(read_exactly, ReadInt(false, 0x3fffffff, 0x40000000)) .WillOnce(Return(-1)); - EXPECT_FALSE(read_exactly.ReadExactlyInt(0, 0xffffffff, false)); + EXPECT_FALSE(read_exactly.ReadExactlyInt(false, 0, 0x7fffffff)); } TEST(FileIO, ReadExactly_TripleMax) { MockReadExactly read_exactly; InSequence in_sequence; - constexpr size_t max = std::numeric_limits::max(); - constexpr size_t increment = - std::numeric_limits::type>::max(); - EXPECT_CALL(read_exactly, ReadInt(0, max, false)).WillOnce(Return(increment)); - EXPECT_CALL(read_exactly, ReadInt(increment, max - increment, false)) + // ReadExactly supports at most int32_t bytes. + constexpr size_t max = std::numeric_limits::max(); + constexpr size_t increment = max / 2; + EXPECT_CALL(read_exactly, ReadInt(false, 0, max)).WillOnce(Return(increment)); + EXPECT_CALL(read_exactly, ReadInt(false, increment, max - increment)) .WillOnce(Return(increment)); - EXPECT_CALL(read_exactly, ReadInt(2 * increment, 1, false)) + EXPECT_CALL(read_exactly, ReadInt(false, 2 * increment, 1)) .WillOnce(Return(1)); - EXPECT_TRUE(read_exactly.ReadExactlyInt(0, max, false)); + EXPECT_TRUE(read_exactly.ReadExactlyInt(false, 0, max)); } class MockWriteAll : public internal::WriteAllInternal { diff --git a/util/file/file_reader.cc b/util/file/file_reader.cc index f8fcf818..6c30e524 100644 --- a/util/file/file_reader.cc +++ b/util/file/file_reader.cc @@ -21,33 +21,15 @@ namespace crashpad { -namespace { - -class FileReaderReadExactly final : public internal::ReadExactlyInternal { - public: - explicit FileReaderReadExactly(FileReaderInterface* file_reader) - : ReadExactlyInternal(), file_reader_(file_reader) {} - - FileReaderReadExactly(const FileReaderReadExactly&) = delete; - FileReaderReadExactly& operator=(const FileReaderReadExactly&) = delete; - - ~FileReaderReadExactly() {} - - private: - // ReadExactlyInternal: - FileOperationResult Read(void* buffer, size_t size, bool can_log) override { - DCHECK(can_log); - return file_reader_->Read(buffer, size); - } - - FileReaderInterface* file_reader_; // weak -}; - -} // namespace - bool FileReaderInterface::ReadExactly(void* data, size_t size) { - FileReaderReadExactly read_exactly(this); - return read_exactly.ReadExactly(data, size, true); + return internal::ReadExactly( + [this](bool can_log, void* buffer, size_t size) { + DCHECK(can_log); + return Read(buffer, size); + }, + true, + data, + size); } WeakFileHandleFileReader::WeakFileHandleFileReader(FileHandle file_handle) diff --git a/util/file/file_reader_test.cc b/util/file/file_reader_test.cc index c3e9486e..e68959f0 100644 --- a/util/file/file_reader_test.cc +++ b/util/file/file_reader_test.cc @@ -156,8 +156,8 @@ TEST(FileReader, ReadExactly_Medium) { TEST(FileReader, ReadExactly_LargeSuccess) { MockFileReader file_reader; InSequence in_sequence; - constexpr size_t max = std::numeric_limits::max(); - constexpr size_t increment = std::numeric_limits::max(); + constexpr size_t max = std::numeric_limits::max(); + constexpr size_t increment = max / 2; EXPECT_CALL(file_reader, ReadInt(0, max)).WillOnce(Return(increment)); EXPECT_CALL(file_reader, ReadInt(increment, max - increment)) .WillOnce(Return(increment)); @@ -169,30 +169,30 @@ TEST(FileReader, ReadExactly_LargeSuccess) { TEST(FileReader, ReadExactly_LargeShort) { MockFileReader file_reader; InSequence in_sequence; - EXPECT_CALL(file_reader, ReadInt(0, 0xffffffff)).WillOnce(Return(0x7fffffff)); - EXPECT_CALL(file_reader, ReadInt(0x7fffffff, 0x80000000)) + EXPECT_CALL(file_reader, ReadInt(0, 0x7fffffff)).WillOnce(Return(0x3fffffff)); + EXPECT_CALL(file_reader, ReadInt(0x3fffffff, 0x40000000)) .WillOnce(Return(0x10000000)); - EXPECT_CALL(file_reader, ReadInt(0x8fffffff, 0x70000000)).WillOnce(Return(0)); + EXPECT_CALL(file_reader, ReadInt(0x4fffffff, 0x30000000)).WillOnce(Return(0)); EXPECT_CALL(file_reader, Seek(_, _)).Times(0); - EXPECT_FALSE(file_reader.ReadExactlyInt(0, 0xffffffff)); + EXPECT_FALSE(file_reader.ReadExactlyInt(0, 0x7fffffff)); } TEST(FileReader, ReadExactly_LargeFailure) { MockFileReader file_reader; InSequence in_sequence; - EXPECT_CALL(file_reader, ReadInt(0, 0xffffffff)).WillOnce(Return(0x7fffffff)); - EXPECT_CALL(file_reader, ReadInt(0x7fffffff, 0x80000000)) + EXPECT_CALL(file_reader, ReadInt(0, 0x7fffffff)).WillOnce(Return(0x3fffffff)); + EXPECT_CALL(file_reader, ReadInt(0x3fffffff, 0x40000000)) .WillOnce(Return(-1)); EXPECT_CALL(file_reader, Seek(_, _)).Times(0); - EXPECT_FALSE(file_reader.ReadExactlyInt(0, 0xffffffff)); + EXPECT_FALSE(file_reader.ReadExactlyInt(0, 0x7fffffff)); } TEST(FileReader, ReadExactly_TripleMax) { MockFileReader file_reader; InSequence in_sequence; - constexpr size_t max = std::numeric_limits::max(); - constexpr size_t increment = - std::numeric_limits::type>::max(); + // ReadExactly supports at most int32_t bytes. + constexpr size_t max = std::numeric_limits::max(); + constexpr size_t increment = max / 2; EXPECT_CALL(file_reader, ReadInt(0, max)).WillOnce(Return(increment)); EXPECT_CALL(file_reader, ReadInt(increment, max - increment)) .WillOnce(Return(increment));