From 947ef27362c91002d933088a61eeef60b104da40 Mon Sep 17 00:00:00 2001 From: "ivan.penkov@gmail.com" Date: Sat, 8 Dec 2012 03:18:52 +0000 Subject: [PATCH] The Google-breakpad processor rejects (ignores) context records that lack CPU type information in their context_flags fields. Such context records can be valid (e.g. contexts captured by ::RtlCaptureContext). http://code.google.com/p/google-breakpad/issues/detail?id=493 http://breakpad.appspot.com/500002/ git-svn-id: http://google-breakpad.googlecode.com/svn/trunk@1088 4c0a9323-5329-0410-9bdc-e9ce6186880e --- Makefile.am | 9 +- Makefile.in | 9 +- src/google_breakpad/processor/minidump.h | 10 ++ src/processor/minidump.cc | 87 ++++++++++++- src/processor/minidump_unittest.cc | 152 +++++++++++++++++++++++ src/processor/synth_minidump.cc | 5 +- src/processor/synth_minidump_unittest.cc | 2 +- 7 files changed, 262 insertions(+), 12 deletions(-) diff --git a/Makefile.am b/Makefile.am index cd91c335..b374fe2e 100644 --- a/Makefile.am +++ b/Makefile.am @@ -46,12 +46,13 @@ endif if GCC # These are good warnings to be treated as errors AM_CXXFLAGS += \ - -Werror=non-virtual-dtor \ - -Werror=vla \ - -Werror=unused-variable \ -Werror=missing-braces \ + -Werror=non-virtual-dtor \ -Werror=overloaded-virtual \ - -Werror=sign-compare + -Werror=reorder \ + -Werror=sign-compare \ + -Werror=unused-variable \ + -Werror=vla endif if LINUX_HOST diff --git a/Makefile.in b/Makefile.in index b9227e6d..9d07f411 100644 --- a/Makefile.in +++ b/Makefile.in @@ -76,12 +76,13 @@ host_triplet = @host@ # These are good warnings to be treated as errors @GCC_TRUE@am__append_2 = \ -@GCC_TRUE@ -Werror=non-virtual-dtor \ -@GCC_TRUE@ -Werror=vla \ -@GCC_TRUE@ -Werror=unused-variable \ @GCC_TRUE@ -Werror=missing-braces \ +@GCC_TRUE@ -Werror=non-virtual-dtor \ @GCC_TRUE@ -Werror=overloaded-virtual \ -@GCC_TRUE@ -Werror=sign-compare +@GCC_TRUE@ -Werror=reorder \ +@GCC_TRUE@ -Werror=sign-compare \ +@GCC_TRUE@ -Werror=unused-variable \ +@GCC_TRUE@ -Werror=vla # Build as PIC on Linux, for linux_client_unittest_shlib diff --git a/src/google_breakpad/processor/minidump.h b/src/google_breakpad/processor/minidump.h index 5e60a77c..0fe78443 100644 --- a/src/google_breakpad/processor/minidump.h +++ b/src/google_breakpad/processor/minidump.h @@ -908,6 +908,16 @@ class Minidump { virtual const MDRawHeader* header() const { return valid_ ? &header_ : NULL; } + // Reads the CPU information from the system info stream and generates the + // appropriate CPU flags. The returned context_cpu_flags are the same as + // if the CPU type bits were set in the context_flags of a context record. + // On success, context_cpu_flags will have the flags that identify the CPU. + // If a system info stream is missing, context_cpu_flags will be 0. + // Returns true if the current position in the stream was not changed. + // Returns false when the current location in the stream was changed and the + // attempt to restore the original position failed. + bool GetContextCPUFlagsFromSystemInfo(u_int32_t* context_cpu_flags); + // Reads the minidump file's header and top-level stream directory. // The minidump is expected to be positioned at the beginning of the // header. Read() sets up the stream list and map, and validates the diff --git a/src/processor/minidump.cc b/src/processor/minidump.cc index 0c13e00e..0bd1f6ba 100644 --- a/src/processor/minidump.cc +++ b/src/processor/minidump.cc @@ -286,8 +286,8 @@ MinidumpStream::MinidumpStream(Minidump* minidump) MinidumpContext::MinidumpContext(Minidump* minidump) : MinidumpStream(minidump), - context_flags_(0), - context_() { + context_(), + context_flags_(0) { } @@ -318,6 +318,14 @@ bool MinidumpContext::Read(u_int32_t expected_size) { Swap(&context_amd64->context_flags); u_int32_t cpu_type = context_amd64->context_flags & MD_CONTEXT_CPU_MASK; + if (cpu_type == 0) { + if (minidump_->GetContextCPUFlagsFromSystemInfo(&cpu_type)) { + context_amd64->context_flags |= cpu_type; + } else { + BPLOG(ERROR) << "Failed to preserve the current stream position"; + return false; + } + } if (cpu_type != MD_CONTEXT_AMD64) { //TODO: fall through to switch below? @@ -422,6 +430,15 @@ bool MinidumpContext::Read(u_int32_t expected_size) { } } + if (cpu_type == 0) { + if (minidump_->GetContextCPUFlagsFromSystemInfo(&cpu_type)) { + context_flags |= cpu_type; + } else { + BPLOG(ERROR) << "Failed to preserve the current stream position"; + return false; + } + } + // Allocate the context structure for the correct CPU and fill it. The // casts are slightly unorthodox, but it seems better to do that than to // maintain a separate pointer for each type of CPU context structure @@ -3816,6 +3833,72 @@ bool Minidump::Open() { return true; } +bool Minidump::GetContextCPUFlagsFromSystemInfo(u_int32_t *context_cpu_flags) { + // Initialize output parameters + *context_cpu_flags = 0; + + // Save the current stream position + off_t saved_position = Tell(); + if (saved_position == -1) { + // Failed to save the current stream position. + // Returns true because the current position of the stream is preserved. + return true; + } + + const MDRawSystemInfo* system_info = + GetSystemInfo() ? GetSystemInfo()->system_info() : NULL; + + if (system_info != NULL) { + switch (system_info->processor_architecture) { + case MD_CPU_ARCHITECTURE_X86: + *context_cpu_flags = MD_CONTEXT_X86; + break; + case MD_CPU_ARCHITECTURE_MIPS: + *context_cpu_flags = MD_CONTEXT_MIPS; + break; + case MD_CPU_ARCHITECTURE_ALPHA: + *context_cpu_flags = MD_CONTEXT_ALPHA; + break; + case MD_CPU_ARCHITECTURE_PPC: + *context_cpu_flags = MD_CONTEXT_PPC; + break; + case MD_CPU_ARCHITECTURE_SHX: + *context_cpu_flags = MD_CONTEXT_SHX; + break; + case MD_CPU_ARCHITECTURE_ARM: + *context_cpu_flags = MD_CONTEXT_ARM; + break; + case MD_CPU_ARCHITECTURE_IA64: + *context_cpu_flags = MD_CONTEXT_IA64; + break; + case MD_CPU_ARCHITECTURE_ALPHA64: + *context_cpu_flags = 0; + break; + case MD_CPU_ARCHITECTURE_MSIL: + *context_cpu_flags = 0; + break; + case MD_CPU_ARCHITECTURE_AMD64: + *context_cpu_flags = MD_CONTEXT_AMD64; + break; + case MD_CPU_ARCHITECTURE_X86_WIN64: + *context_cpu_flags = 0; + break; + case MD_CPU_ARCHITECTURE_SPARC: + *context_cpu_flags = MD_CONTEXT_SPARC; + break; + case MD_CPU_ARCHITECTURE_UNKNOWN: + *context_cpu_flags = 0; + break; + default: + *context_cpu_flags = 0; + break; + } + } + + // Restore position and return + return SeekSet(saved_position); +} + bool Minidump::Read() { // Invalidate cached data. diff --git a/src/processor/minidump_unittest.cc b/src/processor/minidump_unittest.cc index 1faf169d..f94c3eb3 100644 --- a/src/processor/minidump_unittest.cc +++ b/src/processor/minidump_unittest.cc @@ -831,6 +831,158 @@ TEST(Dump, OneExceptionX86XState) { EXPECT_EQ(0x2e951ef7U, raw_context.ss); } +// Testing that the CPU type can be loaded from a system info stream when +// the CPU flags are missing from the context_flags of an exception record +TEST(Dump, OneExceptionX86NoCPUFlags) { + Dump dump(0, kLittleEndian); + + MDRawContextX86 raw_context; + // Intentionally not setting CPU type in the context_flags + raw_context.context_flags = 0; + raw_context.edi = 0x3ecba80d; + raw_context.esi = 0x382583b9; + raw_context.ebx = 0x7fccc03f; + raw_context.edx = 0xf62f8ec2; + raw_context.ecx = 0x46a6a6a8; + raw_context.eax = 0x6a5025e2; + raw_context.ebp = 0xd9fabb4a; + raw_context.eip = 0x6913f540; + raw_context.cs = 0xbffe6eda; + raw_context.eflags = 0xb2ce1e2d; + raw_context.esp = 0x659caaa4; + raw_context.ss = 0x2e951ef7; + Context context(dump, raw_context); + + Exception exception(dump, context, + 0x1234abcd, // thread id + 0xdcba4321, // exception code + 0xf0e0d0c0, // exception flags + 0x0919a9b9c9d9e9f9ULL); // exception address + + dump.Add(&context); + dump.Add(&exception); + + // Add system info. This is needed as an alternative source for CPU type + // information. Note, that the CPU flags were intentionally skipped from + // the context_flags and this alternative source is required. + String csd_version(dump, "Service Pack 2"); + SystemInfo system_info(dump, SystemInfo::windows_x86, csd_version); + dump.Add(&system_info); + dump.Add(&csd_version); + + dump.Finish(); + + string contents; + ASSERT_TRUE(dump.GetContents(&contents)); + + istringstream minidump_stream(contents); + Minidump minidump(minidump_stream); + ASSERT_TRUE(minidump.Read()); + ASSERT_EQ(2U, minidump.GetDirectoryEntryCount()); + + MinidumpException *md_exception = minidump.GetException(); + ASSERT_TRUE(md_exception != NULL); + + u_int32_t thread_id; + ASSERT_TRUE(md_exception->GetThreadID(&thread_id)); + ASSERT_EQ(0x1234abcdU, thread_id); + + const MDRawExceptionStream* raw_exception = md_exception->exception(); + ASSERT_TRUE(raw_exception != NULL); + EXPECT_EQ(0xdcba4321, raw_exception->exception_record.exception_code); + EXPECT_EQ(0xf0e0d0c0, raw_exception->exception_record.exception_flags); + EXPECT_EQ(0x0919a9b9c9d9e9f9ULL, + raw_exception->exception_record.exception_address); + + MinidumpContext *md_context = md_exception->GetContext(); + ASSERT_TRUE(md_context != NULL); + + ASSERT_EQ((u_int32_t) MD_CONTEXT_X86, md_context->GetContextCPU()); + const MDRawContextX86 *md_raw_context = md_context->GetContextX86(); + ASSERT_TRUE(md_raw_context != NULL); + + // Even though the CPU flags were missing from the context_flags, the + // GetContext call above is expected to load the missing CPU flags from the + // system info stream and set the CPU type bits in context_flags. + ASSERT_EQ((u_int32_t) (MD_CONTEXT_X86), md_raw_context->context_flags); + + EXPECT_EQ(0x3ecba80dU, raw_context.edi); + EXPECT_EQ(0x382583b9U, raw_context.esi); + EXPECT_EQ(0x7fccc03fU, raw_context.ebx); + EXPECT_EQ(0xf62f8ec2U, raw_context.edx); + EXPECT_EQ(0x46a6a6a8U, raw_context.ecx); + EXPECT_EQ(0x6a5025e2U, raw_context.eax); + EXPECT_EQ(0xd9fabb4aU, raw_context.ebp); + EXPECT_EQ(0x6913f540U, raw_context.eip); + EXPECT_EQ(0xbffe6edaU, raw_context.cs); + EXPECT_EQ(0xb2ce1e2dU, raw_context.eflags); + EXPECT_EQ(0x659caaa4U, raw_context.esp); + EXPECT_EQ(0x2e951ef7U, raw_context.ss); +} + +// This test covers a scenario where a dump contains an exception but the +// context record of the exception is missing the CPU type information in its +// context_flags. The dump has no system info stream so it is imposible to +// deduce the CPU type, hence the context record is unusable. +TEST(Dump, OneExceptionX86NoCPUFlagsNoSystemInfo) { + Dump dump(0, kLittleEndian); + + MDRawContextX86 raw_context; + // Intentionally not setting CPU type in the context_flags + raw_context.context_flags = 0; + raw_context.edi = 0x3ecba80d; + raw_context.esi = 0x382583b9; + raw_context.ebx = 0x7fccc03f; + raw_context.edx = 0xf62f8ec2; + raw_context.ecx = 0x46a6a6a8; + raw_context.eax = 0x6a5025e2; + raw_context.ebp = 0xd9fabb4a; + raw_context.eip = 0x6913f540; + raw_context.cs = 0xbffe6eda; + raw_context.eflags = 0xb2ce1e2d; + raw_context.esp = 0x659caaa4; + raw_context.ss = 0x2e951ef7; + Context context(dump, raw_context); + + Exception exception(dump, context, + 0x1234abcd, // thread id + 0xdcba4321, // exception code + 0xf0e0d0c0, // exception flags + 0x0919a9b9c9d9e9f9ULL); // exception address + + dump.Add(&context); + dump.Add(&exception); + dump.Finish(); + + string contents; + ASSERT_TRUE(dump.GetContents(&contents)); + + istringstream minidump_stream(contents); + Minidump minidump(minidump_stream); + ASSERT_TRUE(minidump.Read()); + ASSERT_EQ(1U, minidump.GetDirectoryEntryCount()); + + MinidumpException *md_exception = minidump.GetException(); + ASSERT_TRUE(md_exception != NULL); + + u_int32_t thread_id; + ASSERT_TRUE(md_exception->GetThreadID(&thread_id)); + ASSERT_EQ(0x1234abcdU, thread_id); + + const MDRawExceptionStream* raw_exception = md_exception->exception(); + ASSERT_TRUE(raw_exception != NULL); + EXPECT_EQ(0xdcba4321, raw_exception->exception_record.exception_code); + EXPECT_EQ(0xf0e0d0c0, raw_exception->exception_record.exception_flags); + EXPECT_EQ(0x0919a9b9c9d9e9f9ULL, + raw_exception->exception_record.exception_address); + + // The context record of the exception is unusable because the context_flags + // don't have CPU type information and at the same time the minidump lacks + // system info stream so it is impossible to deduce the CPU type. + MinidumpContext *md_context = md_exception->GetContext(); + ASSERT_EQ(NULL, md_context); +} + TEST(Dump, OneExceptionARM) { Dump dump(0, kLittleEndian); diff --git a/src/processor/synth_minidump.cc b/src/processor/synth_minidump.cc index b207c0d9..0b623647 100644 --- a/src/processor/synth_minidump.cc +++ b/src/processor/synth_minidump.cc @@ -126,7 +126,10 @@ void Memory::CiteMemoryIn(test_assembler::Section *section) const { Context::Context(const Dump &dump, const MDRawContextX86 &context) : Section(dump) { // The caller should have properly set the CPU type flag. - assert(context.context_flags & MD_CONTEXT_X86); + // The high 24 bits identify the CPU. Note that context records with no CPU + // type information can be valid (e.g. produced by ::RtlCaptureContext). + assert(((context.context_flags & MD_CONTEXT_CPU_MASK) == 0) || + (context.context_flags & MD_CONTEXT_X86)); // It doesn't make sense to store x86 registers in big-endian form. assert(dump.endianness() == kLittleEndian); D32(context.context_flags); diff --git a/src/processor/synth_minidump_unittest.cc b/src/processor/synth_minidump_unittest.cc index 4376933b..b5cf9557 100644 --- a/src/processor/synth_minidump_unittest.cc +++ b/src/processor/synth_minidump_unittest.cc @@ -147,7 +147,7 @@ TEST(Context, ARM) { TEST(ContextDeathTest, X86BadFlags) { Dump dump(0, kLittleEndian); MDRawContextX86 raw; - raw.context_flags = 0; + raw.context_flags = MD_CONTEXT_AMD64; ASSERT_DEATH(Context context(dump, raw);, "context\\.context_flags & (0x[0-9a-f]+|MD_CONTEXT_X86)"); }