Skip to content

Commit

Permalink
Accept longer Settings::Data structs
Browse files Browse the repository at this point in the history 
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 <[email protected]>
Reviewed-by: Jesse McKenna <[email protected]>
Commit-Queue: Peter Boström <[email protected]>
  • Loading branch information
@pbos
pbos authored and Crashpad LUCI CQ committed Oct 22, 2024
1 parent 41c74f5 commit 70ccb76
Show file tree
Hide file tree
Showing 7 changed files with 276 additions and 193 deletions.
44 changes: 44 additions & 0 deletions client/crash_report_database_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
34 changes: 28 additions & 6 deletions client/settings.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include "client/settings.h"

#include <stdint.h>
#include <string.h>

#include <limits>

Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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;
}

// 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));

bool read_result =
log_read_error
? LoggingReadFileExactly(handle, out_data, sizeof(*out_data))
: ReadFileExactly(handle, out_data, sizeof(*out_data));
const FileOperationResult read_result =
log_read_error ? LoggingReadFileUntil(handle, out_data, sizeof(*out_data))
: ReadFileUntil(handle, out_data, sizeof(*out_data));

if (!read_result)
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<size_t>(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;
Expand Down
91 changes: 55 additions & 36 deletions util/file/file_io.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@

#include "util/file/file_io.h"

#include <functional>

#include "base/check_op.h"
#include "base/logging.h"
#include "base/numerics/safe_conversions.h"
Expand All @@ -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:
Expand All @@ -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<FileOperationResult(void*, size_t)> 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<int32_t>::max()});
uintptr_t buffer_int = reinterpret_cast<uintptr_t>(buffer);
size_t total_bytes = 0;
size_t remaining = size;
while (remaining > 0) {
FileOperationResult bytes_read =
Read(reinterpret_cast<char*>(buffer_int), remaining, can_log);
const FileOperationResult bytes_read =
read_function(reinterpret_cast<char*>(buffer_int), remaining);
if (bytes_read < 0) {
return false;
return bytes_read;
}

DCHECK_LE(static_cast<size_t>(bytes_read), remaining);
Expand All @@ -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;
}

} // namespace

namespace internal {

bool ReadExactly(
std::function<FileOperationResult(bool, void*, size_t)> 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 (total_bytes != size) {
if (static_cast<size_t>(result) != size) {
LOG_IF(ERROR, can_log) << "ReadExactly: expected " << size << ", observed "
<< total_bytes;
<< result;
return false;
}

Expand Down Expand Up @@ -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) {
Expand Down
77 changes: 45 additions & 32 deletions util/file/file_io.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

#include <sys/types.h>

#include <functional>
#include <string>

#include "build/build_config.h"
Expand Down Expand Up @@ -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<FileOperationResult(bool, void*, size_t)> read_function,
bool can_log,
void* buffer,
size_t size);

//! \brief The internal implementation of WriteFile() and its wrappers.
//!
Expand Down Expand Up @@ -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);
Expand All @@ -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. Does not log on
//! failure.
//!
//! \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.
//! exactly \a size bytes are read. Logs an error 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 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
Expand Down
Loading

0 comments on commit 70ccb76

Please sign in to comment.