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.

Sign up for GitHub

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

Jump to bottom

[BUG] libc++_shared.so exposes libgcc _Unwind_* symbols #1166

Closed
rprichard opened this issue Jan 7, 2020 · 1 comment
Closed

[BUG] libc++_shared.so exposes libgcc _Unwind_* symbols #1166

rprichard opened this issue Jan 7, 2020 · 1 comment
Labels
bug
Milestone
r21b

Comments

@rprichard
Copy link
Collaborator

AFAIK, we're trying to hide the libgcc unwinder symbols to avoid the possibility of linking against versions of the symbols from the platform.

I think this is another consequence of the libgcc.a / libgcc_real.a changes.

Maybe https://android-review.googlesource.com/c/platform/ndk/+/1133109 is related?

master

$ for lib in /x/ndk/out/android-ndk-r22-canary/sources/cxx-stl/llvm-libc++/libs/*/libc++_shared.so; do printf '\n%s\n' $lib; readelf --dyn-syms -W $lib | grep ' _Unwind_Resume$'; done

/x/ndk/out/android-ndk-r22-canary/sources/cxx-stl/llvm-libc++/libs/arm64-v8a/libc++_shared.so
  1930: 00000000000b5e40   252 FUNC    GLOBAL DEFAULT    11 _Unwind_Resume

/x/ndk/out/android-ndk-r22-canary/sources/cxx-stl/llvm-libc++/libs/armeabi-v7a/libc++_shared.so

/x/ndk/out/android-ndk-r22-canary/sources/cxx-stl/llvm-libc++/libs/x86_64/libc++_shared.so
  1920: 00000000000c0e70   193 FUNC    GLOBAL DEFAULT    13 _Unwind_Resume

/x/ndk/out/android-ndk-r22-canary/sources/cxx-stl/llvm-libc++/libs/x86/libc++_shared.so
  1890: 000b9723   176 FUNC    GLOBAL DEFAULT    13 _Unwind_Resume

r21 branch

$ for lib in /x/ndk-release-r21/out/android-ndk-r21/sources/cxx-stl/llvm-libc++/libs/*/libc++_shared.so; do printf '\n%s\n' $lib; readelf --dyn-syms -W $lib | grep ' _Unwind_Resume$'; done

/x/ndk-release-r21/out/android-ndk-r21/sources/cxx-stl/llvm-libc++/libs/arm64-v8a/libc++_shared.so
  1930: 00000000000b5e40   252 FUNC    GLOBAL DEFAULT    11 _Unwind_Resume

/x/ndk-release-r21/out/android-ndk-r21/sources/cxx-stl/llvm-libc++/libs/armeabi-v7a/libc++_shared.so

/x/ndk-release-r21/out/android-ndk-r21/sources/cxx-stl/llvm-libc++/libs/x86_64/libc++_shared.so
  1920: 00000000000c0e70   193 FUNC    GLOBAL DEFAULT    13 _Unwind_Resume

/x/ndk-release-r21/out/android-ndk-r21/sources/cxx-stl/llvm-libc++/libs/x86/libc++_shared.so
  1890: 000b9723   176 FUNC    GLOBAL DEFAULT    13 _Unwind_Resume

r20

$ for lib in /x/android-ndk-r20/sources/cxx-stl/llvm-libc++/libs/*/libc++_shared.so; do printf '\n%s\n' $lib; readelf --dyn-syms -W $lib | grep ' _Unwind_Resume$'; done

/x/android-ndk-r20/sources/cxx-stl/llvm-libc++/libs/arm64-v8a/libc++_shared.so

/x/android-ndk-r20/sources/cxx-stl/llvm-libc++/libs/armeabi-v7a/libc++_shared.so

/x/android-ndk-r20/sources/cxx-stl/llvm-libc++/libs/x86_64/libc++_shared.so

/x/android-ndk-r20/sources/cxx-stl/llvm-libc++/libs/x86/libc++_shared.so
@rprichard rprichard added the bug label Jan 7, 2020
@rprichard rprichard added this to the r21b milestone Jan 14, 2020
@Grimler91 Grimler91 mentioned this issue Feb 11, 2020
Closed
@DanAlbert
Copy link
Member

Ah, I see the issue. By hiding libgcc_real we also stopped hiding libgcc proper in all the other ABIs. Should be harmless, but I'll send a fix anyway.

disigma pushed a commit to wimal-build/ndk that referenced this issue Mar 7, 2020
@DanAlbert
Hide unwinder symbols on all architectures.
3977774 
Test: readelf
Bug: android/ndk#1166
Change-Id: I28b64492e5a66a66608874133ddd955ea2f53d7f
@brunotl brunotl mentioned this issue Aug 4, 2020
Closed
neildhar added a commit to neildhar/hermes that referenced this issue May 27, 2021
@neildhar @facebook-github-bot
Bump gradle version
8340357 
Summary:
This diff updates the minimum gradle plugin version for building the
Hermes OSS release in order to avoid problems in older versions of
the Android NDK.

Specifically, there was a bug
(android/ndk#1166) in the Android NDK that
caused the exception handling functions in `libc++` to be incorrectly
exported. Depending on link order, this can lead to  `libhermes.so`
not statically linking in `_Unwind_resume`, and expecting it instead
to be resolved dynamically to the version in `libc++`. However, if
we're linking against an RN version that was built against the new
version of the NDK, those symbols may not be exported, and end up
unresolved (which crashes the app).

To allow Hermes to be built with newer versions of the NDK, we need
to bump up the gradle plugin version to 4.1. RN already uses 4.1.

Differential Revision: D28763472

fbshipit-source-id: 617749b2b50dbede29205ed43cc3862e36c14ac8
@neildhar neildhar mentioned this issue May 27, 2021
Closed
neildhar added a commit to neildhar/hermes that referenced this issue May 28, 2021
@neildhar @facebook-github-bot
Bump gradle version (facebook#518)
9143549 
Summary:
Pull Request resolved: facebook#518

This diff updates the minimum gradle plugin version for building the
Hermes OSS release in order to avoid problems in older versions of
the Android NDK.

Specifically, there was a bug
(android/ndk#1166) in the Android NDK that
caused the exception handling functions in `libc++` to be incorrectly
exported. Depending on link order, this can lead to  `libhermes.so`
not statically linking in `_Unwind_resume`, and expecting it instead
to be resolved dynamically to the version in `libc++`. However, if
we're linking against an RN version that was built against the new
version of the NDK, those symbols may not be exported, and end up
unresolved (which crashes the app).

To allow Hermes to be built with newer versions of the NDK, we need
to bump up the gradle plugin version to 4.1. RN already uses 4.1.

Bumping the gradle version seems to break the intl tests unless we
add `useLibrary 'android.test.base'` to the gradle file, so do that.
(we're technically supposed to have that anyway)

Differential Revision: D28763472

fbshipit-source-id: 5706ee86d824339c6a3f0171992fe7ffe96cd188
facebook-github-bot pushed a commit to facebook/hermes that referenced this issue Jun 2, 2021
@neildhar @facebook-github-bot
Bump gradle version (#518)
f61a798 
Summary:
Pull Request resolved: #518

This diff updates the minimum gradle plugin version for building the
Hermes OSS release in order to avoid problems in older versions of
the Android NDK.

Specifically, there was a bug
(android/ndk#1166) in the Android NDK that
caused the exception handling functions in `libc++` to be incorrectly
exported. Depending on link order, this can lead to  `libhermes.so`
not statically linking in `_Unwind_resume`, and expecting it instead
to be resolved dynamically to the version in `libc++`. However, if
we're linking against an RN version that was built against the new
version of the NDK, those symbols may not be exported, and end up
unresolved (which crashes the app).

To allow Hermes to be built with newer versions of the NDK, we need
to bump up the gradle plugin version to 4.1. RN already uses 4.1.

Bumping the gradle version seems to break the intl tests unless we
add `useLibrary 'android.test.base'` to the gradle file, so do that.
(we're technically supposed to have that anyway)

Reviewed By: mhorowitz

Differential Revision: D28763472

fbshipit-source-id: 00ffcfa94ee83a9dee0ac894e20e83867200865b
Huxpro pushed a commit to facebook/hermes that referenced this issue Jul 12, 2021
@neildhar @Huxpro
Bump gradle version (#518)
b750ed4 
Summary:
Pull Request resolved: #518

This diff updates the minimum gradle plugin version for building the
Hermes OSS release in order to avoid problems in older versions of
the Android NDK.

Specifically, there was a bug
(android/ndk#1166) in the Android NDK that
caused the exception handling functions in `libc++` to be incorrectly
exported. Depending on link order, this can lead to  `libhermes.so`
not statically linking in `_Unwind_resume`, and expecting it instead
to be resolved dynamically to the version in `libc++`. However, if
we're linking against an RN version that was built against the new
version of the NDK, those symbols may not be exported, and end up
unresolved (which crashes the app).

To allow Hermes to be built with newer versions of the NDK, we need
to bump up the gradle plugin version to 4.1. RN already uses 4.1.

Bumping the gradle version seems to break the intl tests unless we
add `useLibrary 'android.test.base'` to the gradle file, so do that.
(we're technically supposed to have that anyway)

Reviewed By: mhorowitz

Differential Revision: D28763472

fbshipit-source-id: 00ffcfa94ee83a9dee0ac894e20e83867200865b
@bghgary bghgary mentioned this issue Sep 12, 2022
Merged
4 tasks
sepehrst pushed a commit to spsforks/android-bionic-libc that referenced this issue Apr 22, 2024
Update patch set 4
523b7cc 
Patch Set 4:

> Patch Set 4:
> 
> > Patch Set 4:
> > 
> > I want to reduce DSO proliferation on the system. Why do we need a libunwind.so instead of just exporting the unwinder from libc?
> 
> I think we could export the unwinder from libc, although this would mean that the unwinder symbols would be exposed to the NDK (we'd be adding them to LIBC_PRIVATE so they wouldn't appear in the NDK stubs but could still be used at runtime for symbol resolution). I think this is unlikely to cause problems but I'd like to get opinions from Bionic folks as well.

Adding LIBC_PRIVATE to the libc.so symbols doesn't help much unless the relocations from an NDK app use a non-empty version string that isn't LIBC_PRIVATE. If the relocation has no version string, then it will match a non-hidden LIBC_PRIVATE symbol.

It doesn't seem that difficult to have relocations to the unwinder without a version string. (It's currently happening with the libc++_shared.so in NDK r21 beta2 -- android/ndk#1166.) By default, the bfd/gold/lld linkers allow relocations to unresolved symbols in DSOs, though it is common to change this default using --no-undefined, -Z,defs, etc.

Patch-set: 4
Reviewer: Gerrit User 1229339 <1229339@85c56323-6fa9-3386-8a01-6480fb634889>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
r21b
Development

No branches or pull requests
2 participants
@DanAlbert @rprichard