Refactor TaskMemory initialization
Currently, TaskMemory implements the ProcessMemory interface almost
exactly; however, it's initialized using a constructor instead of an
Initialize method which makes it incompatible with a number of
ProcessMemory tests. Change its initialization to match the other
ProcessMemory classes.
Bug: crashpad:263
Change-Id: I8022dc3e1827a5bb398aace0058ce9494b6b6eb6
Reviewed-on: https://chromium-review.googlesource.com/c/1384447
Commit-Queue: Vlad Tsyrklevich <vtsyrklevich@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>
diff --git a/snapshot/mac/process_reader_mac.cc b/snapshot/mac/process_reader_mac.cc
index e142fd2..4850bba 100644
--- a/snapshot/mac/process_reader_mac.cc
+++ b/snapshot/mac/process_reader_mac.cc
@@ -113,9 +113,11 @@
return false;
}
- is_64_bit_ = process_info_.Is64Bit();
+ if (!task_memory_.Initialize(task)) {
+ return false;
+ }
- task_memory_.reset(new TaskMemory(task));
+ is_64_bit_ = process_info_.Is64Bit();
task_ = task;
INITIALIZATION_STATE_SET_VALID(initialized_);
@@ -441,7 +443,7 @@
Module module;
module.timestamp = image_info.imageFileModDate;
- if (!task_memory_->ReadCString(image_info.imageFilePath, &module.name)) {
+ if (!task_memory_.ReadCString(image_info.imageFilePath, &module.name)) {
LOG(WARNING) << "could not read dyld_image_info::imageFilePath";
// Proceed anyway with an empty module name.
}
diff --git a/snapshot/mac/process_reader_mac.h b/snapshot/mac/process_reader_mac.h
index 91836db..8a5b2c8 100644
--- a/snapshot/mac/process_reader_mac.h
+++ b/snapshot/mac/process_reader_mac.h
@@ -138,7 +138,7 @@
bool CPUTimes(timeval* user_time, timeval* system_time) const;
//! \return Accesses the memory of the target task.
- TaskMemory* Memory() { return task_memory_.get(); }
+ TaskMemory* Memory() { return &task_memory_; }
//! \return The threads that are in the task (process). The first element (at
//! index `0`) corresponds to the main thread.
@@ -232,7 +232,7 @@
std::vector<Thread> threads_; // owns send rights
std::vector<Module> modules_;
std::vector<std::unique_ptr<MachOImageReader>> module_readers_;
- std::unique_ptr<TaskMemory> task_memory_;
+ TaskMemory task_memory_;
task_t task_; // weak
InitializationStateDcheck initialized_;
diff --git a/util/mach/task_memory.cc b/util/mach/task_memory.cc
index ad4c8c3..14069dd 100644
--- a/util/mach/task_memory.cc
+++ b/util/mach/task_memory.cc
@@ -64,10 +64,18 @@
DCHECK_LE(user_end, vm_end);
}
-TaskMemory::TaskMemory(task_t task) : task_(task) {
+TaskMemory::TaskMemory() : task_(TASK_NULL), initialized_() {}
+
+bool TaskMemory::Initialize(task_t task) {
+ INITIALIZATION_STATE_SET_INITIALIZING(initialized_);
+ task_ = task;
+ INITIALIZATION_STATE_SET_VALID(initialized_);
+ return true;
}
bool TaskMemory::Read(mach_vm_address_t address, size_t size, void* buffer) {
+ INITIALIZATION_STATE_DCHECK_VALID(initialized_);
+
std::unique_ptr<MappedMemory> memory = ReadMapped(address, size);
if (!memory) {
return false;
@@ -80,6 +88,8 @@
std::unique_ptr<TaskMemory::MappedMemory> TaskMemory::ReadMapped(
mach_vm_address_t address,
size_t size) {
+ INITIALIZATION_STATE_DCHECK_VALID(initialized_);
+
if (size == 0) {
return std::unique_ptr<MappedMemory>(new MappedMemory(0, 0, 0, 0));
}
@@ -104,12 +114,16 @@
}
bool TaskMemory::ReadCString(mach_vm_address_t address, std::string* string) {
+ INITIALIZATION_STATE_DCHECK_VALID(initialized_);
+
return ReadCStringInternal(address, false, 0, string);
}
bool TaskMemory::ReadCStringSizeLimited(mach_vm_address_t address,
mach_vm_size_t size,
std::string* string) {
+ INITIALIZATION_STATE_DCHECK_VALID(initialized_);
+
return ReadCStringInternal(address, true, size, string);
}
@@ -117,6 +131,8 @@
bool has_size,
mach_vm_size_t size,
std::string* string) {
+ INITIALIZATION_STATE_DCHECK_VALID(initialized_);
+
if (!has_size) {
size = PAGE_SIZE;
}
diff --git a/util/mach/task_memory.h b/util/mach/task_memory.h
index c8caf85..118ff0b 100644
--- a/util/mach/task_memory.h
+++ b/util/mach/task_memory.h
@@ -23,6 +23,7 @@
#include "base/mac/scoped_mach_vm.h"
#include "base/macros.h"
+#include "util/misc/initialization_state_dcheck.h"
namespace crashpad {
@@ -87,12 +88,21 @@
DISALLOW_COPY_AND_ASSIGN(MappedMemory);
};
- //! \param[in] task A send right to the target task’s task port. This object
- //! does not take ownership of the send right.
- explicit TaskMemory(task_t task);
-
+ TaskMemory();
~TaskMemory() {}
+ //! \brief Initializes this object to read the memory of a task with the
+ //! provided task port.
+ //!
+ //! This method must be called successfully prior to calling any other method
+ //! in this class.
+ //!
+ //! \param[in] task A send right to the target task's task port. This object
+ //! does not take ownership of the send right.
+ //!
+ //! \return `true` on success, `false` on failure with a message logged.
+ bool Initialize(task_t task);
+
//! \brief Copies memory from the target task into a caller-provided buffer in
//! the current task.
//!
@@ -170,6 +180,7 @@
std::string* string);
task_t task_; // weak
+ InitializationStateDcheck initialized_;
DISALLOW_COPY_AND_ASSIGN(TaskMemory);
};
diff --git a/util/mach/task_memory_test.cc b/util/mach/task_memory_test.cc
index e3ef760..904dfca 100644
--- a/util/mach/task_memory_test.cc
+++ b/util/mach/task_memory_test.cc
@@ -44,7 +44,8 @@
region[index] = (index % 256) ^ ((index >> 8) % 256);
}
- TaskMemory memory(mach_task_self());
+ TaskMemory memory;
+ ASSERT_TRUE(memory.Initialize(mach_task_self()));
// This tests using both the Read() and ReadMapped() interfaces.
std::string result(kSize, '\0');
@@ -119,7 +120,8 @@
mach_task_self(), address + PAGE_SIZE, PAGE_SIZE, FALSE, VM_PROT_NONE);
ASSERT_EQ(kr, KERN_SUCCESS) << MachErrorMessage(kr, "vm_protect");
- TaskMemory memory(mach_task_self());
+ TaskMemory memory;
+ ASSERT_TRUE(memory.Initialize(mach_task_self()));
std::string result(kSize, '\0');
EXPECT_FALSE(memory.Read(address, kSize, &result[0]));
@@ -171,7 +173,8 @@
}
TEST(TaskMemory, ReadCStringSelf) {
- TaskMemory memory(mach_task_self());
+ TaskMemory memory;
+ ASSERT_TRUE(memory.Initialize(mach_task_self()));
std::string result;
const char kConstCharEmpty[] = "";
@@ -253,7 +256,8 @@
mach_task_self(), address + PAGE_SIZE, PAGE_SIZE, FALSE, VM_PROT_NONE);
ASSERT_EQ(kr, KERN_SUCCESS) << MachErrorMessage(kr, "vm_protect");
- TaskMemory memory(mach_task_self());
+ TaskMemory memory;
+ ASSERT_TRUE(memory.Initialize(mach_task_self()));
std::string result;
EXPECT_FALSE(memory.ReadCString(address, &result));
@@ -298,7 +302,8 @@
}
TEST(TaskMemory, ReadCStringSizeLimited_ConstCharEmpty) {
- TaskMemory memory(mach_task_self());
+ TaskMemory memory;
+ ASSERT_TRUE(memory.Initialize(mach_task_self()));
std::string result;
static constexpr char kConstCharEmpty[] = "";
@@ -319,7 +324,8 @@
}
TEST(TaskMemory, ReadCStringSizeLimited_ConstCharShort) {
- TaskMemory memory(mach_task_self());
+ TaskMemory memory;
+ ASSERT_TRUE(memory.Initialize(mach_task_self()));
std::string result;
static constexpr char kConstCharShort[] = "A short const char[]";
@@ -339,7 +345,8 @@
}
TEST(TaskMemory, ReadCStringSizeLimited_StaticConstCharEmpty) {
- TaskMemory memory(mach_task_self());
+ TaskMemory memory;
+ ASSERT_TRUE(memory.Initialize(mach_task_self()));
std::string result;
static constexpr char kStaticConstCharEmpty[] = "";
@@ -364,7 +371,8 @@
}
TEST(TaskMemory, ReadCStringSizeLimited_StaticConstCharShort) {
- TaskMemory memory(mach_task_self());
+ TaskMemory memory;
+ ASSERT_TRUE(memory.Initialize(mach_task_self()));
std::string result;
static constexpr char kStaticConstCharShort[] =
@@ -391,7 +399,8 @@
}
TEST(TaskMemory, ReadCStringSizeLimited_StringShort) {
- TaskMemory memory(mach_task_self());
+ TaskMemory memory;
+ ASSERT_TRUE(memory.Initialize(mach_task_self()));
std::string result;
std::string string_short("A short std::string in a function");
@@ -411,7 +420,8 @@
}
TEST(TaskMemory, ReadCStringSizeLimited_StringLong) {
- TaskMemory memory(mach_task_self());
+ TaskMemory memory;
+ ASSERT_TRUE(memory.Initialize(mach_task_self()));
std::string result;
std::string string_long;
@@ -477,7 +487,8 @@
// hopefully there are either no other threads or they’re all quiescent, so
// nothing else should wind up mapped in the address.
- TaskMemory memory(mach_task_self());
+ TaskMemory memory;
+ ASSERT_TRUE(memory.Initialize(mach_task_self()));
std::unique_ptr<TaskMemory::MappedMemory> mapped;
static constexpr char kTestBuffer[] = "hello!";
@@ -514,7 +525,8 @@
TEST(TaskMemory, MappedMemoryReadCString) {
// This tests the behavior of TaskMemory::MappedMemory::ReadCString().
- TaskMemory memory(mach_task_self());
+ TaskMemory memory;
+ ASSERT_TRUE(memory.Initialize(mach_task_self()));
std::unique_ptr<TaskMemory::MappedMemory> mapped;
static constexpr char kTestBuffer[] = "0\0" "2\0" "45\0" "789";
