From b1d17d6e31f8fceb4fbe5fbd99aa443d79c77c19 Mon Sep 17 00:00:00 2001 From: Dan Albert Date: Fri, 4 Oct 2019 12:40:52 -0700 Subject: [PATCH] Fix unwinder visibility for most ABIs. When we switched to using a linker script for libgcc in r19 we broke `-Wl,--exclude-libs,libgcc.a`, because `--exclude-libs` has no effect on the contents of linker scripts. As such, libgcc was not being hidden as expected. The good news is that on the one platform this causes the most trouble (Arm32, since it uses the LLVM unwinder for exception handling rather than libgcc) this didn't cause any issues. Since libunwind was linked first (and the `--exclude-libs` worked there, since the actual library was named), the libgcc unwind symbols did not end up in the output at all. So while this has not caused any issues, it is still a good idea for us to link libgcc hidden to guard against any future unwind ABI breaks. Test: Added a test that checks ndk-build output with readelf Bug: https://github.com/android/ndk/issues/1092 Change-Id: I5c8203afd29f0b7b272a51aadfc1a5ca6cbd33e4 --- build/cmake/android.toolchain.cmake | 2 +- build/core/default-build-commands.mk | 2 +- docs/BuildSystemMaintainers.md | 11 +-- docs/changelogs/Changelog-r21.md | 5 ++ ndk/hosts.py | 7 +- ndk/test/types.py | 1 + tests/build/unwinder_hidden/__init__.py | 0 .../unwinder_hidden/project/jni/Android.mk | 7 ++ .../project/jni/Application.mk | 1 + .../build/unwinder_hidden/project/jni/foo.cpp | 3 + tests/build/unwinder_hidden/test.py | 82 +++++++++++++++++++ 11 files changed, 108 insertions(+), 13 deletions(-) create mode 100644 tests/build/unwinder_hidden/__init__.py create mode 100644 tests/build/unwinder_hidden/project/jni/Android.mk create mode 100644 tests/build/unwinder_hidden/project/jni/Application.mk create mode 100644 tests/build/unwinder_hidden/project/jni/foo.cpp create mode 100644 tests/build/unwinder_hidden/test.py diff --git a/build/cmake/android.toolchain.cmake b/build/cmake/android.toolchain.cmake index 5ad98d95c..9d7e50902 100644 --- a/build/cmake/android.toolchain.cmake +++ b/build/cmake/android.toolchain.cmake @@ -347,7 +347,7 @@ if(ANDROID_LD STREQUAL lld) endif() # Don't re-export libgcc symbols in every binary. -list(APPEND ANDROID_LINKER_FLAGS -Wl,--exclude-libs,libgcc.a) +list(APPEND ANDROID_LINKER_FLAGS -Wl,--exclude-libs,libgcc_real.a) list(APPEND ANDROID_LINKER_FLAGS -Wl,--exclude-libs,libatomic.a) # STL. diff --git a/build/core/default-build-commands.mk b/build/core/default-build-commands.mk index 4a71f5e0b..138707bdb 100644 --- a/build/core/default-build-commands.mk +++ b/build/core/default-build-commands.mk @@ -84,7 +84,7 @@ endef cmd-strip = $(PRIVATE_STRIP) $(PRIVATE_STRIP_MODE) $(call host-path,$1) -TARGET_LIBGCC = -lgcc -Wl,--exclude-libs,libgcc.a +TARGET_LIBGCC = -lgcc -Wl,--exclude-libs,libgcc_real.a TARGET_LIBATOMIC = -latomic -Wl,--exclude-libs,libatomic.a TARGET_LDLIBS := -lc -lm diff --git a/docs/BuildSystemMaintainers.md b/docs/BuildSystemMaintainers.md index 8c12ae0ce..d23860a3c 100644 --- a/docs/BuildSystemMaintainers.md +++ b/docs/BuildSystemMaintainers.md @@ -416,9 +416,10 @@ preserved. By default, only symbols in used sections will be included in the linked binary. If this behavior is not desired for your build system, ensure that these flags -are at least used for libgcc.a and libunwind.a (libunwind is only used for -ARM32). This is necessary to avoid unwinding bugs on ARM32. See [Unwinding] for -more information. +are at least used for `libgcc_real.a` (`libgcc.a` is a linker script, and +`--exclude-libs` does not have any effect on the contents of linker scripts) and +`libunwind.a` (libunwind is only used for ARM32). This is necessary to avoid +unwinding bugs on ARM32. See [Unwinding] for more information. [visibility]: https://gcc.gnu.org/wiki/Visibility @@ -518,8 +519,8 @@ these symbols from a shared library. If this library was built with the wrong unwinder, it is possible for one unwinder to call into the other. As they are not compatible, this will likely result in either a crash or a failed unwind. To avoid this problem, libraries should always be built with -`-Wl,--exclude-libs,libgcc.a` and `-Wl,--exclude-libs,libunwind.a` (the latter -is only necessary for 32-bit ARM) to ensure that unwind symbols are not +`-Wl,--exclude-libs,libgcc_real.a` and `-Wl,--exclude-libs,libunwind.a` (the +latter is only necessary for 32-bit ARM) to ensure that unwind symbols are not re-exported from shared libraries. Even with the above precautions, it is still possible for an improperly built diff --git a/docs/changelogs/Changelog-r21.md b/docs/changelogs/Changelog-r21.md index e42e210f6..e3ce22396 100644 --- a/docs/changelogs/Changelog-r21.md +++ b/docs/changelogs/Changelog-r21.md @@ -93,10 +93,15 @@ For Android Studio issues, follow the docs on the [Android Studio site]. of limited use today. Also note that the above code will behave correctly even on pre-r21 because calling an undefined function in make returns the empty string, so the else case will be taken. + * [Issue 1092]: Fixed hiding of unwinder symbols in outputs of ndk-build and + CMake. Maintainers of third-party build systems should apply similar fixes + when using NDK r19 and above to guard against possible future compatibility + issues. [FORTIFY in Android]: https://android-developers.googleblog.com/2017/04/fortify-in-android.html [Issue 1004]: https://github.com/android-ndk/ndk/issues/1004 [Issue 1028]: https://github.com/android/ndk/issues/1028 +[Issue 1092]: https://github.com/android/ndk/issues/1092 [Issue 744]: https://github.com/android/ndk/issues/744 [Issue 855]: https://github.com/android-ndk/ndk/issues/855 [Issue 859]: https://github.com/android-ndk/ndk/issues/859 diff --git a/ndk/hosts.py b/ndk/hosts.py index c1c6ca248..a2759f2d3 100644 --- a/ndk/hosts.py +++ b/ndk/hosts.py @@ -17,7 +17,6 @@ from __future__ import absolute_import import enum -import os import sys @@ -51,11 +50,7 @@ def get_host_tag(ndk_path: str) -> str: elif sys.platform == 'darwin': return 'darwin-x86_64' elif sys.platform == 'win32': - host_tag = 'windows-x86_64' - test_path = os.path.join(ndk_path, 'prebuilt', host_tag) - if not os.path.exists(test_path): - host_tag = 'windows' - return host_tag + return 'windows-x86_64' raise ValueError('Unknown host: {}'.format(sys.platform)) diff --git a/ndk/test/types.py b/ndk/test/types.py index 02ccb2f62..92c4a4fb6 100644 --- a/ndk/test/types.py +++ b/ndk/test/types.py @@ -193,6 +193,7 @@ def run(self, obj_dir: str, _dist_dir: str, _prep_build_dir(self.test_dir, build_dir) with ndk.ext.os.cd(build_dir): module = imp.load_source('test', 'test.py') + assert self.platform is not None success, failure_message = module.run_test( # type: ignore self.ndk_path, self.abi, self.platform, self.config.linker, self.ndk_build_flags) if success: diff --git a/tests/build/unwinder_hidden/__init__.py b/tests/build/unwinder_hidden/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/tests/build/unwinder_hidden/project/jni/Android.mk b/tests/build/unwinder_hidden/project/jni/Android.mk new file mode 100644 index 000000000..714fb84ca --- /dev/null +++ b/tests/build/unwinder_hidden/project/jni/Android.mk @@ -0,0 +1,7 @@ +LOCAL_PATH := $(call my-dir) + +include $(CLEAR_VARS) +LOCAL_MODULE := foo +LOCAL_SRC_FILES := foo.cpp +LOCAL_CPPFLAGS := -fexceptions -frtti +include $(BUILD_SHARED_LIBRARY) diff --git a/tests/build/unwinder_hidden/project/jni/Application.mk b/tests/build/unwinder_hidden/project/jni/Application.mk new file mode 100644 index 000000000..ce095350e --- /dev/null +++ b/tests/build/unwinder_hidden/project/jni/Application.mk @@ -0,0 +1 @@ +APP_STL := c++_static diff --git a/tests/build/unwinder_hidden/project/jni/foo.cpp b/tests/build/unwinder_hidden/project/jni/foo.cpp new file mode 100644 index 000000000..607a21cd2 --- /dev/null +++ b/tests/build/unwinder_hidden/project/jni/foo.cpp @@ -0,0 +1,3 @@ +void foo() { + throw 42; +} diff --git a/tests/build/unwinder_hidden/test.py b/tests/build/unwinder_hidden/test.py new file mode 100644 index 000000000..b0d75b60c --- /dev/null +++ b/tests/build/unwinder_hidden/test.py @@ -0,0 +1,82 @@ +# +# Copyright (C) 2018 The Android Open Source Project +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# +"""Check for correct link order from ndk-build. +""" +from pathlib import Path +import re +import subprocess +from typing import Iterator, List, Optional, Tuple + +from ndk.abis import Abi +import ndk.hosts +from ndk.toolchains import LinkerOption + + +def find_public_unwind_symbols(output: str) -> Iterator[str]: + """Returns an iterator over readelf lines with unwind symbols in them.""" + # 274: 00000000000223d8 8 FUNC GLOBAL DEFAULT 11 _Unwind_GetIP + # Group 1: Visibility + # Group 2: Name + readelf_regex = re.compile(r'^.*?(\S+)\s+\d+\s+(\S+)$') + for line in output.splitlines(): + match = readelf_regex.match(line) + if match is None: + continue + visibility, name = match.groups() + if name.startswith('_Unwind') and visibility == 'DEFAULT': + yield name + + +def readelf(ndk_path: Path, host: ndk.hosts.Host, library: Path, + *args: str) -> str: + """Runs readelf, returning the output.""" + readelf_path = (ndk_path / 'toolchains/llvm/prebuilt' / + ndk.hosts.get_host_tag(str(ndk_path)) / 'bin/llvm-readelf') + if host.is_windows: + readelf_path = readelf_path.with_suffix('.exe') + + return subprocess.run( + [str(readelf_path), *args, str(library)], + check=True, + encoding='utf-8', + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT).stdout + + +def run_test(ndk_path: str, abi: Abi, platform: Optional[int], + linker: LinkerOption, build_flags: List[str]) -> Tuple[bool, str]: + """Check that unwinder symbols are hidden in outputs.""" + ndk_build = Path(ndk_path) / 'ndk-build' + host = ndk.hosts.get_default_host() + if host.is_windows: + ndk_build = ndk_build.with_suffix('.cmd') + project_path = Path('project') + ndk_args = build_flags + [ + f'APP_ABI={abi}', + f'APP_LD={linker.value}', + f'APP_PLATFORM=android-{platform}', + ] + subprocess.run( + [str(ndk_build), '-C', str(project_path)] + ndk_args, + check=True, + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT) + + library = project_path / 'libs' / str(abi) / 'libfoo.so' + readelf_output = readelf(Path(ndk_path), host, library, '-sW') + for symbol in find_public_unwind_symbols(readelf_output): + return False, f'Found public unwind symbol: {symbol}' + return True, ''