diff --git a/client/client.gyp b/client/client.gyp index 5faaf036..0c39f1e0 100644 --- a/client/client.gyp +++ b/client/client.gyp @@ -54,10 +54,6 @@ '-lrpcrt4.lib', ], }, - 'sources!': [ - # Port to Win https://code.google.com/p/crashpad/issues/detail?id=13 - 'settings.cc', - ], }], ], 'direct_dependent_settings': { diff --git a/client/client_test.gyp b/client/client_test.gyp index 71341620..cc6893f9 100644 --- a/client/client_test.gyp +++ b/client/client_test.gyp @@ -39,14 +39,6 @@ 'simple_string_dictionary_test.cc', 'simulate_crash_mac_test.cc', ], - 'conditions': [ - ['OS=="win"', { - 'sources!': [ - # Port to Win https://code.google.com/p/crashpad/issues/detail?id=13 - 'settings_test.cc', - ], - }], - ], }, ], } diff --git a/client/crash_report_database_win.cc b/client/crash_report_database_win.cc index 6dbaf0ea..894f9226 100644 --- a/client/crash_report_database_win.cc +++ b/client/crash_report_database_win.cc @@ -14,7 +14,6 @@ #include "client/crash_report_database.h" -#include #include #include #include @@ -573,12 +572,9 @@ OperationStatus CrashReportDatabaseWin::PrepareNewCrashReport( NewReport** report) { INITIALIZATION_STATE_DCHECK_VALID(initialized_); - ::UUID system_uuid; - if (UuidCreate(&system_uuid) != RPC_S_OK) - return kFileSystemError; - scoped_ptr new_report(new NewReport()); - new_report->uuid.InitializeFromSystemUUID(&system_uuid); + if (!new_report->uuid.InitializeWithNew()) + return kFileSystemError; new_report->path = base_dir_.Append(kReportsDirectory) .Append(new_report->uuid.ToString16() + L"." + kCrashReportFileExtension); diff --git a/client/settings.cc b/client/settings.cc index aa2c4f8f..d1b35f1a 100644 --- a/client/settings.cc +++ b/client/settings.cc @@ -16,10 +16,6 @@ #include -#include -#include -#include - #include "base/compiler_specific.h" #include "base/logging.h" #include "base/posix/eintr_wrapper.h" @@ -27,6 +23,23 @@ namespace crashpad { +namespace internal { + +// static +FileHandle ScopedLockedFileHandleTraits::InvalidValue() { + return kInvalidFileHandle; +} + +// static +void ScopedLockedFileHandleTraits::Free(FileHandle handle) { + if (handle != kInvalidFileHandle) { + LoggingUnlockFile(handle); + CheckedCloseFile(handle); + } +} + +} // namespace internal + struct ALIGNAS(4) Settings::Data { static const uint32_t kSettingsMagic = 'CPds'; static const uint32_t kSettingsVersion = 1; @@ -61,23 +74,6 @@ Settings::~Settings() { bool Settings::Initialize() { INITIALIZATION_STATE_SET_INITIALIZING(initialized_); - ScopedFileHandle handle(HANDLE_EINTR( - open(file_path().value().c_str(), - O_CREAT | O_EXCL | O_WRONLY | O_EXLOCK, - 0644))); - - // The file was created, so this is a new database that needs to be - // initialized with a client ID. - if (handle.is_valid()) { - bool initialized = InitializeSettings(handle.get()); - if (initialized) - INITIALIZATION_STATE_SET_VALID(initialized_); - return initialized; - } - - // The file wasn't created, try opening it for a write operation. If the file - // needs to be recovered, writing is necessary. This also ensures that the - // process has permission to write the file. Data settings; if (!OpenForWritingAndReadSettings(&settings).is_valid()) return false; @@ -112,7 +108,7 @@ bool Settings::SetUploadsEnabled(bool enabled) { INITIALIZATION_STATE_DCHECK_VALID(initialized_); Data settings; - ScopedFileHandle handle = OpenForWritingAndReadSettings(&settings); + ScopedLockedFileHandle handle = OpenForWritingAndReadSettings(&settings); if (!handle.is_valid()) return false; @@ -140,7 +136,7 @@ bool Settings::SetLastUploadAttemptTime(time_t time) { INITIALIZATION_STATE_DCHECK_VALID(initialized_); Data settings; - ScopedFileHandle handle = OpenForWritingAndReadSettings(&settings); + ScopedLockedFileHandle handle = OpenForWritingAndReadSettings(&settings); if (!handle.is_valid()) return false; @@ -149,22 +145,33 @@ bool Settings::SetLastUploadAttemptTime(time_t time) { return WriteSettings(handle.get(), settings); } -ScopedFileHandle Settings::OpenForReading() { - ScopedFileHandle handle(HANDLE_EINTR( - open(file_path().value().c_str(), O_RDONLY | O_SHLOCK))); - PLOG_IF(ERROR, !handle.is_valid()) << "open for reading"; - return handle.Pass(); +// static +Settings::ScopedLockedFileHandle Settings::MakeScopedLockedFileHandle( + FileHandle file, + FileLocking locking) { + ScopedFileHandle scoped(file); + if (scoped.is_valid()) { + if (!LoggingLockFile(scoped.get(), locking)) + scoped.reset(); + } + return ScopedLockedFileHandle(scoped.release()); } -ScopedFileHandle Settings::OpenForReadingAndWriting() { - ScopedFileHandle handle(HANDLE_EINTR( - open(file_path().value().c_str(), O_RDWR | O_EXLOCK | O_CREAT, 0644))); - PLOG_IF(ERROR, !handle.is_valid()) << "open for writing"; - return handle.Pass(); +Settings::ScopedLockedFileHandle Settings::OpenForReading() { + return MakeScopedLockedFileHandle(LoggingOpenFileForRead(file_path()), + FileLocking::kShared); +} + +Settings::ScopedLockedFileHandle Settings::OpenForReadingAndWriting() { + return MakeScopedLockedFileHandle( + LoggingOpenFileForReadAndWrite(file_path(), + FileWriteMode::kReuseOrCreate, + FilePermissions::kWorldReadable), + FileLocking::kExclusive); } bool Settings::OpenAndReadSettings(Data* out_data) { - ScopedFileHandle handle = OpenForReading(); + ScopedLockedFileHandle handle = OpenForReading(); if (!handle.is_valid()) return false; @@ -178,14 +185,15 @@ bool Settings::OpenAndReadSettings(Data* out_data) { return RecoverSettings(kInvalidFileHandle, out_data); } -ScopedFileHandle Settings::OpenForWritingAndReadSettings(Data* out_data) { - ScopedFileHandle handle = OpenForReadingAndWriting(); +Settings::ScopedLockedFileHandle Settings::OpenForWritingAndReadSettings( + Data* out_data) { + ScopedLockedFileHandle handle = OpenForReadingAndWriting(); if (!handle.is_valid()) - return ScopedFileHandle(); + return ScopedLockedFileHandle(); if (!ReadSettings(handle.get(), out_data)) { if (!RecoverSettings(handle.get(), out_data)) - return ScopedFileHandle(); + return ScopedLockedFileHandle(); } return handle.Pass(); @@ -215,16 +223,14 @@ bool Settings::WriteSettings(FileHandle handle, const Data& data) { if (LoggingSeekFile(handle, 0, SEEK_SET) != 0) return false; - if (HANDLE_EINTR(ftruncate(handle, 0)) != 0) { - PLOG(ERROR) << "ftruncate settings file"; + if (!LoggingTruncateFile(handle)) return false; - } return LoggingWriteFile(handle, &data, sizeof(Data)); } bool Settings::RecoverSettings(FileHandle handle, Data* out_data) { - ScopedFileHandle scoped_handle; + ScopedLockedFileHandle scoped_handle; if (handle == kInvalidFileHandle) { scoped_handle = OpenForReadingAndWriting(); handle = scoped_handle.get(); @@ -235,8 +241,6 @@ bool Settings::RecoverSettings(FileHandle handle, Data* out_data) { return true; } - LOG(INFO) << "Recovering settings file " << file_path().value(); - if (handle == kInvalidFileHandle) { LOG(ERROR) << "Invalid file handle"; return false; @@ -249,11 +253,9 @@ bool Settings::RecoverSettings(FileHandle handle, Data* out_data) { } bool Settings::InitializeSettings(FileHandle handle) { - uuid_t uuid; - uuid_generate(uuid); - Data settings; - settings.client_id.InitializeFromBytes(uuid); + if (!settings.client_id.InitializeWithNew()) + return false; return WriteSettings(handle, settings); } diff --git a/client/settings.h b/client/settings.h index bab5186a..61e18c43 100644 --- a/client/settings.h +++ b/client/settings.h @@ -21,12 +21,22 @@ #include "base/basictypes.h" #include "base/files/file_path.h" +#include "base/scoped_generic.h" #include "util/file/file_io.h" #include "util/misc/initialization_state_dcheck.h" #include "util/misc/uuid.h" namespace crashpad { +namespace internal { + +struct ScopedLockedFileHandleTraits { + static FileHandle InvalidValue(); + static void Free(FileHandle handle); +}; + +} // namespace internal + //! \brief An interface for accessing and modifying the settings of a //! CrashReportDatabase. //! @@ -95,13 +105,21 @@ class Settings { private: struct Data; + // This must be constructed with MakeScopedLockedFileHandle(). It both unlocks + // and closes the file on destruction. + using ScopedLockedFileHandle = + base::ScopedGeneric; + static ScopedLockedFileHandle MakeScopedLockedFileHandle(FileHandle file, + FileLocking locking); + // Opens the settings file for reading. On error, logs a message and returns // the invalid handle. - ScopedFileHandle OpenForReading(); + ScopedLockedFileHandle OpenForReading(); // Opens the settings file for reading and writing. On error, logs a message // and returns the invalid handle. - ScopedFileHandle OpenForReadingAndWriting(); + ScopedLockedFileHandle OpenForReadingAndWriting(); // Opens the settings file and reads the data. If that fails, an error will // be logged and the settings will be recovered and re-initialized. If that @@ -111,7 +129,7 @@ class Settings { // Opens the settings file for writing and reads the data. If reading fails, // recovery is attempted. Returns the opened file handle on success, or the // invalid file handle on failure, with an error logged. - ScopedFileHandle OpenForWritingAndReadSettings(Data* out_data); + ScopedLockedFileHandle OpenForWritingAndReadSettings(Data* out_data); // Reads the settings from |handle|. Logs an error and returns false on // failure. This does not perform recovery. diff --git a/client/settings_test.cc b/client/settings_test.cc index 7ce8fa23..203bd215 100644 --- a/client/settings_test.cc +++ b/client/settings_test.cc @@ -15,6 +15,7 @@ #include "client/settings.h" #include "gtest/gtest.h" +#include "test/errors.h" #include "test/scoped_temp_dir.h" #include "util/file/file_io.h" @@ -27,7 +28,7 @@ class SettingsTest : public testing::Test { SettingsTest() : settings_(settings_path()) {} base::FilePath settings_path() { - return temp_dir_.path().Append("settings"); + return temp_dir_.path().Append(FILE_PATH_LITERAL("settings")); } Settings* settings() { return &settings_; } @@ -151,7 +152,13 @@ TEST_F(SettingsTest, UnlinkFile) { EXPECT_TRUE(settings()->SetUploadsEnabled(true)); EXPECT_TRUE(settings()->SetLastUploadAttemptTime(time(nullptr))); - EXPECT_EQ(0, unlink(settings_path().value().c_str())); +#if defined(OS_WIN) + EXPECT_EQ(0, _wunlink(settings_path().value().c_str())) + << ErrnoMessage("_wunlink"); +#else + EXPECT_EQ(0, unlink(settings_path().value().c_str())) + << ErrnoMessage("unlink"); +#endif Settings local_settings(settings_path()); EXPECT_TRUE(local_settings.Initialize()); diff --git a/util/file/file_io.h b/util/file/file_io.h index 5d5a957d..c3120c96 100644 --- a/util/file/file_io.h +++ b/util/file/file_io.h @@ -188,9 +188,9 @@ FileHandle LoggingOpenFileForRead(const base::FilePath& path); //! \brief Wraps `open()` or `CreateFile()`, creating a file for output. Logs //! an error if the operation fails. //! -//! \a write_mode determines the style (truncate, reuse, etc.) that is used to -//! open the file. On POSIX, \a permissions determines the value that is passed -//! as `mode` to `open()`. On Windows, the file is always opened in binary mode +//! \a mode determines the style (truncate, reuse, etc.) that is used to open +//! the file. On POSIX, \a permissions determines the value that is passed as +//! `mode` to `open()`. On Windows, the file is always opened in binary mode //! (that is, no CRLF translation). On Windows, the file is opened for sharing, //! see LoggingLockFile() and LoggingUnlockFile() to control concurrent access. //! @@ -200,9 +200,27 @@ FileHandle LoggingOpenFileForRead(const base::FilePath& path); //! \sa FilePermissions //! \sa ScopedFileHandle FileHandle LoggingOpenFileForWrite(const base::FilePath& path, - FileWriteMode write_mode, + FileWriteMode mode, FilePermissions permissions); +//! \brief Wraps `open()` or `CreateFile()`, creating a file for both input and +//! output. Logs an error if the operation fails. +//! +//! \a mode determines the style (truncate, reuse, etc.) that is used to open +//! the file. On POSIX, \a permissions determines the value that is passed as +//! `mode` to `open()`. On Windows, the file is always opened in binary mode +//! (that is, no CRLF translation). On Windows, the file is opened for sharing, +//! see LoggingLockFile() and LoggingUnlockFile() to control concurrent access. +//! +//! \return The newly opened FileHandle, or an invalid FileHandle on failure. +//! +//! \sa FileWriteMode +//! \sa FilePermissions +//! \sa ScopedFileHandle +FileHandle LoggingOpenFileForReadAndWrite(const base::FilePath& path, + FileWriteMode mode, + FilePermissions permissions); + //! \brief Locks the given \a file using `flock()` on POSIX or `LockFileEx()` on //! Windows. //! @@ -243,6 +261,11 @@ bool LoggingUnlockFile(FileHandle file); //! `-1` on failure. FileOffset LoggingSeekFile(FileHandle file, FileOffset offset, int whence); +//! \brief Truncates the given \a file to zero bytes in length. +//! +//! \return `true` on success, or `false`, and a message will be logged. +bool LoggingTruncateFile(FileHandle file); + //! \brief Wraps `close()` or `CloseHandle()`, logging an error if the operation //! fails. //! diff --git a/util/file/file_io_posix.cc b/util/file/file_io_posix.cc index a1f3b5d1..9511b34e 100644 --- a/util/file/file_io_posix.cc +++ b/util/file/file_io_posix.cc @@ -69,6 +69,29 @@ ssize_t ReadOrWrite(int fd, namespace crashpad { +namespace { + +FileHandle LoggingOpenFileForOutput(int rdwr_or_wronly, + const base::FilePath& path, + FileWriteMode mode, + FilePermissions permissions) { + int flags = rdwr_or_wronly | O_CREAT; + // kReuseOrCreate does not need any additional flags. + if (mode == FileWriteMode::kTruncateOrCreate) + flags |= O_TRUNC; + else if (mode == FileWriteMode::kCreateOrFail) + flags |= O_EXCL; + + int fd = HANDLE_EINTR( + open(path.value().c_str(), + flags, + permissions == FilePermissions::kWorldReadable ? 0644 : 0600)); + PLOG_IF(ERROR, fd < 0) << "open " << path.value(); + return fd; +} + +} // namespace + ssize_t ReadFile(FileHandle file, void* buffer, size_t size) { return ReadOrWrite(file, buffer, size); } @@ -86,19 +109,13 @@ FileHandle LoggingOpenFileForRead(const base::FilePath& path) { FileHandle LoggingOpenFileForWrite(const base::FilePath& path, FileWriteMode mode, FilePermissions permissions) { - int flags = O_WRONLY | O_CREAT; - // kReuseOrCreate does not need any additional flags. - if (mode == FileWriteMode::kTruncateOrCreate) - flags |= O_TRUNC; - else if (mode == FileWriteMode::kCreateOrFail) - flags |= O_EXCL; + return LoggingOpenFileForOutput(O_WRONLY, path, mode, permissions); +} - int fd = HANDLE_EINTR( - open(path.value().c_str(), - flags, - permissions == FilePermissions::kWorldReadable ? 0644 : 0600)); - PLOG_IF(ERROR, fd < 0) << "open " << path.value(); - return fd; +FileHandle LoggingOpenFileForReadAndWrite(const base::FilePath& path, + FileWriteMode mode, + FilePermissions permissions) { + return LoggingOpenFileForOutput(O_RDWR, path, mode, permissions); } bool LoggingLockFile(FileHandle file, FileLocking locking) { @@ -120,6 +137,14 @@ FileOffset LoggingSeekFile(FileHandle file, FileOffset offset, int whence) { return rv; } +bool LoggingTruncateFile(FileHandle file) { + if (HANDLE_EINTR(ftruncate(file, 0)) != 0) { + PLOG(ERROR) << "ftruncate"; + return false; + } + return true; +} + bool LoggingCloseFile(FileHandle file) { int rv = IGNORE_EINTR(close(file)); PLOG_IF(ERROR, rv != 0) << "close"; diff --git a/util/file/file_io_win.cc b/util/file/file_io_win.cc index 33da8baa..0f7476d0 100644 --- a/util/file/file_io_win.cc +++ b/util/file/file_io_win.cc @@ -34,6 +34,38 @@ bool IsSocketHandle(HANDLE file) { namespace crashpad { +namespace { + +FileHandle LoggingOpenFileForOutput(DWORD access, + const base::FilePath& path, + FileWriteMode mode, + FilePermissions permissions) { + DWORD disposition = 0; + switch (mode) { + case FileWriteMode::kReuseOrCreate: + disposition = OPEN_ALWAYS; + break; + case FileWriteMode::kTruncateOrCreate: + disposition = CREATE_ALWAYS; + break; + case FileWriteMode::kCreateOrFail: + disposition = CREATE_NEW; + break; + } + HANDLE file = CreateFile(path.value().c_str(), + access, + FILE_SHARE_READ | FILE_SHARE_WRITE, + nullptr, + disposition, + FILE_ATTRIBUTE_NORMAL, + nullptr); + PLOG_IF(ERROR, file == INVALID_HANDLE_VALUE) << "CreateFile " + << path.value().c_str(); + return file; +} + +} // namespace + // TODO(scottmg): Handle > DWORD sized writes if necessary. ssize_t ReadFile(FileHandle file, void* buffer, size_t size) { @@ -95,28 +127,14 @@ FileHandle LoggingOpenFileForRead(const base::FilePath& path) { FileHandle LoggingOpenFileForWrite(const base::FilePath& path, FileWriteMode mode, FilePermissions permissions) { - DWORD disposition = 0; - switch (mode) { - case FileWriteMode::kReuseOrCreate: - disposition = OPEN_ALWAYS; - break; - case FileWriteMode::kTruncateOrCreate: - disposition = CREATE_ALWAYS; - break; - case FileWriteMode::kCreateOrFail: - disposition = CREATE_NEW; - break; - } - HANDLE file = CreateFile(path.value().c_str(), - GENERIC_WRITE, - FILE_SHARE_READ | FILE_SHARE_WRITE, - nullptr, - disposition, - FILE_ATTRIBUTE_NORMAL, - nullptr); - PLOG_IF(ERROR, file == INVALID_HANDLE_VALUE) << "CreateFile " - << path.value().c_str(); - return file; + return LoggingOpenFileForOutput(GENERIC_WRITE, path, mode, permissions); +} + +FileHandle LoggingOpenFileForReadAndWrite(const base::FilePath& path, + FileWriteMode mode, + FilePermissions permissions) { + return LoggingOpenFileForOutput( + GENERIC_READ | GENERIC_WRITE, path, mode, permissions); } bool LoggingLockFile(FileHandle file, FileLocking locking) { @@ -174,6 +192,16 @@ FileOffset LoggingSeekFile(FileHandle file, FileOffset offset, int whence) { return new_offset.QuadPart; } +bool LoggingTruncateFile(FileHandle file) { + if (LoggingSeekFile(file, 0, SEEK_SET) != 0) + return false; + if (!SetEndOfFile(file)) { + PLOG(ERROR) << "SetEndOfFile"; + return false; + } + return true; +} + bool LoggingCloseFile(FileHandle file) { BOOL rv = CloseHandle(file); PLOG_IF(ERROR, !rv) << "CloseHandle"; diff --git a/util/misc/uuid.cc b/util/misc/uuid.cc index 904efcd3..ca7b22b1 100644 --- a/util/misc/uuid.cc +++ b/util/misc/uuid.cc @@ -23,11 +23,16 @@ #include #include "base/basictypes.h" +#include "base/logging.h" #include "base/strings/stringprintf.h" #include "base/strings/utf_string_conversions.h" #include "base/sys_byteorder.h" #include "util/stdlib/cxx.h" +#if defined(OS_MACOSX) +#include +#endif // OS_MACOSX + #if CXX_LIBRARY_VERSION >= 2011 #include #endif @@ -44,6 +49,10 @@ static_assert(std::is_standard_layout::value, UUID::UUID() : data_1(0), data_2(0), data_3(0), data_4(), data_5() { } +UUID::UUID(InitializeWithNewTag) { + CHECK(InitializeWithNew()); +} + UUID::UUID(const uint8_t* bytes) { InitializeFromBytes(bytes); } @@ -88,6 +97,25 @@ bool UUID::InitializeFromString(const base::StringPiece& string) { return true; } +bool UUID::InitializeWithNew() { +#if defined(OS_MACOSX) + uuid_t uuid; + uuid_generate(uuid); + InitializeFromBytes(uuid); + return true; +#elif defined(OS_WIN) + ::UUID system_uuid; + if (UuidCreate(&system_uuid) != RPC_S_OK) { + LOG(ERROR) << "UuidCreate"; + return false; + } + InitializeFromSystemUUID(&system_uuid); + return true; +#else +#error Port. +#endif // OS_MACOSX +} + #if defined(OS_WIN) void UUID::InitializeFromSystemUUID(const ::UUID* system_uuid) { static_assert(sizeof(::UUID) == sizeof(UUID), diff --git a/util/misc/uuid.h b/util/misc/uuid.h index 29b36bc0..f25aa652 100644 --- a/util/misc/uuid.h +++ b/util/misc/uuid.h @@ -41,6 +41,16 @@ struct UUID { //! \brief Initializes the %UUID to zero. UUID(); + //! \brief Tag to pass to constructor to indicate it should initialize with + //! generated data. + struct InitializeWithNewTag {}; + + //! \brief Initializes the %UUID using a standard system facility to generate + //! the value. + //! + //! CHECKs on failure with a message logged. + explicit UUID(InitializeWithNewTag); + //! \copydoc InitializeFromBytes() explicit UUID(const uint8_t* bytes); @@ -67,6 +77,13 @@ struct UUID { //! parsed, with the object state untouched. bool InitializeFromString(const base::StringPiece& string); + //! \brief Initializes the %UUID using a standard system facility to generate + //! the value. + //! + //! \return `true` if the %UUID was initialized correctly, `false` otherwise + //! with a message logged. + bool InitializeWithNew(); + #if defined(OS_WIN) || DOXYGEN //! \brief Initializes the %UUID from a system `UUID` or `GUID` structure. //! diff --git a/util/misc/uuid_test.cc b/util/misc/uuid_test.cc index 0d08b22c..0dc6b425 100644 --- a/util/misc/uuid_test.cc +++ b/util/misc/uuid_test.cc @@ -153,6 +153,9 @@ TEST(UUID, UUID) { EXPECT_EQ(0x45u, uuid.data_5[4]); EXPECT_EQ(0x45u, uuid.data_5[5]); EXPECT_EQ("45454545-4545-4545-4545-454545454545", uuid.ToString()); + + UUID initialized_generated(UUID::InitializeWithNewTag{}); + EXPECT_NE(initialized_generated, uuid_zero); } TEST(UUID, FromString) {