From d11ed86d65e870c5ea0d4918693376d474dbfe7d Mon Sep 17 00:00:00 2001 From: Christopher Ferris Date: Thu, 11 Apr 2019 19:45:35 -0700 Subject: [PATCH] Fix pc/function name for signal handler frame. This refactors the step function slightly to split it up into distinct pieces since the code needs to handle a signal handler versus normal step slightly differently. Add a new error for an invalid elf. Modify libbacktrace code to handle new error code. Bug: 130302288 Test: libbacktrace/libunwindstack unit tests. Change-Id: I3fb9b00c02d2cf2cc5911541bba0346c6f39b8e6 --- libbacktrace/Backtrace.cpp | 2 + libbacktrace/UnwindStack.cpp | 4 ++ libbacktrace/include/backtrace/Backtrace.h | 2 + libunwindstack/Elf.cpp | 21 +++---- libunwindstack/LocalUnwinder.cpp | 26 ++++----- libunwindstack/Unwinder.cpp | 55 ++++++++++++------- libunwindstack/include/unwindstack/Elf.h | 5 +- libunwindstack/include/unwindstack/Error.h | 1 + libunwindstack/include/unwindstack/Unwinder.h | 3 +- libunwindstack/tests/ElfTest.cpp | 13 +++-- libunwindstack/tests/UnwindOfflineTest.cpp | 12 ++-- 11 files changed, 84 insertions(+), 60 deletions(-) diff --git a/libbacktrace/Backtrace.cpp b/libbacktrace/Backtrace.cpp index 6bec63c23..71980d7a5 100644 --- a/libbacktrace/Backtrace.cpp +++ b/libbacktrace/Backtrace.cpp @@ -170,5 +170,7 @@ std::string Backtrace::GetErrorString(BacktraceUnwindError error) { return "Failed to unwind due to invalid unwind information"; case BACKTRACE_UNWIND_ERROR_REPEATED_FRAME: return "Failed to unwind due to same sp/pc repeating"; + case BACKTRACE_UNWIND_ERROR_INVALID_ELF: + return "Failed to unwind due to invalid elf"; } } diff --git a/libbacktrace/UnwindStack.cpp b/libbacktrace/UnwindStack.cpp index f5f9b2ada..36640cdfa 100644 --- a/libbacktrace/UnwindStack.cpp +++ b/libbacktrace/UnwindStack.cpp @@ -89,6 +89,10 @@ bool Backtrace::Unwind(unwindstack::Regs* regs, BacktraceMap* back_map, case unwindstack::ERROR_REPEATED_FRAME: error->error_code = BACKTRACE_UNWIND_ERROR_REPEATED_FRAME; break; + + case unwindstack::ERROR_INVALID_ELF: + error->error_code = BACKTRACE_UNWIND_ERROR_INVALID_ELF; + break; } } diff --git a/libbacktrace/include/backtrace/Backtrace.h b/libbacktrace/include/backtrace/Backtrace.h index 10e790b3d..404e7e8ab 100644 --- a/libbacktrace/include/backtrace/Backtrace.h +++ b/libbacktrace/include/backtrace/Backtrace.h @@ -64,6 +64,8 @@ enum BacktraceUnwindErrorCode : uint32_t { BACKTRACE_UNWIND_ERROR_UNWIND_INFO, // Unwind information stopped due to sp/pc repeating. BACKTRACE_UNWIND_ERROR_REPEATED_FRAME, + // Unwind information stopped due to invalid elf. + BACKTRACE_UNWIND_ERROR_INVALID_ELF, }; struct BacktraceUnwindError { diff --git a/libunwindstack/Elf.cpp b/libunwindstack/Elf.cpp index 4b93abb43..345491356 100644 --- a/libunwindstack/Elf.cpp +++ b/libunwindstack/Elf.cpp @@ -160,7 +160,7 @@ ErrorCode Elf::GetLastErrorCode() { if (valid_) { return interface_->LastErrorCode(); } - return ERROR_NONE; + return ERROR_INVALID_ELF; } uint64_t Elf::GetLastErrorAddress() { @@ -170,22 +170,23 @@ uint64_t Elf::GetLastErrorAddress() { return 0; } +// The relative pc expectd by this function is relative to the start of the elf. +bool Elf::StepIfSignalHandler(uint64_t rel_pc, Regs* regs, Memory* process_memory) { + if (!valid_) { + return false; + } + return regs->StepIfSignalHandler(rel_pc, this, process_memory); +} + // The relative pc is always relative to the start of the map from which it comes. -bool Elf::Step(uint64_t rel_pc, uint64_t adjusted_rel_pc, Regs* regs, Memory* process_memory, - bool* finished) { +bool Elf::Step(uint64_t rel_pc, Regs* regs, Memory* process_memory, bool* finished) { if (!valid_) { return false; } - // The relative pc expectd by StepIfSignalHandler is relative to the start of the elf. - if (regs->StepIfSignalHandler(rel_pc, this, process_memory)) { - *finished = false; - return true; - } - // Lock during the step which can update information in the object. std::lock_guard guard(lock_); - return interface_->Step(adjusted_rel_pc, regs, process_memory, finished); + return interface_->Step(rel_pc, regs, process_memory, finished); } bool Elf::IsValidElf(Memory* memory) { diff --git a/libunwindstack/LocalUnwinder.cpp b/libunwindstack/LocalUnwinder.cpp index 5b2fadf2b..5d8120036 100644 --- a/libunwindstack/LocalUnwinder.cpp +++ b/libunwindstack/LocalUnwinder.cpp @@ -111,6 +111,14 @@ bool LocalUnwinder::Unwind(std::vector* frame_info, size_t max_f pc_adjustment = 0; } step_pc -= pc_adjustment; + + bool finished = false; + if (elf->StepIfSignalHandler(rel_pc, regs.get(), process_memory_.get())) { + step_pc = rel_pc; + } else if (!elf->Step(step_pc, regs.get(), process_memory_.get(), &finished)) { + finished = true; + } + // Skip any locations that are within this library. if (num_frames != 0 || !ShouldSkipLibrary(map_info->name)) { // Add frame information. @@ -124,22 +132,12 @@ bool LocalUnwinder::Unwind(std::vector* frame_info, size_t max_f } num_frames++; } - if (!elf->valid()) { - break; - } - if (frame_info->size() == max_frames) { - break; - } + if (finished || frame_info->size() == max_frames || + (cur_pc == regs->pc() && cur_sp == regs->sp())) { + break; + } adjust_pc = true; - bool finished; - if (!elf->Step(rel_pc, step_pc, regs.get(), process_memory_.get(), &finished) || finished) { - break; - } - // pc and sp are the same, terminate the unwind. - if (cur_pc == regs->pc() && cur_sp == regs->sp()) { - break; - } } return num_frames != 0; } diff --git a/libunwindstack/Unwinder.cpp b/libunwindstack/Unwinder.cpp index 3f2e1c1b8..f3d2b5e1c 100644 --- a/libunwindstack/Unwinder.cpp +++ b/libunwindstack/Unwinder.cpp @@ -89,8 +89,8 @@ void Unwinder::FillInDexFrame() { #endif } -void Unwinder::FillInFrame(MapInfo* map_info, Elf* elf, uint64_t rel_pc, uint64_t func_pc, - uint64_t pc_adjustment) { +FrameData* Unwinder::FillInFrame(MapInfo* map_info, Elf* elf, uint64_t rel_pc, + uint64_t pc_adjustment) { size_t frame_num = frames_.size(); frames_.resize(frame_num + 1); FrameData* frame = &frames_.at(frame_num); @@ -100,7 +100,8 @@ void Unwinder::FillInFrame(MapInfo* map_info, Elf* elf, uint64_t rel_pc, uint64_ frame->pc = regs_->pc() - pc_adjustment; if (map_info == nullptr) { - return; + // Nothing else to update. + return nullptr; } if (resolve_names_) { @@ -118,12 +119,7 @@ void Unwinder::FillInFrame(MapInfo* map_info, Elf* elf, uint64_t rel_pc, uint64_ frame->map_end = map_info->end; frame->map_flags = map_info->flags; frame->map_load_bias = elf->GetLoadBias(); - - if (!resolve_names_ || - !elf->GetFunctionName(func_pc, &frame->function_name, &frame->function_offset)) { - frame->function_name = ""; - frame->function_offset = 0; - } + return frame; } static bool ShouldStop(const std::vector* map_suffixes_to_ignore, @@ -194,6 +190,7 @@ void Unwinder::Unwind(const std::vector* initial_map_names_to_skip, } } + FrameData* frame = nullptr; if (map_info == nullptr || initial_map_names_to_skip == nullptr || std::find(initial_map_names_to_skip->begin(), initial_map_names_to_skip->end(), basename(map_info->name.c_str())) == initial_map_names_to_skip->end()) { @@ -210,23 +207,21 @@ void Unwinder::Unwind(const std::vector* initial_map_names_to_skip, } } - FillInFrame(map_info, elf, rel_pc, step_pc, pc_adjustment); + frame = FillInFrame(map_info, elf, rel_pc, pc_adjustment); // Once a frame is added, stop skipping frames. initial_map_names_to_skip = nullptr; } adjust_pc = true; - bool stepped; + bool stepped = false; bool in_device_map = false; - if (map_info == nullptr) { - stepped = false; - } else { + bool finished = false; + if (map_info != nullptr) { if (map_info->flags & MAPS_FLAGS_DEVICE_MAP) { // Do not stop here, fall through in case we are // in the speculative unwind path and need to remove // some of the speculative frames. - stepped = false; in_device_map = true; } else { MapInfo* sp_info = maps_->Find(regs_->sp()); @@ -234,19 +229,37 @@ void Unwinder::Unwind(const std::vector* initial_map_names_to_skip, // Do not stop here, fall through in case we are // in the speculative unwind path and need to remove // some of the speculative frames. - stepped = false; in_device_map = true; } else { - bool finished; - stepped = elf->Step(rel_pc, step_pc, regs_, process_memory_.get(), &finished); - elf->GetLastError(&last_error_); - if (stepped && finished) { - break; + if (elf->StepIfSignalHandler(rel_pc, regs_, process_memory_.get())) { + stepped = true; + if (frame != nullptr) { + // Need to adjust the relative pc because the signal handler + // pc should not be adjusted. + frame->rel_pc = rel_pc; + frame->pc += pc_adjustment; + step_pc = rel_pc; + } + } else if (elf->Step(step_pc, regs_, process_memory_.get(), &finished)) { + stepped = true; } + elf->GetLastError(&last_error_); } } } + if (frame != nullptr) { + if (!resolve_names_ || + !elf->GetFunctionName(step_pc, &frame->function_name, &frame->function_offset)) { + frame->function_name = ""; + frame->function_offset = 0; + } + } + + if (finished) { + break; + } + if (!stepped) { if (return_address_attempt) { // Only remove the speculative frame if there are more than two frames diff --git a/libunwindstack/include/unwindstack/Elf.h b/libunwindstack/include/unwindstack/Elf.h index ac94f101c..56bf318c8 100644 --- a/libunwindstack/include/unwindstack/Elf.h +++ b/libunwindstack/include/unwindstack/Elf.h @@ -67,8 +67,9 @@ class Elf { uint64_t GetRelPc(uint64_t pc, const MapInfo* map_info); - bool Step(uint64_t rel_pc, uint64_t adjusted_rel_pc, Regs* regs, Memory* process_memory, - bool* finished); + bool StepIfSignalHandler(uint64_t rel_pc, Regs* regs, Memory* process_memory); + + bool Step(uint64_t rel_pc, Regs* regs, Memory* process_memory, bool* finished); ElfInterface* CreateInterfaceFromMemory(Memory* memory); diff --git a/libunwindstack/include/unwindstack/Error.h b/libunwindstack/include/unwindstack/Error.h index 6ed0e0fb0..72ec4547f 100644 --- a/libunwindstack/include/unwindstack/Error.h +++ b/libunwindstack/include/unwindstack/Error.h @@ -29,6 +29,7 @@ enum ErrorCode : uint8_t { ERROR_INVALID_MAP, // Unwind in an invalid map. ERROR_MAX_FRAMES_EXCEEDED, // The number of frames exceed the total allowed. ERROR_REPEATED_FRAME, // The last frame has the same pc/sp as the next. + ERROR_INVALID_ELF, // Unwind in an invalid elf. }; struct ErrorData { diff --git a/libunwindstack/include/unwindstack/Unwinder.h b/libunwindstack/include/unwindstack/Unwinder.h index 8b01654a9..75be209a3 100644 --- a/libunwindstack/include/unwindstack/Unwinder.h +++ b/libunwindstack/include/unwindstack/Unwinder.h @@ -118,8 +118,7 @@ class Unwinder { Unwinder(size_t max_frames) : max_frames_(max_frames) { frames_.reserve(max_frames); } void FillInDexFrame(); - void FillInFrame(MapInfo* map_info, Elf* elf, uint64_t rel_pc, uint64_t func_pc, - uint64_t pc_adjustment); + FrameData* FillInFrame(MapInfo* map_info, Elf* elf, uint64_t rel_pc, uint64_t pc_adjustment); size_t max_frames_; Maps* maps_; diff --git a/libunwindstack/tests/ElfTest.cpp b/libunwindstack/tests/ElfTest.cpp index 23c9cf860..c432d6d4f 100644 --- a/libunwindstack/tests/ElfTest.cpp +++ b/libunwindstack/tests/ElfTest.cpp @@ -132,8 +132,12 @@ TEST_F(ElfTest, elf_invalid) { uint64_t func_offset; ASSERT_FALSE(elf.GetFunctionName(0, &name, &func_offset)); + ASSERT_FALSE(elf.StepIfSignalHandler(0, nullptr, nullptr)); + EXPECT_EQ(ERROR_INVALID_ELF, elf.GetLastErrorCode()); + bool finished; - ASSERT_FALSE(elf.Step(0, 0, nullptr, nullptr, &finished)); + ASSERT_FALSE(elf.Step(0, nullptr, nullptr, &finished)); + EXPECT_EQ(ERROR_INVALID_ELF, elf.GetLastErrorCode()); } TEST_F(ElfTest, elf32_invalid_machine) { @@ -295,9 +299,8 @@ TEST_F(ElfTest, step_in_signal_map) { } elf.FakeSetValid(true); - bool finished; - ASSERT_TRUE(elf.Step(0x3000, 0x1000, ®s, &process_memory, &finished)); - EXPECT_FALSE(finished); + ASSERT_TRUE(elf.StepIfSignalHandler(0x3000, ®s, &process_memory)); + EXPECT_EQ(ERROR_NONE, elf.GetLastErrorCode()); EXPECT_EQ(15U, regs.pc()); EXPECT_EQ(13U, regs.sp()); } @@ -336,7 +339,7 @@ TEST_F(ElfTest, step_in_interface) { EXPECT_CALL(*interface, Step(0x1000, ®s, &process_memory, &finished)) .WillOnce(::testing::Return(true)); - ASSERT_TRUE(elf.Step(0x1004, 0x1000, ®s, &process_memory, &finished)); + ASSERT_TRUE(elf.Step(0x1000, ®s, &process_memory, &finished)); } TEST_F(ElfTest, get_global_invalid_elf) { diff --git a/libunwindstack/tests/UnwindOfflineTest.cpp b/libunwindstack/tests/UnwindOfflineTest.cpp index 02ba9c8fa..6c64c4097 100644 --- a/libunwindstack/tests/UnwindOfflineTest.cpp +++ b/libunwindstack/tests/UnwindOfflineTest.cpp @@ -1215,7 +1215,7 @@ TEST_F(UnwindOfflineTest, offset_arm) { " #02 pc 0032bff3 libunwindstack_test (SignalOuterFunction+2)\n" " #03 pc 0032fed3 libunwindstack_test " "(unwindstack::SignalCallerHandler(int, siginfo*, void*)+26)\n" - " #04 pc 00026528 libc.so\n" + " #04 pc 0002652c libc.so (__restore)\n" " #05 pc 00000000 \n" " #06 pc 0032c2d9 libunwindstack_test (InnerFunction+736)\n" " #07 pc 0032cc4f libunwindstack_test (MiddleFunction+42)\n" @@ -1243,7 +1243,7 @@ TEST_F(UnwindOfflineTest, offset_arm) { EXPECT_EQ(0xf43d2ce8U, unwinder.frames()[2].sp); EXPECT_EQ(0x2e59ed3U, unwinder.frames()[3].pc); EXPECT_EQ(0xf43d2cf0U, unwinder.frames()[3].sp); - EXPECT_EQ(0xf4136528U, unwinder.frames()[4].pc); + EXPECT_EQ(0xf413652cU, unwinder.frames()[4].pc); EXPECT_EQ(0xf43d2d10U, unwinder.frames()[4].sp); EXPECT_EQ(0U, unwinder.frames()[5].pc); EXPECT_EQ(0xffcc0ee0U, unwinder.frames()[5].sp); @@ -1326,7 +1326,7 @@ TEST_F(UnwindOfflineTest, shared_lib_in_apk_arm64) { " #00 pc 000000000014ccbc linker64 (__dl_syscall+28)\n" " #01 pc 000000000005426c linker64 " "(__dl__ZL24debuggerd_signal_handleriP7siginfoPv+1128)\n" - " #02 pc 00000000000008bc vdso.so\n" + " #02 pc 00000000000008c0 vdso.so (__kernel_rt_sigreturn)\n" " #03 pc 00000000000846f4 libc.so (abort+172)\n" " #04 pc 0000000000084ad4 libc.so (__assert2+36)\n" " #05 pc 000000000003d5b4 ANGLEPrebuilt.apk!libfeature_support_angle.so (offset 0x4000) " @@ -1338,7 +1338,7 @@ TEST_F(UnwindOfflineTest, shared_lib_in_apk_arm64) { EXPECT_EQ(0x7df8ca3bf0ULL, unwinder.frames()[0].sp); EXPECT_EQ(0x7e82b5726cULL, unwinder.frames()[1].pc); EXPECT_EQ(0x7df8ca3bf0ULL, unwinder.frames()[1].sp); - EXPECT_EQ(0x7e82b018bcULL, unwinder.frames()[2].pc); + EXPECT_EQ(0x7e82b018c0ULL, unwinder.frames()[2].pc); EXPECT_EQ(0x7df8ca3da0ULL, unwinder.frames()[2].sp); EXPECT_EQ(0x7e7eecc6f4ULL, unwinder.frames()[3].pc); EXPECT_EQ(0x7dabf3db60ULL, unwinder.frames()[3].sp); @@ -1366,7 +1366,7 @@ TEST_F(UnwindOfflineTest, shared_lib_in_apk_memory_only_arm64) { " #00 pc 000000000014ccbc linker64 (__dl_syscall+28)\n" " #01 pc 000000000005426c linker64 " "(__dl__ZL24debuggerd_signal_handleriP7siginfoPv+1128)\n" - " #02 pc 00000000000008bc vdso.so\n" + " #02 pc 00000000000008c0 vdso.so (__kernel_rt_sigreturn)\n" " #03 pc 00000000000846f4 libc.so (abort+172)\n" " #04 pc 0000000000084ad4 libc.so (__assert2+36)\n" " #05 pc 000000000003d5b4 ANGLEPrebuilt.apk (offset 0x21d5000)\n" @@ -1377,7 +1377,7 @@ TEST_F(UnwindOfflineTest, shared_lib_in_apk_memory_only_arm64) { EXPECT_EQ(0x7df8ca3bf0ULL, unwinder.frames()[0].sp); EXPECT_EQ(0x7e82b5726cULL, unwinder.frames()[1].pc); EXPECT_EQ(0x7df8ca3bf0ULL, unwinder.frames()[1].sp); - EXPECT_EQ(0x7e82b018bcULL, unwinder.frames()[2].pc); + EXPECT_EQ(0x7e82b018c0ULL, unwinder.frames()[2].pc); EXPECT_EQ(0x7df8ca3da0ULL, unwinder.frames()[2].sp); EXPECT_EQ(0x7e7eecc6f4ULL, unwinder.frames()[3].pc); EXPECT_EQ(0x7dabf3db60ULL, unwinder.frames()[3].sp);