From 656fa55c74a8cb0fec9cfd0bf1c9135390c9831f Mon Sep 17 00:00:00 2001 From: Vlad Tsyrklevich Date: Wed, 14 Nov 2018 10:15:50 -0800 Subject: [PATCH] Refactor ProcessReaderWin to use ProcessMemoryWin Remove ProcessReaderWin's ReadMemory() and ReadAvailableMemory() methods and replace their uses with a new method that exposes an instance of ProcessMemoryWin instead. BUG=crashpad:262 Change-Id: Ief5b660b0504d7a740ee53c7cd2fa7672ae56249 Reviewed-on: https://chromium-review.googlesource.com/c/1278830 Reviewed-by: Mark Mentovai Commit-Queue: Vlad Tsyrklevich --- snapshot/win/capture_memory_delegate_win.cc | 3 +- snapshot/win/exception_snapshot_win.cc | 10 ++-- snapshot/win/memory_snapshot_win.cc | 2 +- snapshot/win/module_snapshot_win.cc | 6 +- snapshot/win/pe_image_annotations_reader.cc | 14 ++--- snapshot/win/process_reader_win.cc | 65 +++------------------ snapshot/win/process_reader_win.h | 23 ++------ snapshot/win/process_reader_win_test.cc | 4 +- snapshot/win/process_snapshot_win.cc | 33 ++++++----- snapshot/win/process_subrange_reader.cc | 4 +- 10 files changed, 51 insertions(+), 113 deletions(-) diff --git a/snapshot/win/capture_memory_delegate_win.cc b/snapshot/win/capture_memory_delegate_win.cc index fc1c608b..b652f4a4 100644 --- a/snapshot/win/capture_memory_delegate_win.cc +++ b/snapshot/win/capture_memory_delegate_win.cc @@ -39,7 +39,8 @@ bool CaptureMemoryDelegateWin::Is64Bit() const { bool CaptureMemoryDelegateWin::ReadMemory(uint64_t at, uint64_t num_bytes, void* into) const { - return process_reader_->ReadMemory(at, num_bytes, into); + return process_reader_->Memory()->Read( + at, base::checked_cast(num_bytes), into); } std::vector> CaptureMemoryDelegateWin::GetReadableRanges( diff --git a/snapshot/win/exception_snapshot_win.cc b/snapshot/win/exception_snapshot_win.cc index 64b53424..cba6f121 100644 --- a/snapshot/win/exception_snapshot_win.cc +++ b/snapshot/win/exception_snapshot_win.cc @@ -176,9 +176,9 @@ bool ExceptionSnapshotWin::InitializeFromExceptionPointers( CPUContext* context, CPUContextUnion* context_union)) { ExceptionPointersType exception_pointers; - if (!process_reader->ReadMemory(exception_pointers_address, - sizeof(exception_pointers), - &exception_pointers)) { + if (!process_reader->Memory()->Read(exception_pointers_address, + sizeof(exception_pointers), + &exception_pointers)) { LOG(ERROR) << "EXCEPTION_POINTERS read failed"; return false; } @@ -188,7 +188,7 @@ bool ExceptionSnapshotWin::InitializeFromExceptionPointers( } ExceptionRecordType first_record; - if (!process_reader->ReadMemory( + if (!process_reader->Memory()->Read( static_cast(exception_pointers.ExceptionRecord), sizeof(first_record), &first_record)) { @@ -240,7 +240,7 @@ bool ExceptionSnapshotWin::InitializeFromExceptionPointers( } ContextType context_record; - if (!process_reader->ReadMemory( + if (!process_reader->Memory()->Read( static_cast(exception_pointers.ContextRecord), sizeof(context_record), &context_record)) { diff --git a/snapshot/win/memory_snapshot_win.cc b/snapshot/win/memory_snapshot_win.cc index 17e8ac11..d56b9520 100644 --- a/snapshot/win/memory_snapshot_win.cc +++ b/snapshot/win/memory_snapshot_win.cc @@ -60,7 +60,7 @@ bool MemorySnapshotWin::Read(Delegate* delegate) const { } std::unique_ptr buffer(new uint8_t[size_]); - if (!process_reader_->ReadMemory(address_, size_, buffer.get())) { + if (!process_reader_->Memory()->Read(address_, size_, buffer.get())) { return false; } return delegate->MemorySnapshotDelegateRead(buffer.get(), size_); diff --git a/snapshot/win/module_snapshot_win.cc b/snapshot/win/module_snapshot_win.cc index 880e9da0..d76a187e 100644 --- a/snapshot/win/module_snapshot_win.cc +++ b/snapshot/win/module_snapshot_win.cc @@ -278,7 +278,7 @@ void ModuleSnapshotWin::GetCrashpadExtraMemoryRanges( std::vector simple_ranges( SimpleAddressRangeBag::num_entries); - if (!process_reader_->ReadMemory( + if (!process_reader_->Memory()->Read( crashpad_info.extra_address_ranges, simple_ranges.size() * sizeof(simple_ranges[0]), &simple_ranges[0])) { @@ -304,8 +304,8 @@ void ModuleSnapshotWin::GetCrashpadUserMinidumpStreams( for (uint64_t cur = crashpad_info.user_data_minidump_stream_head; cur;) { internal::UserDataMinidumpStreamListEntry list_entry; - if (!process_reader_->ReadMemory( - cur, sizeof(list_entry), &list_entry)) { + if (!process_reader_->Memory()->Read( + cur, sizeof(list_entry), &list_entry)) { LOG(WARNING) << "could not read user data stream entry from " << base::UTF16ToUTF8(name_); return; diff --git a/snapshot/win/pe_image_annotations_reader.cc b/snapshot/win/pe_image_annotations_reader.cc index 67961457..4c1f66fd 100644 --- a/snapshot/win/pe_image_annotations_reader.cc +++ b/snapshot/win/pe_image_annotations_reader.cc @@ -92,7 +92,7 @@ void PEImageAnnotationsReader::ReadCrashpadSimpleAnnotations( std::vector simple_annotations(SimpleStringDictionary::num_entries); - if (!process_reader_->ReadMemory( + if (!process_reader_->Memory()->Read( crashpad_info.simple_annotations, simple_annotations.size() * sizeof(simple_annotations[0]), &simple_annotations[0])) { @@ -127,9 +127,9 @@ void PEImageAnnotationsReader::ReadCrashpadAnnotationsList( } process_types::AnnotationList annotation_list_object; - if (!process_reader_->ReadMemory(crashpad_info.annotations_list, - sizeof(annotation_list_object), - &annotation_list_object)) { + if (!process_reader_->Memory()->Read(crashpad_info.annotations_list, + sizeof(annotation_list_object), + &annotation_list_object)) { LOG(WARNING) << "could not read annotations list object in " << base::UTF16ToUTF8(name_); return; @@ -140,7 +140,7 @@ void PEImageAnnotationsReader::ReadCrashpadAnnotationsList( current.link_node != annotation_list_object.tail_pointer && index < kMaxNumberOfAnnotations; ++index) { - if (!process_reader_->ReadMemory( + if (!process_reader_->Memory()->Read( current.link_node, sizeof(current), ¤t)) { LOG(WARNING) << "could not read annotation at index " << index << " in " << base::UTF16ToUTF8(name_); @@ -155,7 +155,7 @@ void PEImageAnnotationsReader::ReadCrashpadAnnotationsList( snapshot.type = current.type; char name[Annotation::kNameMaxLength]; - if (!process_reader_->ReadMemory(current.name, arraysize(name), name)) { + if (!process_reader_->Memory()->Read(current.name, arraysize(name), name)) { LOG(WARNING) << "could not read annotation name at index " << index << " in " << base::UTF16ToUTF8(name_); continue; @@ -167,7 +167,7 @@ void PEImageAnnotationsReader::ReadCrashpadAnnotationsList( size_t value_length = std::min(static_cast(current.size), Annotation::kValueMaxSize); snapshot.value.resize(value_length); - if (!process_reader_->ReadMemory( + if (!process_reader_->Memory()->Read( current.value, value_length, snapshot.value.data())) { LOG(WARNING) << "could not read annotation value at index " << index << " in " << base::UTF16ToUTF8(name_); diff --git a/snapshot/win/process_reader_win.cc b/snapshot/win/process_reader_win.cc index 2fa02584..16280e96 100644 --- a/snapshot/win/process_reader_win.cc +++ b/snapshot/win/process_reader_win.cc @@ -204,6 +204,7 @@ ProcessReaderWin::Thread::Thread() ProcessReaderWin::ProcessReaderWin() : process_(INVALID_HANDLE_VALUE), process_info_(), + process_memory_(), threads_(), modules_(), suspension_state_(), @@ -220,67 +221,15 @@ bool ProcessReaderWin::Initialize(HANDLE process, process_ = process; suspension_state_ = suspension_state; - process_info_.Initialize(process); + if (!process_info_.Initialize(process)) + return false; + if (!process_memory_.Initialize(process)) + return false; INITIALIZATION_STATE_SET_VALID(initialized_); return true; } -bool ProcessReaderWin::ReadMemory(WinVMAddress at, - WinVMSize num_bytes, - void* into) const { - if (num_bytes == 0) - return 0; - - SIZE_T bytes_read; - if (!ReadProcessMemory(process_, - reinterpret_cast(at), - into, - base::checked_cast(num_bytes), - &bytes_read) || - num_bytes != bytes_read) { - PLOG(ERROR) << "ReadMemory at 0x" << std::hex << at << std::dec << " of " - << num_bytes << " bytes failed"; - return false; - } - return true; -} - -WinVMSize ProcessReaderWin::ReadAvailableMemory(WinVMAddress at, - WinVMSize num_bytes, - void* into) const { - if (num_bytes == 0) - return 0; - - auto ranges = process_info_.GetReadableRanges( - CheckedRange(at, num_bytes)); - - // We only read up until the first unavailable byte, so we only read from the - // first range. If we have no ranges, then no bytes were accessible anywhere - // in the range. - if (ranges.empty()) { - LOG(ERROR) << base::StringPrintf( - "range at 0x%llx, size 0x%llx completely inaccessible", at, num_bytes); - return 0; - } - - // If the start address was adjusted, we couldn't read even the first - // requested byte. - if (ranges.front().base() != at) { - LOG(ERROR) << base::StringPrintf( - "start of range at 0x%llx, size 0x%llx inaccessible", at, num_bytes); - return 0; - } - - DCHECK_LE(ranges.front().size(), num_bytes); - - // If we fail on a normal read, then something went very wrong. - if (!ReadMemory(ranges.front().base(), ranges.front().size(), into)) - return 0; - - return ranges.front().size(); -} - bool ProcessReaderWin::StartTime(timeval* start_time) const { FILETIME creation, exit, kernel, user; if (!GetProcessTimes(process_, &creation, &exit, &kernel, &user)) { @@ -398,7 +347,7 @@ void ProcessReaderWin::ReadThreadData(bool is_64_reading_32) { process_types::NT_TIB tib; thread.teb_address = thread_basic_info.TebBaseAddress; thread.teb_size = sizeof(process_types::TEB); - if (ReadMemory(thread.teb_address, sizeof(tib), &tib)) { + if (process_memory_.Read(thread.teb_address, sizeof(tib), &tib)) { WinVMAddress base = 0; WinVMAddress limit = 0; // If we're reading a WOW64 process, then the TIB we just retrieved is the @@ -409,7 +358,7 @@ void ProcessReaderWin::ReadThreadData(bool is_64_reading_32) { thread.teb_address = tib.Wow64Teb; thread.teb_size = sizeof(process_types::TEB); - if (ReadMemory(thread.teb_address, sizeof(tib32), &tib32)) { + if (process_memory_.Read(thread.teb_address, sizeof(tib32), &tib32)) { base = tib32.StackBase; limit = tib32.StackLimit; } diff --git a/snapshot/win/process_reader_win.h b/snapshot/win/process_reader_win.h index 1794a51a..a4e32aaf 100644 --- a/snapshot/win/process_reader_win.h +++ b/snapshot/win/process_reader_win.h @@ -23,6 +23,7 @@ #include "base/macros.h" #include "build/build_config.h" #include "util/misc/initialization_state_dcheck.h" +#include "util/process/process_memory_win.h" #include "util/win/address_types.h" #include "util/win/process_info.h" @@ -84,25 +85,8 @@ class ProcessReaderWin { //! \return `true` if the target task is a 64-bit process. bool Is64Bit() const { return process_info_.Is64Bit(); } - //! \brief Attempts to read \a num_bytes bytes from the target process - //! starting at address \a at into \a into. - //! - //! \return `true` if the entire region could be read, or `false` with an - //! error logged. - //! - //! \sa ReadAvailableMemory - bool ReadMemory(WinVMAddress at, WinVMSize num_bytes, void* into) const; - - //! \brief Attempts to read \a num_bytes bytes from the target process - //! starting at address \a at into \a into. If some of the specified range - //! is not accessible, reads up to the first inaccessible byte. - //! - //! \return The actual number of bytes read. - //! - //! \sa ReadMemory - WinVMSize ReadAvailableMemory(WinVMAddress at, - WinVMSize num_bytes, - void* into) const; + //! \brief Return a memory reader for the target process. + const ProcessMemoryWin* Memory() const { return &process_memory_; } //! \brief Determines the target process' start time. //! @@ -145,6 +129,7 @@ class ProcessReaderWin { HANDLE process_; ProcessInfo process_info_; + ProcessMemoryWin process_memory_; std::vector threads_; std::vector modules_; ProcessSuspensionState suspension_state_; diff --git a/snapshot/win/process_reader_win_test.cc b/snapshot/win/process_reader_win_test.cc index d0d4d96f..0f122fbd 100644 --- a/snapshot/win/process_reader_win_test.cc +++ b/snapshot/win/process_reader_win_test.cc @@ -43,7 +43,7 @@ TEST(ProcessReaderWin, SelfBasic) { static constexpr char kTestMemory[] = "Some test memory"; char buffer[arraysize(kTestMemory)]; - ASSERT_TRUE(process_reader.ReadMemory( + ASSERT_TRUE(process_reader.Memory()->Read( reinterpret_cast(kTestMemory), sizeof(kTestMemory), &buffer)); EXPECT_STREQ(kTestMemory, buffer); } @@ -72,7 +72,7 @@ class ProcessReaderChild final : public WinMultiprocess { char buffer[sizeof(kTestMemory)]; ASSERT_TRUE( - process_reader.ReadMemory(address, sizeof(kTestMemory), &buffer)); + process_reader.Memory()->Read(address, sizeof(kTestMemory), &buffer)); EXPECT_EQ(strcmp(kTestMemory, buffer), 0); } diff --git a/snapshot/win/process_snapshot_win.cc b/snapshot/win/process_snapshot_win.cc index 289f50c0..7f1f93a0 100644 --- a/snapshot/win/process_snapshot_win.cc +++ b/snapshot/win/process_snapshot_win.cc @@ -21,7 +21,7 @@ #include #include "base/logging.h" -#include "base/strings/stringprintf.h" +#include "base/numerics/safe_conversions.h" #include "base/strings/utf_string_conversions.h" #include "util/misc/from_pointer_cast.h" #include "util/misc/time.h" @@ -63,9 +63,9 @@ bool ProcessSnapshotWin::Initialize( if (exception_information_address != 0) { ExceptionInformation exception_information = {}; - if (!process_reader_.ReadMemory(exception_information_address, - sizeof(exception_information), - &exception_information)) { + if (!process_reader_.Memory()->Read(exception_information_address, + sizeof(exception_information), + &exception_information)) { LOG(WARNING) << "ReadMemory ExceptionInformation failed"; return false; } @@ -298,9 +298,9 @@ void ProcessSnapshotWin::InitializeUnloadedModules() { FromPointerCast(event_trace_address); Traits::Pointer pointer_to_array; - if (!process_reader_.ReadMemory(address_in_target_process, - sizeof(pointer_to_array), - &pointer_to_array)) { + if (!process_reader_.Memory()->Read(address_in_target_process, + sizeof(pointer_to_array), + &pointer_to_array)) { LOG(ERROR) << "failed to read target address"; return; } @@ -311,7 +311,7 @@ void ProcessSnapshotWin::InitializeUnloadedModules() { const size_t data_size = *element_size * *element_count; std::vector data(data_size); - if (!process_reader_.ReadMemory(pointer_to_array, data_size, &data[0])) { + if (!process_reader_.Memory()->Read(pointer_to_array, data_size, &data[0])) { LOG(ERROR) << "failed to read unloaded module data"; return; } @@ -377,14 +377,15 @@ void ProcessSnapshotWin::InitializePebData( AddMemorySnapshot(peb_address, peb_size, &extra_memory_); process_types::PEB peb_data; - if (!process_reader_.ReadMemory(peb_address, peb_size, &peb_data)) { + if (!process_reader_.Memory()->Read( + peb_address, base::checked_cast(peb_size), &peb_data)) { LOG(ERROR) << "ReadMemory PEB"; return; } process_types::PEB_LDR_DATA peb_ldr_data; AddMemorySnapshot(peb_data.Ldr, sizeof(peb_ldr_data), &extra_memory_); - if (!process_reader_.ReadMemory( + if (!process_reader_.Memory()->Read( peb_data.Ldr, sizeof(peb_ldr_data), &peb_ldr_data)) { LOG(ERROR) << "ReadMemory PEB_LDR_DATA"; } else { @@ -406,9 +407,9 @@ void ProcessSnapshotWin::InitializePebData( } process_types::RTL_USER_PROCESS_PARAMETERS process_parameters; - if (!process_reader_.ReadMemory(peb_data.ProcessParameters, - sizeof(process_parameters), - &process_parameters)) { + if (!process_reader_.Memory()->Read(peb_data.ProcessParameters, + sizeof(process_parameters), + &process_parameters)) { LOG(ERROR) << "ReadMemory RTL_USER_PROCESS_PARAMETERS"; return; } @@ -496,7 +497,7 @@ void ProcessSnapshotWin::AddMemorySnapshotForLdrLIST_ENTRY( for (;;) { // |cur| is the pointer to LIST_ENTRY embedded in the LDR_DATA_TABLE_ENTRY. // So we need to offset back to the beginning of the structure. - if (!process_reader_.ReadMemory( + if (!process_reader_.Memory()->Read( cur - offset_of_member, sizeof(entry), &entry)) { return; } @@ -520,7 +521,7 @@ WinVMSize ProcessSnapshotWin::DetermineSizeOfEnvironmentBlock( // be more than enough. std::wstring env_block; env_block.resize(32768); - WinVMSize bytes_read = process_reader_.ReadAvailableMemory( + size_t bytes_read = process_reader_.Memory()->ReadAvailableMemory( start_of_environment_block, env_block.size() * sizeof(env_block[0]), &env_block[0]); @@ -543,7 +544,7 @@ void ProcessSnapshotWin::ReadLock( // RTL_CRITICAL_SECTION_DEBUG. process_types::RTL_CRITICAL_SECTION critical_section; - if (!process_reader_.ReadMemory( + if (!process_reader_.Memory()->Read( start, sizeof(critical_section), &critical_section)) { LOG(ERROR) << "failed to read RTL_CRITICAL_SECTION"; return; diff --git a/snapshot/win/process_subrange_reader.cc b/snapshot/win/process_subrange_reader.cc index 996fbdf3..124c5d02 100644 --- a/snapshot/win/process_subrange_reader.cc +++ b/snapshot/win/process_subrange_reader.cc @@ -15,6 +15,7 @@ #include "snapshot/win/process_subrange_reader.h" #include "base/logging.h" +#include "base/numerics/safe_conversions.h" #include "snapshot/win/process_reader_win.h" namespace crashpad { @@ -82,7 +83,8 @@ bool ProcessSubrangeReader::ReadMemory(WinVMAddress address, return false; } - return process_reader_->ReadMemory(address, size, into); + return process_reader_->Memory()->Read( + address, base::checked_cast(size), into); } bool ProcessSubrangeReader::InitializeInternal(ProcessReaderWin* process_reader,