From ecf3b37863367acfca2dcbae14a26e965d713e84 Mon Sep 17 00:00:00 2001 From: Scott Graham Date: Thu, 1 Oct 2015 14:04:49 -0700 Subject: [PATCH] win: Save contents of TEBs allowing !teb and !gle to work in windbg crashy_program's log looks something like this now: 0:000> .ecxr eax=00000007 ebx=7f24e000 ecx=7f24d000 edx=00000000 esi=00497ec8 edi=00d39ca0 eip=00cf5d12 esp=001ffcd8 ebp=001ffcdc iopl=0 nv up ei ng nz ac po cy cs=0023 ss=002b ds=002b es=002b fs=0053 gs=002b efl=00010293 crashy_program+0x5d12: 00cf5d12 ?? ??? 0:000> !teb TEB at 7f24d000 ExceptionList: 001ff548 StackBase: 00200000 StackLimit: 001fd000 SubSystemTib: 00000000 FiberData: 00001e00 ArbitraryUserPointer: 00000000 Self: 7f24d000 EnvironmentPointer: 00000000 ClientId: 00003658 . 00004630 RpcHandle: 00000000 Tls Storage: 7f24d02c PEB Address: 7f24e000 LastErrorValue: 2 LastStatusValue: c000000f Count Owned Locks: 0 HardErrorMode: 0 0:000> !gle LastErrorValue: (Win32) 0x2 (2) - The system cannot find the file specified. LastStatusValue: (NTSTATUS) 0xc000000f - {File Not Found} The file %hs does not exist. R=mark@chromium.org BUG=crashpad:46 Review URL: https://codereview.chromium.org/1364803004 . --- handler/win/crashy_test_program.cc | 22 ++++++++++++++++- minidump/minidump_thread_writer.cc | 5 ++++ minidump/minidump_thread_writer_test.cc | 33 +++++++++++++++++++++++-- snapshot/test/test_process_snapshot.h | 5 ++-- snapshot/test/test_thread_snapshot.cc | 7 ++++++ snapshot/test/test_thread_snapshot.h | 14 +++++++++++ snapshot/thread_snapshot.h | 11 +++++++++ snapshot/win/process_reader_win.cc | 14 +++++++---- snapshot/win/process_reader_win.h | 5 ++-- snapshot/win/thread_snapshot_win.cc | 22 +++++++++++++++-- snapshot/win/thread_snapshot_win.h | 5 ++++ util/win/process_structs.h | 16 ++++++++++-- 12 files changed, 143 insertions(+), 16 deletions(-) diff --git a/handler/win/crashy_test_program.cc b/handler/win/crashy_test_program.cc index 07c9cf10..622a5e5d 100644 --- a/handler/win/crashy_test_program.cc +++ b/handler/win/crashy_test_program.cc @@ -12,15 +12,35 @@ // See the License for the specific language governing permissions and // limitations under the License. -#include "client/crashpad_client.h" +#include +#include + +// ntstatus.h conflicts with windows.h so define this locally. +#ifndef STATUS_NO_SUCH_FILE +#define STATUS_NO_SUCH_FILE static_cast(0xC000000F) +#endif #include "base/logging.h" +#include "client/crashpad_client.h" #include "tools/tool_support.h" namespace crashpad { namespace { +ULONG RtlNtStatusToDosError(NTSTATUS status) { + static decltype(::RtlNtStatusToDosError)* rtl_nt_status_to_dos_error = + reinterpret_cast( + GetProcAddress(LoadLibrary(L"ntdll.dll"), "RtlNtStatusToDosError")); + DCHECK(rtl_nt_status_to_dos_error); + return rtl_nt_status_to_dos_error(status); +} + void SomeCrashyFunction() { + // SetLastError and NTSTATUS so that we have something to view in !gle in + // windbg. RtlNtStatusToDosError() stores STATUS_NO_SUCH_FILE into the + // LastStatusError of the TEB as a side-effect, and we'll be setting + // ERROR_FILE_NOT_FOUND for GetLastError(). + SetLastError(RtlNtStatusToDosError(STATUS_NO_SUCH_FILE)); volatile int* foo = reinterpret_cast(7); *foo = 42; } diff --git a/minidump/minidump_thread_writer.cc b/minidump/minidump_thread_writer.cc index a36d7614..72050bdb 100644 --- a/minidump/minidump_thread_writer.cc +++ b/minidump/minidump_thread_writer.cc @@ -150,6 +150,11 @@ void MinidumpThreadListWriter::InitializeFromSnapshot( thread->InitializeFromSnapshot(thread_snapshot, thread_id_map); AddThread(thread.Pass()); } + + // Do this in a separate loop to keep the thread stacks earlier in the dump, + // and together. + for (const ThreadSnapshot* thread_snapshot : thread_snapshots) + memory_list_writer_->AddFromSnapshot(thread_snapshot->ExtraMemory()); } void MinidumpThreadListWriter::SetMemoryListWriter( diff --git a/minidump/minidump_thread_writer_test.cc b/minidump/minidump_thread_writer_test.cc index 76087a37..9571f7c2 100644 --- a/minidump/minidump_thread_writer_test.cc +++ b/minidump/minidump_thread_writer_test.cc @@ -18,6 +18,8 @@ #include #include +#include + #include "base/compiler_specific.h" #include "base/format_macros.h" #include "base/strings/stringprintf.h" @@ -527,6 +529,9 @@ void RunInitializeFromSnapshotTest(bool thread_id_collision) { uint64_t thread_ids[arraysize(expect_threads)] = {}; uint8_t memory_values[arraysize(expect_threads)] = {}; uint32_t context_seeds[arraysize(expect_threads)] = {}; + MINIDUMP_MEMORY_DESCRIPTOR tebs[arraysize(expect_threads)] = {}; + + const size_t kTebSize = 1024; expect_threads[0].ThreadId = 1; expect_threads[0].SuspendCount = 2; @@ -537,6 +542,8 @@ void RunInitializeFromSnapshotTest(bool thread_id_collision) { expect_threads[0].ThreadContext.DataSize = sizeof(MinidumpContextType); memory_values[0] = 'A'; context_seeds[0] = 0x80000000; + tebs[0].StartOfMemoryRange = expect_threads[0].Teb; + tebs[0].Memory.DataSize = kTebSize; // The thread at index 1 has no stack. expect_threads[1].ThreadId = 11; @@ -545,6 +552,8 @@ void RunInitializeFromSnapshotTest(bool thread_id_collision) { expect_threads[1].Teb = 0xfedcba9876543210; expect_threads[1].ThreadContext.DataSize = sizeof(MinidumpContextType); context_seeds[1] = 0x40000001; + tebs[1].StartOfMemoryRange = expect_threads[1].Teb; + tebs[1].Memory.DataSize = kTebSize; expect_threads[2].ThreadId = 21; expect_threads[2].SuspendCount = 22; @@ -555,6 +564,8 @@ void RunInitializeFromSnapshotTest(bool thread_id_collision) { expect_threads[2].ThreadContext.DataSize = sizeof(MinidumpContextType); memory_values[2] = 'd'; context_seeds[2] = 0x20000002; + tebs[2].StartOfMemoryRange = expect_threads[2].Teb; + tebs[2].Memory.DataSize = kTebSize; if (thread_id_collision) { thread_ids[0] = 0x0123456700000001; @@ -595,6 +606,12 @@ void RunInitializeFromSnapshotTest(bool thread_id_collision) { Traits::InitializeCPUContext(thread_snapshot->MutableContext(), context_seeds[index]); + auto teb_snapshot = make_scoped_ptr(new TestMemorySnapshot()); + teb_snapshot->SetAddress(expect_threads[index].Teb); + teb_snapshot->SetSize(kTebSize); + teb_snapshot->SetValue(static_cast('t' + index)); + thread_snapshot->AddExtraMemory(teb_snapshot.Pass()); + thread_snapshots.push_back(thread_snapshot); } @@ -617,7 +634,7 @@ void RunInitializeFromSnapshotTest(bool thread_id_collision) { GetThreadListStream(string_file.string(), &thread_list, &memory_list)); ASSERT_EQ(3u, thread_list->NumberOfThreads); - ASSERT_EQ(2u, memory_list->NumberOfMemoryRanges); + ASSERT_EQ(5u, memory_list->NumberOfMemoryRanges); size_t memory_index = 0; for (size_t index = 0; index < thread_list->NumberOfThreads; ++index) { @@ -643,7 +660,7 @@ void RunInitializeFromSnapshotTest(bool thread_id_collision) { observed_stack, string_file.string(), memory_values[index], - index == thread_list->NumberOfThreads - 1)); + false)); ASSERT_NO_FATAL_FAILURE(ExpectMinidumpMemoryDescriptor( observed_stack, &memory_list->MemoryRanges[memory_index])); @@ -651,6 +668,18 @@ void RunInitializeFromSnapshotTest(bool thread_id_collision) { ++memory_index; } } + + for (size_t index = 0; index < thread_list->NumberOfThreads; ++index) { + const MINIDUMP_MEMORY_DESCRIPTOR* memory = + &memory_list->MemoryRanges[memory_index]; + ASSERT_NO_FATAL_FAILURE( + ExpectMinidumpMemoryDescriptor(&tebs[index], memory)); + std::string expected_data(kTebSize, static_cast('t' + index)); + std::string observed_data(&string_file.string()[memory->Memory.Rva], + memory->Memory.DataSize); + EXPECT_EQ(expected_data, observed_data); + ++memory_index; + } } TEST(MinidumpThreadWriter, InitializeFromSnapshot_x86) { diff --git a/snapshot/test/test_process_snapshot.h b/snapshot/test/test_process_snapshot.h index 361559f5..a30d82a3 100644 --- a/snapshot/test/test_process_snapshot.h +++ b/snapshot/test/test_process_snapshot.h @@ -98,8 +98,9 @@ class TestProcessSnapshot final : public ProcessSnapshot { //! \brief Add a memory snapshot to be returned by ExtraMemory(). //! - //! \param[in] peb The memory snapshot that will be included in ExtraMemory(). - //! The TestProcessSnapshot object takes ownership of \a extra_memory. + //! \param[in] extra_memory The memory snapshot that will be included in + //! ExtraMemory(). The TestProcessSnapshot object takes ownership of \a + //! extra_memory. void AddExtraMemory(scoped_ptr extra_memory) { extra_memory_.push_back(extra_memory.release()); } diff --git a/snapshot/test/test_thread_snapshot.cc b/snapshot/test/test_thread_snapshot.cc index 8ee26dff..621ff424 100644 --- a/snapshot/test/test_thread_snapshot.cc +++ b/snapshot/test/test_thread_snapshot.cc @@ -55,5 +55,12 @@ uint64_t TestThreadSnapshot::ThreadSpecificDataAddress() const { return thread_specific_data_address_; } +std::vector TestThreadSnapshot::ExtraMemory() const { + std::vector extra_memory; + for (const auto& em : extra_memory_) + extra_memory.push_back(em); + return extra_memory; +} + } // namespace test } // namespace crashpad diff --git a/snapshot/test/test_thread_snapshot.h b/snapshot/test/test_thread_snapshot.h index b9564695..9b00d32f 100644 --- a/snapshot/test/test_thread_snapshot.h +++ b/snapshot/test/test_thread_snapshot.h @@ -17,11 +17,14 @@ #include +#include + #include "base/basictypes.h" #include "base/memory/scoped_ptr.h" #include "snapshot/cpu_context.h" #include "snapshot/memory_snapshot.h" #include "snapshot/thread_snapshot.h" +#include "util/stdlib/pointer_container.h" namespace crashpad { namespace test { @@ -61,6 +64,15 @@ class TestThreadSnapshot final : public ThreadSnapshot { thread_specific_data_address_ = thread_specific_data_address; } + //! \brief Add a memory snapshot to be returned by ExtraMemory(). + //! + //! \param[in] extra_memory The memory snapshot that will be included in + //! ExtraMemory(). The TestThreadSnapshot object takes ownership of \a + //! extra_memory. + void AddExtraMemory(scoped_ptr extra_memory) { + extra_memory_.push_back(extra_memory.release()); + } + // ThreadSnapshot: const CPUContext* Context() const override; @@ -69,6 +81,7 @@ class TestThreadSnapshot final : public ThreadSnapshot { int SuspendCount() const override; int Priority() const override; uint64_t ThreadSpecificDataAddress() const override; + std::vector ExtraMemory() const override; private: union { @@ -81,6 +94,7 @@ class TestThreadSnapshot final : public ThreadSnapshot { int suspend_count_; int priority_; uint64_t thread_specific_data_address_; + PointerVector extra_memory_; DISALLOW_COPY_AND_ASSIGN(TestThreadSnapshot); }; diff --git a/snapshot/thread_snapshot.h b/snapshot/thread_snapshot.h index d43fc6e5..4d732578 100644 --- a/snapshot/thread_snapshot.h +++ b/snapshot/thread_snapshot.h @@ -17,6 +17,8 @@ #include +#include + namespace crashpad { struct CPUContext; @@ -62,6 +64,15 @@ class ThreadSnapshot { //! \brief Returns the base address of a region used to store thread-specific //! data. virtual uint64_t ThreadSpecificDataAddress() const = 0; + + //! \brief Returns a vector of additional memory blocks that should be + //! included in a minidump. + //! + //! \return A vector of MemorySnapshot objects that will be included in the + //! crash dump. The caller does not take ownership of these objects, they + //! are scoped to the lifetime of the ThreadSnapshot object that they + //! were obtained from. + virtual std::vector ExtraMemory() const = 0; }; } // namespace crashpad diff --git a/snapshot/win/process_reader_win.cc b/snapshot/win/process_reader_win.cc index 20e17ed9..a7665c29 100644 --- a/snapshot/win/process_reader_win.cc +++ b/snapshot/win/process_reader_win.cc @@ -177,7 +177,8 @@ bool FillThreadContextAndSuspendCount(HANDLE thread_handle, ProcessReaderWin::Thread::Thread() : context(), id(0), - teb(0), + teb_address(0), + teb_size(0), stack_region_address(0), stack_region_size(0), suspend_count(0), @@ -332,8 +333,9 @@ void ProcessReaderWin::ReadThreadData(bool is_64_reading_32) { // Read the TIB (Thread Information Block) which is the first element of the // TEB, for its stack fields. process_types::NT_TIB tib; - thread.teb = thread_basic_info.TebBaseAddress; - if (ReadMemory(thread.teb, sizeof(tib), &tib)) { + thread.teb_address = thread_basic_info.TebBaseAddress; + thread.teb_size = sizeof(process_types::TEB); + if (ReadMemory(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 @@ -341,8 +343,10 @@ void ProcessReaderWin::ReadThreadData(bool is_64_reading_32) { // https://msdn.microsoft.com/en-us/library/dn424783.aspx if (is_64_reading_32) { process_types::NT_TIB tib32; - thread.teb = tib.Wow64Teb; - if (ReadMemory(thread.teb, sizeof(tib32), &tib32)) { + thread.teb_address = tib.Wow64Teb; + thread.teb_size = + sizeof(process_types::TEB); + if (ReadMemory(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 934db724..285fb7f0 100644 --- a/snapshot/win/process_reader_win.h +++ b/snapshot/win/process_reader_win.h @@ -48,10 +48,11 @@ class ProcessReaderWin { CONTEXT native; #if defined(ARCH_CPU_64_BITS) WOW64_CONTEXT wow64; -#endif; +#endif } context; uint64_t id; - WinVMAddress teb; + WinVMAddress teb_address; + WinVMSize teb_size; WinVMAddress stack_region_address; WinVMSize stack_region_size; uint32_t suspend_count; diff --git a/snapshot/win/thread_snapshot_win.cc b/snapshot/win/thread_snapshot_win.cc index ce4b309b..8a0de39e 100644 --- a/snapshot/win/thread_snapshot_win.cc +++ b/snapshot/win/thread_snapshot_win.cc @@ -14,6 +14,8 @@ #include "snapshot/win/thread_snapshot_win.h" +#include + #include "base/logging.h" #include "snapshot/win/cpu_context_win.h" #include "snapshot/win/process_reader_win.h" @@ -22,7 +24,12 @@ namespace crashpad { namespace internal { ThreadSnapshotWin::ThreadSnapshotWin() - : ThreadSnapshot(), context_(), stack_(), thread_(), initialized_() { + : ThreadSnapshot(), + context_(), + stack_(), + teb_(), + thread_(), + initialized_() { } ThreadSnapshotWin::~ThreadSnapshotWin() { @@ -34,8 +41,11 @@ bool ThreadSnapshotWin::Initialize( INITIALIZATION_STATE_SET_INITIALIZING(initialized_); thread_ = process_reader_thread; + // TODO(scottmg): Ensure these regions are readable + // https://code.google.com/p/crashpad/issues/detail?id=59 stack_.Initialize( process_reader, thread_.stack_region_address, thread_.stack_region_size); + teb_.Initialize(process_reader, thread_.teb_address, thread_.teb_size); #if defined(ARCH_CPU_X86_64) if (process_reader->Is64Bit()) { @@ -84,7 +94,15 @@ int ThreadSnapshotWin::Priority() const { uint64_t ThreadSnapshotWin::ThreadSpecificDataAddress() const { INITIALIZATION_STATE_DCHECK_VALID(initialized_); - return thread_.teb; + return thread_.teb_address; +} + +std::vector ThreadSnapshotWin::ExtraMemory() const { + INITIALIZATION_STATE_DCHECK_VALID(initialized_); + // TODO(scottmg): Ensure this region is readable, and make sure we don't + // discard the entire dump if it isn't. + // https://code.google.com/p/crashpad/issues/detail?id=59 + return std::vector(1, &teb_); } } // namespace internal diff --git a/snapshot/win/thread_snapshot_win.h b/snapshot/win/thread_snapshot_win.h index 9829707f..3f283a18 100644 --- a/snapshot/win/thread_snapshot_win.h +++ b/snapshot/win/thread_snapshot_win.h @@ -17,6 +17,8 @@ #include +#include + #include "base/basictypes.h" #include "snapshot/cpu_context.h" #include "snapshot/memory_snapshot.h" @@ -24,6 +26,7 @@ #include "snapshot/win/memory_snapshot_win.h" #include "snapshot/win/process_reader_win.h" #include "util/misc/initialization_state_dcheck.h" +#include "util/stdlib/pointer_container.h" namespace crashpad { @@ -58,6 +61,7 @@ class ThreadSnapshotWin final : public ThreadSnapshot { int SuspendCount() const override; int Priority() const override; uint64_t ThreadSpecificDataAddress() const override; + std::vector ExtraMemory() const override; private: #if defined(ARCH_CPU_X86_FAMILY) @@ -68,6 +72,7 @@ class ThreadSnapshotWin final : public ThreadSnapshot { #endif CPUContext context_; MemorySnapshotWin stack_; + internal::MemorySnapshotWin teb_; ProcessReaderWin::Thread thread_; InitializationStateDcheck initialized_; diff --git a/util/win/process_structs.h b/util/win/process_structs.h index 79f65152..9c3be181 100644 --- a/util/win/process_structs.h +++ b/util/win/process_structs.h @@ -309,12 +309,24 @@ struct CLIENT_ID { }; // This is a partial definition of the TEB, as we do not currently use many -// fields of it. See http://www.nirsoft.net/kernel_struct/vista/TEB.html. +// fields of it. See http://www.nirsoft.net/kernel_struct/vista/TEB.html, and +// the (arch-specific) definition of _TEB in winternl.h. template struct TEB { NT_TIB NtTib; - typename Traits::Pointer EnvironmentPointer; + typename Traits::Pointer ProcessEnvironmentBlock; CLIENT_ID ClientId; + + // Not identical to Reserved2 in winternl's _TEB because we define ClientId. + typename Traits::Pointer RemainderOfReserved2[397]; + + BYTE Reserved3[1952]; + typename Traits::Pointer TlsSlots[64]; + BYTE Reserved4[8]; + typename Traits::Pointer Reserved5[26]; + typename Traits::Pointer ReservedForOle; + typename Traits::Pointer Reserved6[4]; + typename Traits::Pointer TlsExpansionSlots; }; // See https://msdn.microsoft.com/en-us/library/gg750724.aspx.