New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 843987 link

Starred by 2 users

Issue metadata

Status: Assigned
Owner:
Last visit > 30 days ago
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Bug



Sign in to add a comment

Android crazy linker: Better detect and load system libraries

Project Member Reported by digit@google.com, May 17 2018

Issue description

As stated in http://crbug.com/843804, code in libchrome.so that calls dlopen() to load a system library like libGLESv2.so, ends up loading it with the crazy linker, instead of the system one.

The crazy linker is not designed to support this use case (in particular, it doesn't support ELF features that system libraries may rely on, like DT_RUNPATH entries, or GNU-format hash tables), and makes no guarantees that this will always work. Its purpose should be to load Chrome libraries properly, and provide transparent dlopen() wrappers to them.

The initial release actually recognized NDK-approved system libraries, to properly use the system ::dlopen() to load them, but this was removed in a previous CL at https://codereview.chromium.org/1105893002/

The motivation for the removal was to support LD_PRELOAD, which is set on certain OEM devices (due to OEM customizations), on Android 5.0 (but not 5.1) due to the use of libsigaction.so, and when enable ASAN at runtime.

This issue is created to provide a better way to detect which libraries should be opened with the system linker, when a wrapped dlopen() is performed, in a way that is compatible with all LD_PRELOAD uses, and system linker variants.

 
Project Member

Comment 1 by bugdroid1@chromium.org, Jun 7 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ad16aee9590a3ba86af8c8a26acf24de4c230ac2

commit ad16aee9590a3ba86af8c8a26acf24de4c230ac2
Author: David 'Digit' Turner <digit@google.com>
Date: Thu Jun 07 21:08:13 2018

android: crazy-linker: Simplify zip library loading.

Use SearchPathList::Find()'s ability to look directly inside
a zip archive if the path contains a ! separator to simplify
LibraryList::LoadLibraryFromZipFile().

+ LibraryList::LoadLibraryWithSystemLinker() is introduced to
  greatly simplify the code flow and the LibraryList::LoadLibrary()
  parameter list.

This is part of an ongoing effort to support the component build,
cleanup the crazy linker, and better detect system libraries.

NOTE: The LegacyLinker.java change is necessary to ensure
      RELRO sharing works properly. The issue was that the
      RELRO was associated with the file path of the APK itself,
      instead of the library SO_NAME.

BUG=802068,843987
R=pasko@chromium.org, agrieve@chromium.org, rcmilroy@chromium.org

Change-Id: I833b02635f71d0bf63794681ba78e9a33647e204
Reviewed-on: https://chromium-review.googlesource.com/1091058
Reviewed-by: agrieve <agrieve@chromium.org>
Commit-Queue: David Turner <digit@chromium.org>
Cr-Commit-Position: refs/heads/master@{#565407}
[modify] https://crrev.com/ad16aee9590a3ba86af8c8a26acf24de4c230ac2/base/android/java/src/org/chromium/base/library_loader/LegacyLinker.java
[modify] https://crrev.com/ad16aee9590a3ba86af8c8a26acf24de4c230ac2/third_party/android_crazy_linker/src/src/crazy_linker_api.cpp
[modify] https://crrev.com/ad16aee9590a3ba86af8c8a26acf24de4c230ac2/third_party/android_crazy_linker/src/src/crazy_linker_library_list.cpp
[modify] https://crrev.com/ad16aee9590a3ba86af8c8a26acf24de4c230ac2/third_party/android_crazy_linker/src/src/crazy_linker_library_list.h
[modify] https://crrev.com/ad16aee9590a3ba86af8c8a26acf24de4c230ac2/third_party/android_crazy_linker/src/src/crazy_linker_library_view.h
[modify] https://crrev.com/ad16aee9590a3ba86af8c8a26acf24de4c230ac2/third_party/android_crazy_linker/src/src/crazy_linker_wrappers.cpp

Project Member

Comment 2 by bugdroid1@chromium.org, Jun 11 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2d9ea2afcd2a6911c7f54a945d9b41cb217ea33e

commit 2d9ea2afcd2a6911c7f54a945d9b41cb217ea33e
Author: David 'Digit' Turner <digit@google.com>
Date: Mon Jun 11 17:08:41 2018

chromium_android_linker: Simplify loading from zip archives.

Now that the crazy linker supports finding and loading libraries
directly from zip archives, by adding a search path item that
contains an exclamation mark, use that feature to simplify
the chromium android linker Java and C++ sources.

This allow us to remove the ugly function
crazy_linker_load_library_from_zip_file() and the code it
depends on from the crazy linker as well.

BUG=802068,843987
R=agrieve@chromium.org, rmcilroy@chromium.org, pasko@chromium.org

Change-Id: I15a5493c1bd7e9b5bbc40185e947db42fe5779b9
Reviewed-on: https://chromium-review.googlesource.com/1094641
Reviewed-by: agrieve <agrieve@chromium.org>
Commit-Queue: David Turner <digit@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566036}
[modify] https://crrev.com/2d9ea2afcd2a6911c7f54a945d9b41cb217ea33e/base/android/java/src/org/chromium/base/library_loader/LegacyLinker.java
[modify] https://crrev.com/2d9ea2afcd2a6911c7f54a945d9b41cb217ea33e/base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
[modify] https://crrev.com/2d9ea2afcd2a6911c7f54a945d9b41cb217ea33e/base/android/java/src/org/chromium/base/library_loader/Linker.java
[modify] https://crrev.com/2d9ea2afcd2a6911c7f54a945d9b41cb217ea33e/base/android/linker/legacy_linker_jni.cc
[modify] https://crrev.com/2d9ea2afcd2a6911c7f54a945d9b41cb217ea33e/third_party/android_crazy_linker/src/include/crazy_linker.h
[modify] https://crrev.com/2d9ea2afcd2a6911c7f54a945d9b41cb217ea33e/third_party/android_crazy_linker/src/src/crazy_linker_api.cpp
[modify] https://crrev.com/2d9ea2afcd2a6911c7f54a945d9b41cb217ea33e/third_party/android_crazy_linker/src/src/crazy_linker_library_list.cpp
[modify] https://crrev.com/2d9ea2afcd2a6911c7f54a945d9b41cb217ea33e/third_party/android_crazy_linker/src/src/crazy_linker_library_list.h

Project Member

Comment 3 by bugdroid1@chromium.org, Aug 10

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/901d58a2649bd8977365b4af8c6159bbd5d5e70b

commit 901d58a2649bd8977365b4af8c6159bbd5d5e70b
Author: David 'Digit' Turner <digit@google.com>
Date: Fri Aug 10 08:00:56 2018

android: crazy_linker: Minor cleanups.

Minor cleanups to the code base. Preparing for future changes.

BUG=843987,802068
R=agrieve@chromium.org, pasko@chromium.org, rmcilroy@chromium.org

Change-Id: I65598259521b3e21b540cedadba99cfa58f13051
Reviewed-on: https://chromium-review.googlesource.com/1169480
Reviewed-by: agrieve <agrieve@chromium.org>
Reviewed-by: Ross McIlroy <rmcilroy@chromium.org>
Reviewed-by: Egor Pasko <pasko@chromium.org>
Commit-Queue: David Turner <digit@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582088}
[modify] https://crrev.com/901d58a2649bd8977365b4af8c6159bbd5d5e70b/third_party/android_crazy_linker/BUILD.gn
[modify] https://crrev.com/901d58a2649bd8977365b4af8c6159bbd5d5e70b/third_party/android_crazy_linker/src/src/crazy_linker_pointer_set.cpp
[modify] https://crrev.com/901d58a2649bd8977365b4af8c6159bbd5d5e70b/third_party/android_crazy_linker/src/src/crazy_linker_pointer_set.h
[modify] https://crrev.com/901d58a2649bd8977365b4af8c6159bbd5d5e70b/third_party/android_crazy_linker/src/src/crazy_linker_pointer_set_unittest.cpp
[modify] https://crrev.com/901d58a2649bd8977365b4af8c6159bbd5d5e70b/third_party/android_crazy_linker/src/src/crazy_linker_system.cpp
[modify] https://crrev.com/901d58a2649bd8977365b4af8c6159bbd5d5e70b/third_party/android_crazy_linker/src/src/crazy_linker_system_mock.cpp
[modify] https://crrev.com/901d58a2649bd8977365b4af8c6159bbd5d5e70b/third_party/android_crazy_linker/src/src/crazy_linker_util.cpp
[modify] https://crrev.com/901d58a2649bd8977365b4af8c6159bbd5d5e70b/third_party/android_crazy_linker/src/src/crazy_linker_util.h
[modify] https://crrev.com/901d58a2649bd8977365b4af8c6159bbd5d5e70b/third_party/android_crazy_linker/src/src/crazy_linker_util_unittest.cpp
[modify] https://crrev.com/901d58a2649bd8977365b4af8c6159bbd5d5e70b/third_party/android_crazy_linker/src/src/crazy_linker_wrappers.cpp

Project Member

Comment 4 by bugdroid1@chromium.org, Aug 10

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a6a2898fbc25e7b76550954eea99df00a3fda590

commit a6a2898fbc25e7b76550954eea99df00a3fda590
Author: David 'Digit' Turner <digit@google.com>
Date: Fri Aug 10 10:05:49 2018

android: crazy_linker: Better symbol search implementation.

This CL improves a few functions related to symbol search
and relocation. This prepares for a major change that will
considerably change the way symbols are resolved during
relocations and dlsym(), to better match default ELF
semantics and help fix the component build as well.

- Use a SearchResult result when looking for symbols,
  in order to allow symbols with a null address to be
  supported (this can happen for weak symbol exports).

  See SystemLinker::SearchResult and the related
  LibraryView::SearchResult struct definitions.

- Ensure that SharedLibraryResolver properly releases
  its global scope handle on destruction. Since these
  correspond to a reference-counted global object, that
  will never be destroyed by the system linker, this
  was not a real resource leak, but is still the
  correct way to deal with this.

- Add a comment to SharedLibraryResolver::Lookup()
  explaining why the code is written and will need to
  be refactored in a future CL.

- Make LibraryView::LookupSymbol() const (finally!)

- Use a few const references, instead of raw pointers
  when calling SharedLibrary::Relocate().

BUG=843987,802068
R=pasko@chromium.org, rmcilroy@chromium.org, agrieve@chromium.org

android: crazy_linker: Minor cleanups.

Minor cleanups to the code base. Preparing for future changes.

BUG=NONE
R=agrieve@chromium.org, pasko@chromium.org, rmcilroy@chromium.org

Change-Id: I91c955c7be24e3738b118832c9ae8775934bc572
Reviewed-on: https://chromium-review.googlesource.com/1169603
Commit-Queue: David Turner <digit@chromium.org>
Reviewed-by: agrieve <agrieve@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582103}
[modify] https://crrev.com/a6a2898fbc25e7b76550954eea99df00a3fda590/third_party/android_crazy_linker/src/src/crazy_linker_api.cpp
[modify] https://crrev.com/a6a2898fbc25e7b76550954eea99df00a3fda590/third_party/android_crazy_linker/src/src/crazy_linker_library_list.cpp
[modify] https://crrev.com/a6a2898fbc25e7b76550954eea99df00a3fda590/third_party/android_crazy_linker/src/src/crazy_linker_library_list.h
[modify] https://crrev.com/a6a2898fbc25e7b76550954eea99df00a3fda590/third_party/android_crazy_linker/src/src/crazy_linker_library_view.cpp
[modify] https://crrev.com/a6a2898fbc25e7b76550954eea99df00a3fda590/third_party/android_crazy_linker/src/src/crazy_linker_library_view.h
[modify] https://crrev.com/a6a2898fbc25e7b76550954eea99df00a3fda590/third_party/android_crazy_linker/src/src/crazy_linker_shared_library.cpp
[modify] https://crrev.com/a6a2898fbc25e7b76550954eea99df00a3fda590/third_party/android_crazy_linker/src/src/crazy_linker_shared_library.h
[modify] https://crrev.com/a6a2898fbc25e7b76550954eea99df00a3fda590/third_party/android_crazy_linker/src/src/crazy_linker_system_linker.cpp
[modify] https://crrev.com/a6a2898fbc25e7b76550954eea99df00a3fda590/third_party/android_crazy_linker/src/src/crazy_linker_system_linker.h
[modify] https://crrev.com/a6a2898fbc25e7b76550954eea99df00a3fda590/third_party/android_crazy_linker/src/src/crazy_linker_wrappers.cpp

Project Member

Comment 5 by bugdroid1@chromium.org, Sep 20

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c9bffc922e813e0f9572b4baf74db5cdbb7b166e

commit c9bffc922e813e0f9572b4baf74db5cdbb7b166e
Author: David 'Digit' Turner <digit@google.com>
Date: Thu Sep 20 17:43:52 2018

android: crazy_linker: Load system libraries with system linker.

Add a way to detect system libraries, and fallback to the system
linker whenever they need to be loaded. This is needed because

Chrome probes a few system libraries like libEGL.so to check that
certain GL-related symbols are available. It does that by using
dlopen() + multiple dlsym() + dlclose(), which end up being
performed through the crazy linker wrappers (e.g. WrapDlopen()).

When enabling the crazy linker, it seems this happens everytime
the user navigates to a different web page!

Due to the way the code works before this CL, this ended up
loading said system libraries with the crazy linker, which
is problematic because:

- If the system library was already loaded in the Zygote,
  this would create a duplicate copy, loaded at a different
  address. Also this is much slower than simply incrementing
  the reference-count of an existing system library entry.

- This creates pending RDebug-related tasks, which happen to
  be racy, and the source of runtime crashes.

By detecting system libraries properly, the code can now
always fall back to use the system linker to load them,
avoiding these two problems entirely.

In particular, it is hoped that this will decrease the
amount of runtime crashes described in 831403.

BUG=843987,831403

R=pasko@chromium.org, agrieve@chromium.org, rcmilroy@chromium.org

Change-Id: I86d89d6a812b778cf962aa5da028449b2b79f1fa
Reviewed-on: https://chromium-review.googlesource.com/1236656
Reviewed-by: agrieve <agrieve@chromium.org>
Commit-Queue: David Turner <digit@chromium.org>
Cr-Commit-Position: refs/heads/master@{#592861}
[modify] https://crrev.com/c9bffc922e813e0f9572b4baf74db5cdbb7b166e/third_party/android_crazy_linker/src/src/crazy_linker_library_list.cpp
[modify] https://crrev.com/c9bffc922e813e0f9572b4baf74db5cdbb7b166e/third_party/android_crazy_linker/src/src/crazy_linker_system.cpp
[modify] https://crrev.com/c9bffc922e813e0f9572b4baf74db5cdbb7b166e/third_party/android_crazy_linker/src/src/crazy_linker_system.h
[modify] https://crrev.com/c9bffc922e813e0f9572b4baf74db5cdbb7b166e/third_party/android_crazy_linker/src/src/crazy_linker_system_unittest.cpp
[modify] https://crrev.com/c9bffc922e813e0f9572b4baf74db5cdbb7b166e/third_party/android_crazy_linker/src/src/crazy_linker_wrappers.cpp

Sign in to add a comment