From 4b091a7cc27ef07ba5c0d46a02454ecbb16f6575 Mon Sep 17 00:00:00 2001 From: Rodrigo Tobar Date: Wed, 29 Sep 2021 21:36:02 +0800 Subject: [PATCH] LibELF: Fix dynamic linking of dlopen()-ed libs Consider the situation where two shared libraries libA and libB, both depending (as in having a NEEDED dtag) on libC. libA is first dlopen()-ed, which produces libC to be mapped and linked. When libB is dlopen()-ed the DynamicLinker would re-map and re-link libC though, causing any previous references to its old location to be invalid. And if libA's PLT has been patched to point to libC's symbols, then any further invocations to libA will cause the code to jump to a virtual address that isn't mapped anymore, therefore causing a crash. This situation was reported in #10014, although the setup was more convolved in the ticket. This commit fixes the issue by distinguishing between a main program loading being performed by Loader.so, and a dlopen() call. The main difference between these two cases is that in the former the s_globals_objects maps is always empty, while in the latter it might already contain dependencies for the library being dlopen()-ed. Hence, when collecting dependencies to map and link, dlopen() should skip those that are present in the global map to avoid the issue described above. With this patch the original issue seen in #10014 is gone, with all python3 modules (so far) loading correctly. A unit test reproducing a simplified issue is also included in this commit. The unit test includes the building of two dynamic libraries A and B with both depending on libline.so (and B also depending on A); the test then dlopen()s libA, invokes one its function, then does the same with libB. --- Tests/LibELF/CMakeLists.txt | 16 ++++++++++++- Tests/LibELF/Dynlib.cpp | 15 +++++++++++++ Tests/LibELF/TestDlOpen.cpp | 25 +++++++++++++++++++++ Userland/Libraries/LibELF/DynamicLinker.cpp | 19 +++++++++------- 4 files changed, 66 insertions(+), 9 deletions(-) create mode 100644 Tests/LibELF/Dynlib.cpp create mode 100644 Tests/LibELF/TestDlOpen.cpp diff --git a/Tests/LibELF/CMakeLists.txt b/Tests/LibELF/CMakeLists.txt index ab5a9f200a..5e5303be1d 100644 --- a/Tests/LibELF/CMakeLists.txt +++ b/Tests/LibELF/CMakeLists.txt @@ -1,7 +1,21 @@ +macro(add_dlopen_lib NAME FUNCTION) + add_library(${NAME} SHARED Dynlib.cpp) + target_compile_definitions(${NAME} PRIVATE -DFUNCTION=${FUNCTION}) + # LibLine is not special, just an "external" dependency + target_link_libraries(${NAME} LibLine) + # Avoid execution by the test runner + install(TARGETS ${NAME} + DESTINATION usr/Tests/LibELF + PERMISSIONS OWNER_READ GROUP_READ WORLD_READ) +endmacro() +add_dlopen_lib(DynlibA dynliba_function) +add_dlopen_lib(DynlibB dynlibb_function) + set(TEST_SOURCES test-elf.cpp + TestDlOpen.cpp ) foreach(source IN LISTS TEST_SOURCES) - serenity_test("${source}" LibELF) + serenity_test("${source}" LibELF LIBS LibDl) endforeach() diff --git a/Tests/LibELF/Dynlib.cpp b/Tests/LibELF/Dynlib.cpp new file mode 100644 index 0000000000..4187378fd1 --- /dev/null +++ b/Tests/LibELF/Dynlib.cpp @@ -0,0 +1,15 @@ +/* + * Copyright (c) 2021, Rodrigo Tobar + * + * SPDX-License-Identifier: BSD-2-Clause + */ + +#include + +extern "C" { +int FUNCTION(); +int FUNCTION() +{ + return (int)Line::Span(0, 0).beginning(); +} +} diff --git a/Tests/LibELF/TestDlOpen.cpp b/Tests/LibELF/TestDlOpen.cpp new file mode 100644 index 0000000000..92f69a051e --- /dev/null +++ b/Tests/LibELF/TestDlOpen.cpp @@ -0,0 +1,25 @@ +/* + * Copyright (c) 2021, Rodrigo Tobar + * + * SPDX-License-Identifier: BSD-2-Clause + */ + +#include +#include + +TEST_CASE(test_dlopen) +{ + auto liba = dlopen("/usr/Tests/LibELF/libDynlibA.so", 0); + EXPECT_NE(liba, nullptr); + auto libb = dlopen("/usr/Tests/LibELF/libDynlibB.so", 0); + EXPECT_NE(libb, nullptr); + + typedef int (*dynlib_func_t)(); + dynlib_func_t func_a = (dynlib_func_t)dlsym(liba, "dynliba_function"); + EXPECT_NE(func_a, nullptr); + EXPECT_EQ(0, func_a()); + + dynlib_func_t func_b = (dynlib_func_t)dlsym(libb, "dynlibb_function"); + EXPECT_NE(func_b, nullptr); + EXPECT_EQ(0, func_b()); +} diff --git a/Userland/Libraries/LibELF/DynamicLinker.cpp b/Userland/Libraries/LibELF/DynamicLinker.cpp index 4e017bd744..eb05122ad6 100644 --- a/Userland/Libraries/LibELF/DynamicLinker.cpp +++ b/Userland/Libraries/LibELF/DynamicLinker.cpp @@ -257,38 +257,41 @@ static void initialize_libc(DynamicObject& libc) } template -static void for_each_unfinished_dependency_of(const String& name, HashTable& seen_names, Callback callback) +static void for_each_unfinished_dependency_of(const String& name, HashTable& seen_names, bool first, bool skip_global_objects, Callback callback) { if (!s_loaders.contains(name)) return; + if (!first && skip_global_objects && s_global_objects.contains(name)) + return; + if (seen_names.contains(name)) return; seen_names.set(name); for (const auto& needed_name : get_dependencies(name)) - for_each_unfinished_dependency_of(get_library_name(needed_name), seen_names, callback); + for_each_unfinished_dependency_of(get_library_name(needed_name), seen_names, false, skip_global_objects, callback); callback(*s_loaders.get(name).value()); } -static NonnullRefPtrVector collect_loaders_for_library(const String& name) +static NonnullRefPtrVector collect_loaders_for_library(const String& name, bool skip_global_objects) { HashTable seen_names; NonnullRefPtrVector loaders; - for_each_unfinished_dependency_of(name, seen_names, [&](auto& loader) { + for_each_unfinished_dependency_of(name, seen_names, true, skip_global_objects, [&](auto& loader) { loaders.append(loader); }); return loaders; } -static Result, DlErrorMessage> load_main_library(const String& name, int flags) +static Result, DlErrorMessage> load_main_library(const String& name, int flags, bool skip_global_objects) { auto main_library_loader = *s_loaders.get(name); auto main_library_object = main_library_loader->map(); s_global_objects.set(name, *main_library_object); - auto loaders = collect_loaders_for_library(name); + auto loaders = collect_loaders_for_library(name, skip_global_objects); for (auto& loader : loaders) { auto dynamic_object = loader.map(); @@ -411,7 +414,7 @@ static Result __dlopen(const char* filename, int flags) return result2.error(); } - auto result = load_main_library(library_name, flags); + auto result = load_main_library(library_name, flags, true); if (result.is_error()) return result.error(); @@ -535,7 +538,7 @@ void ELF::DynamicLinker::linker_main(String&& main_program_name, int main_progra auto entry_point_function = [&main_program_name] { auto library_name = get_library_name(main_program_name); - auto result = load_main_library(library_name, RTLD_GLOBAL | RTLD_LAZY); + auto result = load_main_library(library_name, RTLD_GLOBAL | RTLD_LAZY, false); if (result.is_error()) { warnln("{}", result.error().text); _exit(1);