LibC+LibELF: Do not override existing weak symbols during magic lookup

Previously, the actual behavior of magic lookup and one described in its
commit description have not matched. Instead of being weak definitions
in a library that is always in the end of load order, the definitions
were normal ones and thus were able to override other weak definitions
in LibC. While this was consistent with how DynamicLoader resolves
ambiguity between normal and weak relocations, this is not the behavior
POSIX mandates -- we should always choose first available definition wrt
load order. To fix this problem, the patch makes sure we don't define
any of magic symbols in LibC.

In addition to this, it makes all provided magic symbols functions
(instead of objects), what renders MagicWeakSymbol class unnecessary.

Userland/DynamicLoader/main.cpp

@@ -15,16 +15,10 @@
 #include <unistd.h>
 
 char* __static_environ[] = { nullptr }; // We don't get the environment without some libc workarounds..
+char** environ = __static_environ;
 
-static void init_libc()
-{
-    environ = __static_environ;
-    __environ_is_malloced = false;
-    __stdio_is_initialized = false;
-    // Initialise the copy of libc included statically in Loader.so,
-    // initialisation of the dynamic libc.so is done by the DynamicLinker
-    __libc_init();
-}
+// FIXME: Kernel should give us a random value for __stack_chk_guard.
+uintptr_t __stack_chk_guard = 0xe0e6'066b'b7ea'c300;
 
 static void perform_self_relocations(auxv_t* auxvp)
 {
@@ -119,7 +113,10 @@ void _entry(int argc, char** argv, char** envp)
 
     auxv_t* auxvp = (auxv_t*)++env;
     perform_self_relocations(auxvp);
-    init_libc();
+
+    // Initialize the copy of libc included statically in Loader.so,
+    // initialization of the dynamic libc.so is done by the DynamicLinker
+    __libc_init(0);
 
     int main_program_fd = -1;
     ByteString main_program_path;

Userland/Libraries/LibC/bits/dlfcn_integration.h

@@ -26,15 +26,3 @@ struct DlErrorMessage {
 
 struct __Dl_info;
 typedef struct __Dl_info Dl_info;
-
-typedef Result<void, DlErrorMessage> (*DlCloseFunction)(void*);
-typedef Result<void*, DlErrorMessage> (*DlOpenFunction)(char const*, int);
-typedef Result<void*, DlErrorMessage> (*DlSymFunction)(void*, char const*);
-typedef Result<void, DlErrorMessage> (*DlAddrFunction)(void const*, Dl_info*);
-
-extern "C" {
-extern DlCloseFunction __dlclose;
-extern DlOpenFunction __dlopen;
-extern DlSymFunction __dlsym;
-extern DlAddrFunction __dladdr;
-}

Userland/Libraries/LibC/dlfcn.cpp

@@ -5,16 +5,16 @@
  */
 
 #include <AK/ByteString.h>
+#include <AK/Result.h>
 #include <AK/Types.h>
 #include <bits/dlfcn_integration.h>
 #include <dlfcn.h>
 #include <string.h>
 
-// These are filled in by the dynamic loader.
-[[gnu::weak]] DlCloseFunction __dlclose;
-[[gnu::weak]] DlOpenFunction __dlopen;
-[[gnu::weak]] DlSymFunction __dlsym;
-[[gnu::weak]] DlAddrFunction __dladdr;
+[[gnu::weak]] Result<void, DlErrorMessage> __dlclose(void*) asm("__dlclose");
+[[gnu::weak]] Result<void*, DlErrorMessage> __dlopen(char const*, int) asm("__dlopen");
+[[gnu::weak]] Result<void*, DlErrorMessage> __dlsym(void*, char const*) asm("__dlsym");
+[[gnu::weak]] Result<void, DlErrorMessage> __dladdr(void const*, Dl_info*) asm("__dladdr");
 
 // FIXME: use thread_local and a String once TLS works
 #ifdef NO_TLS

Userland/Libraries/LibC/libcinit.cpp

@@ -10,6 +10,8 @@
 #include <sys/internals.h>
 #include <unistd.h>
 
+[[gnu::weak]] char** __environ_value() asm("__environ_value");
+
 extern "C" {
 
 #ifdef NO_TLS
@@ -17,10 +19,14 @@ int errno_storage;
 #else
 __thread int errno_storage;
 #endif
-[[gnu::weak]] char** environ;
 bool __environ_is_malloced = false;
-bool __stdio_is_initialized;
-void* __auxiliary_vector;
+bool __stdio_is_initialized = false;
+void* __auxiliary_vector = reinterpret_cast<void*>(explode_byte(0xe1));
+
+#ifndef _DYNAMIC_LOADER
+char** environ = reinterpret_cast<char**>(explode_byte(0xe2));
+uintptr_t __stack_chk_guard;
+#endif
 
 static void __auxiliary_vector_init();
 
@@ -29,8 +35,12 @@ int* __errno_location()
     return &errno_storage;
 }
 
-void __libc_init()
+void __libc_init([[maybe_unused]] uintptr_t cookie)
 {
+#ifndef _DYNAMIC_LOADER
+    __stack_chk_guard = cookie;
+    environ = __environ_value();
+#endif
     __auxiliary_vector_init();
     __malloc_init();
     __stdio_init();

Userland/Libraries/LibC/link.cpp

@@ -7,15 +7,10 @@
 #include <assert.h>
 #include <link.h>
 
-extern "C" {
-
 using DlIteratePhdrCallbackFunction = int (*)(struct dl_phdr_info*, size_t, void*);
-using DlIteratePhdrFunction = int (*)(DlIteratePhdrCallbackFunction, void*);
+[[gnu::weak]] extern int __dl_iterate_phdr(DlIteratePhdrCallbackFunction, void*) asm("__dl_iterate_phdr");
 
-[[gnu::weak]] DlIteratePhdrFunction __dl_iterate_phdr;
-
-int dl_iterate_phdr(int (*callback)(struct dl_phdr_info* info, size_t size, void* data), void* data)
+extern "C" int dl_iterate_phdr(int (*callback)(struct dl_phdr_info* info, size_t size, void* data), void* data)
 {
     return __dl_iterate_phdr(callback, data);
 }
-}

Userland/Libraries/LibC/pthread.cpp

@@ -53,10 +53,10 @@ static thread_local SinglyLinkedList<CleanupHandler> cleanup_handlers;
 
 static __thread bool pending_cancellation = false;
 
-extern "C" {
+[[gnu::weak]] extern ErrorOr<FlatPtr> __create_new_tls_region() asm("__create_new_tls_region");
+[[gnu::weak]] extern ErrorOr<void> __free_tls_region(FlatPtr thread_pointer) asm("__free_tls_region");
 
-[[gnu::weak]] ErrorOr<FlatPtr> (*__create_new_tls_region)();
-[[gnu::weak]] ErrorOr<void> (*__free_tls_region)(FlatPtr thread_pointer);
+extern "C" {
 
 [[noreturn]] static void exit_thread(void* code, void* stack_location, size_t stack_size)
 {

Userland/Libraries/LibC/ssp.cpp

@@ -17,10 +17,6 @@
 
 extern "C" {
 
-extern uintptr_t __stack_chk_guard;
-// Populated by DynamicLinker in shared executables.
-[[gnu::weak]] uintptr_t __stack_chk_guard = (uintptr_t)0xc6c7c8c9;
-
 __attribute__((noreturn)) void __stack_chk_fail()
 {
     dbgln("Error: USERSPACE({}) Stack protector failure, stack smashing detected!", getpid());

Userland/Libraries/LibC/stdlib.cpp

@@ -343,9 +343,9 @@ static T c_str_to_floating_point(char const* str, char** endptr)
     return 0;
 }
 
-extern "C" {
+[[gnu::weak]] extern void __call_fini_functions() asm("__call_fini_functions");
 
-[[gnu::weak]] void (*__call_fini_functions)();
+extern "C" {
 
 void exit(int status)
 {

Userland/Libraries/LibC/sys/internals.h

@@ -13,7 +13,7 @@ __BEGIN_DECLS
 
 typedef void (*AtExitFunction)(void*);
 
-extern void __libc_init(void);
+extern void __libc_init(uintptr_t);
 extern void __malloc_init(void);
 extern void __stdio_init(void);
 extern void __begin_atexit_locking(void);

Userland/Libraries/LibELF/DynamicLinker.cpp

@@ -31,6 +31,7 @@
 #include <link.h>
 #include <pthread.h>
 #include <string.h>
+#include <sys/internals.h>
 #include <sys/mman.h>
 #include <sys/types.h>
 #include <syscall.h>
@@ -73,32 +74,7 @@ static StringView s_ld_library_path;
 static StringView s_main_program_pledge_promises;
 static ByteString s_loader_pledge_promises;
 
-class MagicWeakSymbol : public RefCounted<MagicWeakSymbol> {
-    AK_MAKE_NONCOPYABLE(MagicWeakSymbol);
-    AK_MAKE_NONMOVABLE(MagicWeakSymbol);
-
-public:
-    template<typename T>
-    MagicWeakSymbol(unsigned int type, T value)
-    {
-        m_storage = reinterpret_cast<uintptr_t>(value);
-        m_lookup_result.size = 8;
-        m_lookup_result.type = type;
-        m_lookup_result.address = VirtualAddress { &m_storage };
-        m_lookup_result.bind = STB_GLOBAL;
-    }
-
-    auto lookup_result() const
-    {
-        return m_lookup_result;
-    }
-
-private:
-    DynamicObject::SymbolLookupResult m_lookup_result;
-    uintptr_t m_storage;
-};
-
-static HashMap<StringView, NonnullRefPtr<MagicWeakSymbol>> s_magic_weak_symbols;
+static HashMap<StringView, DynamicObject::SymbolLookupResult> s_magic_functions;
 
 Optional<DynamicObject::SymbolLookupResult> DynamicLinker::lookup_global_symbol(StringView name)
 {
@@ -117,8 +93,10 @@ Optional<DynamicObject::SymbolLookupResult> DynamicLinker::lookup_global_symbol(
         // We don't want to allow local symbols to be pulled in to other modules
     }
 
-    if (auto magic_lookup = s_magic_weak_symbols.get(name); magic_lookup.has_value())
-        weak_result = (*magic_lookup)->lookup_result();
+    if (!weak_result.has_value()) {
+        if (auto magic_lookup = s_magic_functions.get(name); magic_lookup.has_value())
+            weak_result = *magic_lookup;
+    }
 
     return weak_result;
 }
@@ -344,10 +322,15 @@ static int __dl_iterate_phdr(DlIteratePhdrCallbackFunction callback, void* data)
 
 static void initialize_libc(DynamicObject& libc)
 {
+    uintptr_t stack_guard = get_random<uintptr_t>();
+    // We include an additional hardening: zero the first byte of the stack guard to avoid leaking
+    // or overwriting the stack guard with C-style string functions.
+    stack_guard &= ~0xffULL;
+
     auto res = libc.lookup_symbol("__libc_init"sv);
     VERIFY(res.has_value());
-    typedef void libc_init_func();
-    ((libc_init_func*)res.value().address.as_ptr())();
+    using libc_init_func = decltype(__libc_init);
+    ((libc_init_func*)res.value().address.as_ptr())(stack_guard);
 }
 
 template<typename Callback>
@@ -661,6 +644,11 @@ static void __call_fini_functions()
     }
 }
 
+static char** __environ_value()
+{
+    return s_envp;
+}
+
 static void read_environment_variables()
 {
     for (char** env = s_envp; *env; ++env) {
@@ -692,24 +680,24 @@ void ELF::DynamicLinker::linker_main(ByteString&& main_program_path, int main_pr
 
     s_envp = envp;
 
-    uintptr_t stack_guard = get_random<uintptr_t>();
-
-#ifdef AK_ARCH_64_BIT
-    // For 64-bit platforms we include an additional hardening: zero the first byte of the stack guard to avoid
-    // leaking or overwriting the stack guard with C-style string functions.
-    stack_guard &= ~0xffULL;
-#endif
-
-    s_magic_weak_symbols.set("environ"sv, make_ref_counted<MagicWeakSymbol>(STT_OBJECT, s_envp));
-    s_magic_weak_symbols.set("__stack_chk_guard"sv, make_ref_counted<MagicWeakSymbol>(STT_OBJECT, stack_guard));
-    s_magic_weak_symbols.set("__call_fini_functions"sv, make_ref_counted<MagicWeakSymbol>(STT_FUNC, __call_fini_functions));
-    s_magic_weak_symbols.set("__create_new_tls_region"sv, make_ref_counted<MagicWeakSymbol>(STT_FUNC, __create_new_tls_region));
-    s_magic_weak_symbols.set("__free_tls_region"sv, make_ref_counted<MagicWeakSymbol>(STT_FUNC, __free_tls_region));
-    s_magic_weak_symbols.set("__dl_iterate_phdr"sv, make_ref_counted<MagicWeakSymbol>(STT_FUNC, __dl_iterate_phdr));
-    s_magic_weak_symbols.set("__dlclose"sv, make_ref_counted<MagicWeakSymbol>(STT_FUNC, __dlclose));
-    s_magic_weak_symbols.set("__dlopen"sv, make_ref_counted<MagicWeakSymbol>(STT_FUNC, __dlopen));
-    s_magic_weak_symbols.set("__dlsym"sv, make_ref_counted<MagicWeakSymbol>(STT_FUNC, __dlsym));
-    s_magic_weak_symbols.set("__dladdr"sv, make_ref_counted<MagicWeakSymbol>(STT_FUNC, __dladdr));
+    auto define_magic_function = [&](StringView name, auto function) {
+        s_magic_functions.set(name,
+            DynamicObject::SymbolLookupResult {
+                .size = 8,
+                .address = VirtualAddress { reinterpret_cast<void*>(function) },
+                .bind = STB_GLOBAL,
+                .type = STT_FUNC,
+            });
+    };
+    define_magic_function("__call_fini_functions"sv, __call_fini_functions);
+    define_magic_function("__create_new_tls_region"sv, __create_new_tls_region);
+    define_magic_function("__dl_iterate_phdr"sv, __dl_iterate_phdr);
+    define_magic_function("__dladdr"sv, __dladdr);
+    define_magic_function("__dlclose"sv, __dlclose);
+    define_magic_function("__dlopen"sv, __dlopen);
+    define_magic_function("__dlsym"sv, __dlsym);
+    define_magic_function("__environ_value"sv, __environ_value);
+    define_magic_function("__free_tls_region"sv, __free_tls_region);
 
     char* raw_current_directory = getcwd(nullptr, 0);
     s_cwd = raw_current_directory;