diff options
7 files changed, 137 insertions, 36 deletions
diff --git a/src/client/linux/minidump_writer/linux_core_dumper.cc b/src/client/linux/minidump_writer/linux_core_dumper.cc index 3e8c92fe..47cc26c0 100644 --- a/src/client/linux/minidump_writer/linux_core_dumper.cc +++ b/src/client/linux/minidump_writer/linux_core_dumper.cc @@ -102,9 +102,8 @@ bool LinuxCoreDumper::GetThreadInfoByIndex(size_t index, ThreadInfo* info) { #else #error "This code hasn't been ported to your platform yet." #endif - - return GetStackInfo(&info->stack, &info->stack_len, - reinterpret_cast<uintptr_t>(stack_pointer)); + info->stack_pointer = reinterpret_cast<uintptr_t>(stack_pointer); + return true; } bool LinuxCoreDumper::IsPostMortem() const { diff --git a/src/client/linux/minidump_writer/linux_core_dumper_unittest.cc b/src/client/linux/minidump_writer/linux_core_dumper_unittest.cc index d04ef3e1..50b31938 100644 --- a/src/client/linux/minidump_writer/linux_core_dumper_unittest.cc +++ b/src/client/linux/minidump_writer/linux_core_dumper_unittest.cc @@ -105,6 +105,9 @@ TEST(LinuxCoreDumperTest, VerifyDumpWithMultipleThreads) { for (unsigned i = 0; i < kNumOfThreads; ++i) { ThreadInfo info; EXPECT_TRUE(dumper.GetThreadInfoByIndex(i, &info)); + const void* stack; + size_t stack_len; + EXPECT_TRUE(dumper.GetStackInfo(&stack, &stack_len, info.stack_pointer)); EXPECT_EQ(getpid(), info.ppid); } } diff --git a/src/client/linux/minidump_writer/linux_dumper.h b/src/client/linux/minidump_writer/linux_dumper.h index 2ed3e14d..a4a3ab5a 100644 --- a/src/client/linux/minidump_writer/linux_dumper.h +++ b/src/client/linux/minidump_writer/linux_dumper.h @@ -72,10 +72,7 @@ struct ThreadInfo { pid_t tgid; // thread group id pid_t ppid; // parent process - // Even on platforms where the stack grows down, the following will point to - // the smallest address in the stack. - const void* stack; // pointer to the stack area - size_t stack_len; // length of the stack to copy + uintptr_t stack_pointer; // thread stack pointer #if defined(__i386) || defined(__x86_64) diff --git a/src/client/linux/minidump_writer/linux_ptrace_dumper.cc b/src/client/linux/minidump_writer/linux_ptrace_dumper.cc index 3863d79f..45d0f48f 100644 --- a/src/client/linux/minidump_writer/linux_ptrace_dumper.cc +++ b/src/client/linux/minidump_writer/linux_ptrace_dumper.cc @@ -218,9 +218,9 @@ bool LinuxPtraceDumper::GetThreadInfoByIndex(size_t index, ThreadInfo* info) { #else #error "This code hasn't been ported to your platform yet." #endif + info->stack_pointer = reinterpret_cast<uintptr_t>(stack_pointer); - return GetStackInfo(&info->stack, &info->stack_len, - (uintptr_t) stack_pointer); + return true; } bool LinuxPtraceDumper::IsPostMortem() const { diff --git a/src/client/linux/minidump_writer/linux_ptrace_dumper_unittest.cc b/src/client/linux/minidump_writer/linux_ptrace_dumper_unittest.cc index ba3548c5..ee3593cd 100644 --- a/src/client/linux/minidump_writer/linux_ptrace_dumper_unittest.cc +++ b/src/client/linux/minidump_writer/linux_ptrace_dumper_unittest.cc @@ -230,6 +230,10 @@ TEST(LinuxPtraceDumperTest, VerifyStackReadWithMultipleThreads) { ThreadInfo one_thread; for (size_t i = 0; i < dumper.threads().size(); ++i) { EXPECT_TRUE(dumper.GetThreadInfoByIndex(i, &one_thread)); + const void* stack; + size_t stack_len; + EXPECT_TRUE(dumper.GetStackInfo(&stack, &stack_len, + one_thread.stack_pointer)); // In the helper program, we stored a pointer to the thread id in a // specific register. Check that we can recover its value. #if defined(__ARM_EABI__) diff --git a/src/client/linux/minidump_writer/minidump_writer.cc b/src/client/linux/minidump_writer/minidump_writer.cc index 02f6c3e2..43ad05b6 100644 --- a/src/client/linux/minidump_writer/minidump_writer.cc +++ b/src/client/linux/minidump_writer/minidump_writer.cc @@ -628,6 +628,31 @@ class MinidumpWriter { #endif } + bool FillThreadStack(MDRawThread* thread, uintptr_t stack_pointer, + uint8_t** stack_copy) { + *stack_copy = NULL; + const void* stack; + size_t stack_len; + if (dumper_->GetStackInfo(&stack, &stack_len, stack_pointer)) { + UntypedMDRVA memory(&minidump_writer_); + if (!memory.Allocate(stack_len)) + return false; + *stack_copy = reinterpret_cast<uint8_t*>(Alloc(stack_len)); + dumper_->CopyFromProcess(*stack_copy, thread->thread_id, stack, + stack_len); + memory.Copy(*stack_copy, stack_len); + thread->stack.start_of_memory_range = + reinterpret_cast<uintptr_t>(stack); + thread->stack.memory = memory.location(); + memory_blocks_.push_back(thread->stack); + } else { + thread->stack.start_of_memory_range = stack_pointer; + thread->stack.memory.data_size = 0; + thread->stack.memory.rva = minidump_writer_.position(); + } + return true; + } + // Write information about the threads. bool WriteThreadListStream(MDRawDirectory* dirent) { const unsigned num_threads = dumper_->threads().size(); @@ -645,6 +670,7 @@ class MinidumpWriter { MDRawThread thread; my_memset(&thread, 0, sizeof(thread)); thread.thread_id = dumper_->threads()[i]; + // We have a different source of information for the crashing thread. If // we used the actual state of the thread we would find it running in the // signal handler with the alternative stack, which would be deeply @@ -652,20 +678,9 @@ class MinidumpWriter { if (static_cast<pid_t>(thread.thread_id) == GetCrashThread() && ucontext_ && !dumper_->IsPostMortem()) { - const void* stack; - size_t stack_len; - if (!dumper_->GetStackInfo(&stack, &stack_len, GetStackPointer())) + uint8_t* stack_copy; + if (!FillThreadStack(&thread, GetStackPointer(), &stack_copy)) return false; - UntypedMDRVA memory(&minidump_writer_); - if (!memory.Allocate(stack_len)) - return false; - uint8_t* stack_copy = reinterpret_cast<uint8_t*>(Alloc(stack_len)); - dumper_->CopyFromProcess(stack_copy, thread.thread_id, stack, - stack_len); - memory.Copy(stack_copy, stack_len); - thread.stack.start_of_memory_range = (uintptr_t) (stack); - thread.stack.memory = memory.location(); - memory_blocks_.push_back(thread.stack); // Copy 256 bytes around crashing instruction pointer to minidump. const size_t kIPMemorySize = 256; @@ -715,30 +730,26 @@ class MinidumpWriter { return false; my_memset(cpu.get(), 0, sizeof(RawContextCPU)); CPUFillFromUContext(cpu.get(), ucontext_, float_state_); - PopSeccompStackFrame(cpu.get(), thread, stack_copy); + if (stack_copy) + PopSeccompStackFrame(cpu.get(), thread, stack_copy); thread.thread_context = cpu.location(); crashing_thread_context_ = cpu.location(); } else { ThreadInfo info; if (!dumper_->GetThreadInfoByIndex(i, &info)) return false; - UntypedMDRVA memory(&minidump_writer_); - if (!memory.Allocate(info.stack_len)) + + uint8_t* stack_copy; + if (!FillThreadStack(&thread, info.stack_pointer, &stack_copy)) return false; - uint8_t* stack_copy = reinterpret_cast<uint8_t*>(Alloc(info.stack_len)); - dumper_->CopyFromProcess(stack_copy, thread.thread_id, info.stack, - info.stack_len); - memory.Copy(stack_copy, info.stack_len); - thread.stack.start_of_memory_range = (uintptr_t)(info.stack); - thread.stack.memory = memory.location(); - memory_blocks_.push_back(thread.stack); TypedMDRVA<RawContextCPU> cpu(&minidump_writer_); if (!cpu.Allocate()) return false; my_memset(cpu.get(), 0, sizeof(RawContextCPU)); CPUFillFromThreadInfo(cpu.get(), info); - PopSeccompStackFrame(cpu.get(), thread, stack_copy); + if (stack_copy) + PopSeccompStackFrame(cpu.get(), thread, stack_copy); thread.thread_context = cpu.location(); if (dumper_->threads()[i] == GetCrashThread()) { crashing_thread_context_ = cpu.location(); @@ -916,9 +927,16 @@ class MinidumpWriter { bool WriteMemoryListStream(MDRawDirectory* dirent) { TypedMDRVA<uint32_t> list(&minidump_writer_); - if (!list.AllocateObjectAndArray(memory_blocks_.size(), - sizeof(MDMemoryDescriptor))) - return false; + if (memory_blocks_.size()) { + if (!list.AllocateObjectAndArray(memory_blocks_.size(), + sizeof(MDMemoryDescriptor))) + return false; + } else { + // Still create the memory list stream, although it will have zero + // memory blocks. + if (!list.Allocate()) + return false; + } dirent->stream_type = MD_MEMORY_LIST_STREAM; dirent->location = list.location(); diff --git a/src/client/linux/minidump_writer/minidump_writer_unittest.cc b/src/client/linux/minidump_writer/minidump_writer_unittest.cc index e0d0fa9c..68bde073 100644 --- a/src/client/linux/minidump_writer/minidump_writer_unittest.cc +++ b/src/client/linux/minidump_writer/minidump_writer_unittest.cc @@ -501,3 +501,83 @@ TEST(MinidumpWriterTest, AdditionalMemory) { delete[] memory; close(fds[1]); } + +// Test that an invalid thread stack pointer still results in a minidump. +TEST(MinidumpWriterTest, InvalidStackPointer) { + int fds[2]; + ASSERT_NE(-1, pipe(fds)); + + const pid_t child = fork(); + if (child == 0) { + close(fds[1]); + char b; + HANDLE_EINTR(read(fds[0], &b, sizeof(b))); + close(fds[0]); + syscall(__NR_exit); + } + close(fds[0]); + + ExceptionHandler::CrashContext context; + + // This needs a valid context for minidump writing to work, but getting + // a useful one from the child is too much work, so just use one from + // the parent since the child is just a forked copy anyway. + ASSERT_EQ(0, getcontext(&context.context)); + context.tid = child; + + // Fake the child's stack pointer for its crashing thread. NOTE: This must + // be an invalid memory address for the child process (stack or otherwise). +#if defined(__i386) + // Try 1MB below the current stack. + uintptr_t invalid_stack_pointer = + reinterpret_cast<uintptr_t>(&context) - 1024*1024; + context.context.uc_mcontext.gregs[REG_ESP] = invalid_stack_pointer; +#elif defined(__x86_64) + // Try 1MB below the current stack. + uintptr_t invalid_stack_pointer = + reinterpret_cast<uintptr_t>(&context) - 1024*1024; + context.context.uc_mcontext.gregs[REG_RSP] = invalid_stack_pointer; +#elif defined(__ARM_EABI__) + // Try 1MB below the current stack. + uintptr_t invalid_stack_pointer = + reinterpret_cast<uintptr_t>(&context) - 1024*1024; + context.context.uc_mcontext.arm_sp = invalid_stack_pointer; +#else +# error "This code has not been ported to your platform yet." +#endif + + AutoTempDir temp_dir; + string templ = temp_dir.path() + "/minidump-writer-unittest"; + // NOTE: In previous versions of Breakpad, WriteMinidump() would fail if + // presented with an invalid stack pointer. + ASSERT_TRUE(WriteMinidump(templ.c_str(), child, &context, sizeof(context))); + + // Read the minidump. Ensure that the memory region is present + Minidump minidump(templ.c_str()); + ASSERT_TRUE(minidump.Read()); + + // TODO(ted.mielczarek,mkrebs): Enable this part of the test once + // https://breakpad.appspot.com/413002/ is committed. +#if 0 + // Make sure there's a thread without a stack. NOTE: It's okay if + // GetThreadList() shows the error: "ERROR: MinidumpThread has a memory + // region problem". + MinidumpThreadList* dump_thread_list = minidump.GetThreadList(); + ASSERT_TRUE(dump_thread_list); + bool found_empty_stack = false; + for (int i = 0; i < dump_thread_list->thread_count(); i++) { + MinidumpThread* thread = dump_thread_list->GetThreadAtIndex(i); + ASSERT_TRUE(thread->thread() != NULL); + // When the stack size is zero bytes, GetMemory() returns NULL. + if (thread->GetMemory() == NULL) { + found_empty_stack = true; + break; + } + } + // NOTE: If you fail this, first make sure that "invalid_stack_pointer" + // above is indeed set to an invalid address. + ASSERT_TRUE(found_empty_stack); +#endif + + close(fds[1]); +} |