From 03ebc1d245d420a26505623ab9f5790ac76f5b15 Mon Sep 17 00:00:00 2001 From: jimblandy Date: Thu, 28 Jan 2010 05:17:23 +0000 Subject: Breakpad processor: Fix function and public symbol lookup. In r480, I botched the change to make the comparisons that decide whether an address falls within a function's range safe from overflow. The original code said: address >= function_base && address < function_base + function_size which is fine unless the function abuts the end of the address space, in which case the addition overflows and you get a false negative. My change subtracted function_size from both sides of the latter comparison, which is meaning-preserving in true math, and gets you: address >= function_base && address - function_size < function_base This not only reads strangely, but also still overflows if function_size is greater than address. That's rare, but I've added a case to the unit tests that checks it. My intent had been to replace the addition which could overflow with a subtraction that was known not to overflow, namely: address >= function_base && address - function_base < function_size This is equivalent to the original in true math, and because of the first comparison, we know the subtraction won't underflow in MemAddr math. The patch includes similar fixes to the public symbol lookup code, and to FindWindowsFrameInfo, which was the only other function affected by r480. a=jimblandy, r=mmentovai git-svn-id: http://google-breakpad.googlecode.com/svn/trunk@503 4c0a9323-5329-0410-9bdc-e9ce6186880e --- src/processor/basic_source_line_resolver.cc | 6 +++--- src/processor/basic_source_line_resolver_unittest.cc | 12 +++++++++++- src/processor/testdata/module1.out | 3 +++ 3 files changed, 17 insertions(+), 4 deletions(-) (limited to 'src/processor') diff --git a/src/processor/basic_source_line_resolver.cc b/src/processor/basic_source_line_resolver.cc index 1aff2d1d..0bd17124 100644 --- a/src/processor/basic_source_line_resolver.cc +++ b/src/processor/basic_source_line_resolver.cc @@ -442,7 +442,7 @@ void BasicSourceLineResolver::Module::LookupAddress(StackFrame *frame) const { MemAddr public_address; if (functions_.RetrieveNearestRange(address, &func, &function_base, &function_size) && - address >= function_base && address - function_size < function_base) { + address >= function_base && address - function_base < function_size) { frame->function_name = func->name; frame->function_base = frame->module->base_address() + function_base; @@ -458,7 +458,7 @@ void BasicSourceLineResolver::Module::LookupAddress(StackFrame *frame) const { } } else if (public_symbols_.Retrieve(address, &public_symbol, &public_address) && - (!func.get() || public_address - function_size > function_base)) { + (!func.get() || public_address > function_base)) { frame->function_name = public_symbol->name; frame->function_base = frame->module->base_address() + public_address; } @@ -502,7 +502,7 @@ WindowsFrameInfo *BasicSourceLineResolver::Module::FindWindowsFrameInfo( linked_ptr public_symbol; MemAddr public_address; if (public_symbols_.Retrieve(address, &public_symbol, &public_address) && - (!function.get() || public_address - function_size > function_base)) { + (!function.get() || public_address > function_base)) { result->parameter_size = public_symbol->parameter_size; } diff --git a/src/processor/basic_source_line_resolver_unittest.cc b/src/processor/basic_source_line_resolver_unittest.cc index 2940831f..45dfe979 100644 --- a/src/processor/basic_source_line_resolver_unittest.cc +++ b/src/processor/basic_source_line_resolver_unittest.cc @@ -63,7 +63,7 @@ class TestCodeModule : public CodeModule { virtual ~TestCodeModule() {} virtual u_int64_t base_address() const { return 0; } - virtual u_int64_t size() const { return 0x4000; } + virtual u_int64_t size() const { return 0xb000; } virtual string code_file() const { return code_file_; } virtual string code_identifier() const { return ""; } virtual string debug_file() const { return ""; } @@ -162,6 +162,16 @@ static bool RunTests() { frame_info.reset(resolver.FindWindowsFrameInfo(&frame)); ASSERT_FALSE(frame_info.get()); + frame.instruction = 0x2900; + frame.module = &module1; + resolver.FillSourceLineInfo(&frame); + ASSERT_EQ(frame.function_name, string("PublicSymbol")); + + frame.instruction = 0x4000; + frame.module = &module1; + resolver.FillSourceLineInfo(&frame); + ASSERT_EQ(frame.function_name, string("LargeFunction")); + TestCodeModule module2("module2"); frame.instruction = 0x2181; diff --git a/src/processor/testdata/module1.out b/src/processor/testdata/module1.out index 85687828..3fb1f181 100644 --- a/src/processor/testdata/module1.out +++ b/src/processor/testdata/module1.out @@ -13,6 +13,9 @@ FUNC 1200 100 8 Function1_3 FUNC 1300 100 c Function1_4 FUNC 2000 0 0 Test_Zero_Size_Function_Is_Ignored 2000 4 88 2 +PUBLIC 2800 0 PublicSymbol +FUNC 3000 7000 42 LargeFunction +3000 7000 4098359 3 STACK WIN 4 1000 c 1 0 0 0 0 0 1 $eip 4 + ^ = $esp $ebp 8 + = $ebp $ebp ^ = STACK WIN 4 1100 8 1 0 0 0 0 0 1 $eip 4 + ^ = $esp $ebp 8 + = $ebp $ebp ^ = STACK WIN 4 1100 100 1 0 0 0 0 0 1 $eip 4 + ^ = $esp $ebp 8 + = $ebp $ebp ^ = -- cgit v1.2.1