Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit c3d79f7c authored by Christopher Ferris's avatar Christopher Ferris
Browse files

Fix incorrect usage of relative pcs. 

When stepping, it's necessary to use both the unaltered relative pc
and the adjusted relative pc. If the adjusted pc is not used, the
wrong unwind information can be used.

Added new offline unit tests that take real data and verifies that it
unwinds properly.

Fix a bug in the map code that would not properly parse map data for
a 64 bit process when done in a 32 bit process.

Fix bug in eh_frame processing that didn't adjust the pc correctly.
Fix unit tests related to the pc adjustment.

Bug: 69475565

Test: Passes libbacktrace/libunwindstack unit tests.
Test: Run debuggerd -b on processes on a hikey.
Change-Id: Ic501a1c4549c5f61d2742a7105c42a960f2c892b
parent f819c1d9

libunwindstack/Android.bp

+3 −0
Original line number Diff line number Diff line
@@ -130,6 +130,7 @@ cc_test { 
        "tests/RegsStepIfSignalHandlerTest.cpp",

        "tests/RegsTest.cpp",

        "tests/SymbolsTest.cpp",

        "tests/UnwindOfflineTest.cpp",

        "tests/UnwindTest.cpp",

        "tests/UnwinderTest.cpp",

    ],
@@ -153,6 +154,8 @@ cc_test { 
    data: [

        "tests/files/elf32.xz",

        "tests/files/elf64.xz",

        "tests/files/offline/straddle_arm32/*",

        "tests/files/offline/straddle_arm64/*",

    ],

}

libunwindstack/DwarfEhFrame.h

+1 −1
Original line number Diff line number Diff line
@@ -40,7 +40,7 @@ class DwarfEhFrame : public DwarfSectionImpl<AddressType> { 


  uint64_t AdjustPcFromFde(uint64_t pc) override {

    // The eh_frame uses relative pcs.

    return pc + this->memory_.cur_offset();

    return pc + this->memory_.cur_offset() - 4;

  }

};

libunwindstack/Elf.cpp

+6 −6
Original line number Diff line number Diff line
@@ -104,8 +104,8 @@ bool Elf::GetFunctionName(uint64_t addr, std::string* name, uint64_t* func_offse 
}



// 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 elf_offset, Regs* regs, Memory* process_memory,

               bool* finished) {

bool Elf::Step(uint64_t rel_pc, uint64_t adjusted_rel_pc, uint64_t elf_offset, Regs* regs,

               Memory* process_memory, bool* finished) {

  if (!valid_) {

    return false;

  }
@@ -117,16 +117,16 @@ bool Elf::Step(uint64_t rel_pc, uint64_t elf_offset, Regs* regs, Memory* process 
  }



  // Adjust the load bias to get the real relative pc.

  if (rel_pc < load_bias_) {

  if (adjusted_rel_pc < load_bias_) {

    return false;

  }

  rel_pc -= load_bias_;

  adjusted_rel_pc -= load_bias_;



  // Lock during the step which can update information in the object.

  std::lock_guard<std::mutex> guard(lock_);

  return interface_->Step(rel_pc, regs, process_memory, finished) ||

  return interface_->Step(adjusted_rel_pc, regs, process_memory, finished) ||

         (gnu_debugdata_interface_ &&

          gnu_debugdata_interface_->Step(rel_pc, regs, process_memory, finished));

          gnu_debugdata_interface_->Step(adjusted_rel_pc, regs, process_memory, finished));

}



bool Elf::IsValidElf(Memory* memory) {

libunwindstack/Maps.cpp

+6 −6
Original line number Diff line number Diff line
@@ -64,13 +64,13 @@ static MapInfo* InternalParseLine(const char* line) { 
  // 6f000000-6f01e000 rwxp 00000000 00:0c 16389419   /system/lib/libcomposer.so

  char* str;

  const char* old_str = line;

  uint64_t start = strtoul(old_str, &str, 16);

  uint64_t start = strtoull(old_str, &str, 16);

  if (old_str == str || *str++ != '-') {

    return nullptr;

  }



  old_str = str;

  uint64_t end = strtoul(old_str, &str, 16);

  uint64_t end = strtoull(old_str, &str, 16);

  if (old_str == str || !std::isspace(*str++)) {

    return nullptr;

  }
@@ -112,14 +112,14 @@ static MapInfo* InternalParseLine(const char* line) { 
  }



  old_str = str;

  uint64_t offset = strtoul(old_str, &str, 16);

  uint64_t offset = strtoull(old_str, &str, 16);

  if (old_str == str || !std::isspace(*str)) {

    return nullptr;

  }



  // Ignore the 00:00 values.

  old_str = str;

  (void)strtoul(old_str, &str, 16);

  (void)strtoull(old_str, &str, 16);

  if (old_str == str || *str++ != ':') {

    return nullptr;

  }
@@ -129,14 +129,14 @@ static MapInfo* InternalParseLine(const char* line) { 


  // Skip the inode.

  old_str = str;

  (void)strtoul(str, &str, 16);

  (void)strtoull(str, &str, 16);

  if (old_str == str || !std::isspace(*str++)) {

    return nullptr;

  }



  // Skip decimal digit.

  old_str = str;

  (void)strtoul(old_str, &str, 10);

  (void)strtoull(old_str, &str, 10);

  if (old_str == str || (!std::isspace(*str) && *str != '\0')) {

    return nullptr;

  }

libunwindstack/Unwinder.cpp

+15 −13
Original line number Diff line number Diff line
@@ -32,27 +32,20 @@ 


namespace unwindstack {



void Unwinder::FillInFrame(MapInfo* map_info, Elf* elf, uint64_t rel_pc, bool adjust_pc) {

void Unwinder::FillInFrame(MapInfo* map_info, Elf* elf, uint64_t adjusted_rel_pc) {

  size_t frame_num = frames_.size();

  frames_.resize(frame_num + 1);

  FrameData* frame = &frames_.at(frame_num);

  frame->num = frame_num;

  frame->pc = regs_->pc();

  frame->sp = regs_->sp();

  frame->rel_pc = rel_pc;

  frame->rel_pc = adjusted_rel_pc;



  if (map_info == nullptr) {

    frame->pc = regs_->pc();

    return;

  }



  if (adjust_pc) {

    // Don't adjust the first frame pc.

    frame->rel_pc = regs_->GetAdjustedPc(rel_pc, elf);



    // Adjust the original pc.

    frame->pc -= rel_pc - frame->rel_pc;

  }



  frame->pc = map_info->start + adjusted_rel_pc;

  frame->map_name = map_info->name;

  frame->map_offset = map_info->offset;

  frame->map_start = map_info->start;
@@ -92,21 +85,29 @@ void Unwinder::Unwind(const std::vector<std::string>* initial_map_names_to_skip, 


    MapInfo* map_info = maps_->Find(regs_->pc());

    uint64_t rel_pc;

    uint64_t adjusted_rel_pc;

    Elf* elf;

    if (map_info == nullptr) {

      rel_pc = regs_->pc();

      adjusted_rel_pc = rel_pc;

    } else {

      if (ShouldStop(map_suffixes_to_ignore, map_info->name)) {

        break;

      }

      elf = map_info->GetElf(process_memory_, true);

      rel_pc = elf->GetRelPc(regs_->pc(), map_info);

      if (adjust_pc) {

        adjusted_rel_pc = regs_->GetAdjustedPc(rel_pc, elf);

      } else {

        adjusted_rel_pc = rel_pc;

      }

    }



    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()) {

      FillInFrame(map_info, elf, rel_pc, adjust_pc);

      FillInFrame(map_info, elf, adjusted_rel_pc);



      // Once a frame is added, stop skipping frames.

      initial_map_names_to_skip = nullptr;

    }
@@ -133,7 +134,8 @@ void Unwinder::Unwind(const std::vector<std::string>* initial_map_names_to_skip, 
          in_device_map = true;

        } else {

          bool finished;

          stepped = elf->Step(rel_pc, map_info->elf_offset, regs_, process_memory_.get(), &finished);

          stepped = elf->Step(rel_pc, adjusted_rel_pc, map_info->elf_offset, regs_,

                              process_memory_.get(), &finished);

          if (stepped && finished) {

            break;

          }