Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for symbolicating APK/ZIP-embedded libraries on Android #662

Merged

Conversation

sudoBash418
Copy link
Contributor

@sudoBash418 sudoBash418 commented Sep 3, 2024

Fixes #661

Add support for symbolicating APK/ZIP-embedded libraries by doing the following:

In native_libraries:

  1. Each path is checked for the !/ substring
  2. If found, find the matching mapping in /proc/self/maps and use the offset field to determine where the library starts in the archive

In Mapping::new_android (non-embedded libraries use the same logic as before):

  1. No attempts to find separate debug info files are made
  2. Libraries are mapped starting from the offset from native_libraries and ending at the end of the APK.
    (the cost of mmap-ing the unused remainder of the APK should be minimal)

Aside: my previous attempt used the zip crate to determine the exact size of the library and avoid mmap-ing the rest of the archive. But I figured it wasn't worth bringing in a new dependency and bloating the example binary by ~25% 😅

As for potential confusion between a file literally named /path/to/apk!/lib.so and a file named lib.so within /path/to/apk, I tried to follow the dynamic linker's behaviour: treat it as an embedded file first, and fallback to a literal path if mapping fails for any reason.

Tests have been skipped because there is no CI that runs the emulated binaries anyways.

Copy link

github-actions bot commented Sep 3, 2024

Code size changes for a hello-world Rust program linked with libstd with backtrace:

On platform ubuntu-latest:

  • Original binary size: 362,992 B
  • Updated binary size: 367,088 B
  • Difference: 4,096 B (1.13%)

Copy link

github-actions bot commented Sep 3, 2024

Code size changes for a hello-world Rust program linked with libstd with backtrace:

On platform ubuntu-latest:

  • Original binary size: 362,992 B
  • Updated binary size: 367,088 B
  • Difference: 4,096 B (1.13%)

Copy link

github-actions bot commented Sep 3, 2024

Code size changes for a hello-world Rust program linked with libstd with backtrace:

On platform ubuntu-latest:

  • Original binary size: 362,992 B
  • Updated binary size: 367,088 B
  • Difference: 4,096 B (1.13%)

Copy link

github-actions bot commented Sep 3, 2024

Code size changes for a hello-world Rust program linked with libstd with backtrace:

On platform ubuntu-latest:

  • Original binary size: 362,992 B
  • Updated binary size: 367,088 B
  • Difference: 4,096 B (1.13%)

Copy link

github-actions bot commented Sep 3, 2024

Code size changes for a hello-world Rust program linked with libstd with backtrace:

On platform ubuntu-latest:

  • Original binary size: 362,992 B
  • Updated binary size: 367,088 B
  • Difference: 4,096 B (1.13%)

@sudoBash418
Copy link
Contributor Author

The remaining macOS CI failure seems unrelated to this PR.

Not sure why the binary size check reports an increase on Ubuntu; AFAIK my changes should have minimal impact on non-Android targets.

@ChrisDenton
Copy link
Member

The macos failure is caused by a new lint in the nightly compiler. #664 fixes it.

Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please.

@sudoBash418
Copy link
Contributor Author

Rebased and added requested changes.

Also updated the dl_iterate_phdr safety doc + parameter name, which I missed earlier.

@sudoBash418
Copy link
Contributor Author

Looks like the binary size check is broken by the dependency bump I just rebased on, whoops.

@workingjubilee
Copy link
Member

That's fine. It really ought to be fixed but whatever.

Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last(?) fussing about types, and most of the rest is just code and logic organization, and documenting the invariants correctly so as to make auditing and refactoring easy. This is an... oddly-organized library that has grown over quite a long time, before I even started programming, nevermind learned Rust. A significant amount of effort necessarily goes into just keeping this overgrown garden from finally bursting out of its walls, and the current organization doesn't really help you in that. You have my sympathies.

src/symbolize/gimli/mmap_fake.rs Outdated Show resolved Hide resolved
src/symbolize/gimli/libs_dl_iterate_phdr.rs Outdated Show resolved Hide resolved
src/symbolize/gimli/mmap_windows.rs Outdated Show resolved Hide resolved
src/symbolize/gimli.rs Outdated Show resolved Hide resolved
src/symbolize/gimli/libs_dl_iterate_phdr.rs Outdated Show resolved Hide resolved
src/symbolize/gimli/elf.rs Outdated Show resolved Hide resolved
src/symbolize/gimli/mmap_unix.rs Outdated Show resolved Hide resolved
src/symbolize/gimli/mmap_fake.rs Outdated Show resolved Hide resolved
src/symbolize/gimli/elf.rs Outdated Show resolved Hide resolved
src/symbolize/gimli/libs_dl_iterate_phdr.rs Outdated Show resolved Hide resolved
@workingjubilee
Copy link
Member

@sudoBash418 The reason that Windows demands you align the mapping to 64KiB is because that is the value of dwAllocationGranularity... a sort of virtual page size? Except not really. dwAllocationGranularity means that you lock down that much address space, but the actual pages are still faulted one by one within that region.

Comment on lines +93 to +96
// find MapsEntry matching library's base address and get its file offset
maps.iter()
.find(|m| m.ip_matches(dlpi_addr as usize))
.map(|m| m.offset())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self: reread the MapsEntry struct and its FromStr if you somehow develop another question about this.

@workingjubilee
Copy link
Member

Hm, this looks good but...

...ah, right.

Can you construct a test for specifically symbolicating a .so in a zip for Android? It likely has to be its own crate, like the other tests. Honestly, though, a bash script recorded from what you've been doing a zillion times I'm sure, and just plonked into the workflow yaml with a conditional to run it only on android, would also be fine.

With that, I can accept this!

@workingjubilee
Copy link
Member

...wait.

# TODO: run tests in an emulator eventually

...nevermind, we're only testing linkage on Android apparently.

@workingjubilee
Copy link
Member

I believe I can accept this with a rebase.

By default, modern Android build tools will store native libraries
uncompressed, and the loader will map them directly from the APK
(instead of the package manager extracting them on installation).

This commit adds support for symbolicating these embedded libraries.

To avoid parsing ZIP structures, the offset of the library within the
archive is determined via /proc/self/maps.

ref: https://android.googlesource.com/platform/bionic/+/main/android-changes-for-ndk-developers.md#opening-shared-libraries-directly-from-an-apk
@workingjubilee workingjubilee merged commit 9f98e8e into rust-lang:master Sep 9, 2024
40 of 41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Symbolication fails on Android with APK-embedded libraries
3 participants