[llvm-bugs] [Bug 46743] New: libunwind's FrameHeaderCache looks broken on Android 10 and earlier

[via llvm-bugs]

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

            Bug ID: 46743
           Summary: libunwind's FrameHeaderCache looks broken on Android
                    10 and earlier
           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

libunwind now uses the dlpi_adds and dlpi_subs fields in dl_phdr_info (the
callback struct for dl_iterate_phdr) to detect an unloaded module and
invalidate its cache:

https://reviews.llvm.org/D75954

While these fields were common in glibc/BSD/etc for many years, they're still
very new in Android. (They're not in Android 10, but will be in the next
version.)

https://android-review.googlesource.com/c/platform/bionic/+/1104597/

libunwind isn't checking the callback size parameter, so AFAICT, on current
versions of Android, it will read bogus stack memory. If a module is unloaded
(dlclose), then libunwind might fail to invalidate a cache entry.

This issue won't affect ARM32 (because it uses dl_unwind_find_exidx), which is
the only target that Android proper (the platform and NDK) officially use
libunwind for. Android plans to switch to libunwind for the other
architectures, though, and someone in the larger Android community might be
using libunwind on other architectures.

libgcc also uses these fields, but it verifies that `size` is large enough
before assuming the extended dl_phdr_info fields exists. Maybe libunwind could
use a check like:

    (size >= offsetof(dl_phdr_info, dlpi_subs) + sizeof(...->dlpi_subs))

Aside: IIUC, on a cache miss, we're still scanning each entry of the frame
cache, for each module passed to the callback. That seems slow? I would think
we would want to check the cache on only the first callback invocation, and
we'd want to check the cache even when this condition holds:

    pinfo->dlpi_phnum == 0 || cbdata->targetAddr < pinfo->dlpi_addr

Also: A frame cache is still possible (and probably very helpful) on versions
of Android before R, but it requires special logic. On a cache hit, it's
necessary to scan the modules to verify the hit. This scan can be very quick
compared to the current behavior. I'd like to prototype this, but as it's only
useful for Android, I'm not sure how interested upstream would be.

-- 
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/20200716/0dd94079/attachment.html>