Code review: ‹ Prev 17 of 26 Next › Code review: Truncated Windows/x86 stacks (#114)

[Mark Mentovai]

Brian-

I've got a patch for the problems we've seen on Windows, where stacks
were missing frames or were truncated. This patch causes us to scan
for return addresses in FPO frames, and to scan for better candidate
frame pointers in FPO frames that allocate %ebp. I've tested it out
with a selection of real-world dumps and it does a much better job of
matching WinDbg in these cases, and is close to what I believe the
native stackwalker does.

http://code.google.com/p/google-breakpad/issues/detail?id=114
http://code.google.com/p/google-breakpad/issues/attachment?aid=5228666236871880824&name=breakpad.114.1.patch

Mark

[Mark Mentovai]

The initial patch could cause us to walk past the bottom of the stack.
This version contains a better termination case, which is to avoid
scanning for a caller frame if program string evaluation yielded zero
for both the return address and saved frame pointer.

I also fixed a regression that caused correctly-computed stack pointer
values to be obliterated in certain frame types (caught this one by
checking the testdata). Separately, I'd also like to add a test for
this new code, but the easiest way to do that might be to contrive a
dump by hand such that it exercises the patterns we need to test.

http://code.google.com/p/google-breakpad/issues/attachment?aid=916826528983734925&name=breakpad.114.2.patch

Mark

[Brian Ryner]

stackwalker_x86.cc:

  // function.  When encountering a nontraditional frame (one which takes
  // advantage of FPO), the stack may need to be scanned for these values,
  // for traditional frames, simple deterministic dereferencing suffices without
  // any need for scanning.

comment nit: this sentence is really long.  It seems better to split it into two sentences, the second one starting with "For traditional frames".  Also, some of the existing comments for FPO data  might want to be tweaked a bit.

      const int kRASearchWords = 15;

Would there be any reason to make this value tunable?

      u_int32_t location_start = dictionary[".raSearchStart"] + 4;

Might be worth mentioning that we're starting at +4 since we know the eip from raSearchStart is invalid.

      while (!(good_eip = modules_->GetModuleForAddress(eip))) {
        location += 4;
        if (location - kRASearchWords * 4 > location_start)
          break;

        if (!memory_->GetMemoryAtAddress(location, &eip))
          break;
      }

This seems more intuitive to me as a loop over location, with an early exit if GetMemoryAtAddress returns false or GetModuleForAddress returns a module.  e.g.

for (location = location_start; location += 4; location < location_start + kRASearchWords * 4) {
  ...
}

      while (memory_->GetMemoryAtAddress(location, &ebp)) {

Similar comment on loop structure.

This is an edge case, but what happens if there is some kind of dynamically generated code being run, e.g. it doesn't correspond to a mapped module?  Could we clobber a legitimate return address then?  If so, is there some way we could account for other mapped regions of memory?  This could be a TODO.

Looks good otherwise.

--
-Brian

[Mark Mentovai]

OK, updated patch available.

http://code.google.com/p/google-breakpad/issues/attachment?aid=-7870847148378104761&name=breakpad.114.3.patch

Mark

[Brian Ryner]

looks good to me.

--
-Brian