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

@@ -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

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

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

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.

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);
 

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);

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)");
 }