[llvm-bugs] [Bug 46829] New: CFI_Parser::findFDE's end-of-section calculation looks broken

[via llvm-bugs]

https://bugs.llvm.org/show_bug.cgi?id=46829

            Bug ID: 46829
           Summary: CFI_Parser::findFDE's end-of-section calculation looks
                    broken
           Product: Runtime Libraries
           Version: trunk
          Hardware: PC
                OS: Linux
            Status: NEW
          Severity: enhancement
          Priority: P
         Component: libunwind
          Assignee: unassignedbugs at nondot.org
          Reporter: rprichard at google.com
                CC: llvm-bugs at lists.llvm.org

CFI_Parser<A>::findFDE scans an .eh_frame section looking for an FDE matching a
PC. The start of .eh_frame is passed as an argument, ehSectionStart, and the
function calculates the end of .eh_frame (ehSectionEnd):

    /// Scan an eh_frame section to find an FDE for a pc
    template <typename A>
    bool CFI_Parser<A>::findFDE(A &addressSpace, pint_t pc, pint_t
ehSectionStart,
                                uint32_t sectionLength, pint_t fdeHint,
                                FDE_Info *fdeInfo, CIE_Info *cieInfo) {
      //fprintf(stderr, "findFDE(0x%llX)\n", (long long)pc);
      pint_t p = (fdeHint != 0) ? fdeHint : ehSectionStart;
      const pint_t ehSectionEnd = p + sectionLength;
      while (p < ehSectionEnd) {

There are two problems with the calculation:

 * AFAICT, when a hint is provided, ehSectionEnd should still use
ehSectionStart instead of p.

 * sectionLength comes from UnwindInfoSections::dwarf_section_length, which on
targets using dl_iterate_phdr, is the length of a PT_LOAD segment, not the
length of .eh_frame.

To expand on the second bullet point, UnwindInfoSections has (among others)
these five fields:

 - dso_base
 - dwarf_section
 - dwarf_section_length
 - dwarf_index_section
 - dwarf_index_section_length

dwarf_index_section and dwarf_index_section_length are the start and size of
the .eh_frame_hdr section, which indexes into .eh_frame and allows binary
searching.

I'm not sure what dso_base is, generally, but for targets that use
dl_iterate_phdr, it seems to be the start of a PT_LOAD segment associated with
the unwind info.

dwarf_section is always the start of .eh_frame.

For most configurations, dwarf_section_length is the size of .eh_frame, and
UnwindCursor::getInfoFromDwarfSection assumes this is the case when it calls
CFI_Parser::findFDE. For targets using dl_iterate_phdr, however,
dwarf_section_length is instead the size of the PT_LOAD segment indicated by
dso_base.

The recently-added FrameHeaderCache uses (dso_base, dwarf_section_length) to
indicate each cache entry's range of PC values. That happens to match how we're
initializing those fields. (I think initializing dwarf_section_length to the
PT_LOAD size predated the new cache.)

I'm unsure of the practical consequences. I suppose if the unwinder tries to
look up a function with no FDE, then it will enter this .eh_frame linear scan
fallback, and it could read past the end of .eh_frame, unless there's an end
marker (cfiLength == 0, "return false; // end marker").

---

Aside: On targets that use .eh_frame_hdr, it seems like a waste of code to scan
.eh_frame (and a waste of memory in FrameHeaderCache to track .eh_frame
bounds). I think these targets have to locate .eh_frame using .eh_frame_hdr, so
it shouldn't ever find a match. Some Apple targets are different, because the
compact unwind table can defer to using the .eh_frame scan
(compactSaysUseDwarf).

Aside 2: If the .eh_frame scan does find a match, it would add the FDE to
DwarfFDECache, whose entries are not invalidated on module unload (except with
Apple OS's). On Android, the lack of automatic per-module invalidation should
be OK as long as DwarfFDECache is only used for manually-registered FDEs
(__register_frame / __unw_add_dynamic_fde).

Aside 3: The DwarfFDECache offset in getInfoFromDwarfSection is not quite a
"hint". It's used as an offset into .eh_frame, so if it's wrong, we'll try to
decode arbitrary data and may crash. If the hinted-scan ends without a match,
then we try again with a complete .eh_frame scan. If *that* scan turns up an
FDE, then we add it to the end of DwarfFDECache. Adding an entry doesn't remove
the earlier entry, though, so AFAICT, each unwind lookup would find the bad
hint and add another good hint to the end.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-bugs/attachments/20200724/a5a78ca0/attachment.html>