From 6f4e0aee14c613cb35bdb7c4a002657850299731 Mon Sep 17 00:00:00 2001 From: Enrique Saurez Date: Thu, 4 Jun 2026 14:33:10 -0700 Subject: [PATCH] [syscall] B: Fix diamond DT_NEEDED handling The dynamic loader walks `DT_NEEDED` recursively, but two bugs broke diamond dependency graphs of the shape: libdiamond.so -> libleft.so -> libbase.so -> libright.so -> libbase.so both of which only surface once `DT_NEEDED` chains are nontrivial (the existing dlfcn-needed-c test stays a single-edge linear graph, so it never exercised either path). Symptoms before this commit: 1. `dlopen(libdiamond.so)` HANGS inside the recursive load. 2. After the hang fix is in place, `dlclose(libdiamond.so)` panics with `assertion left == right failed: expected to remove exactly one dynamic library file`. Fix 1 -- dlopen deadlock on ancestor lock ----------------------------------------- `load_all_dependencies_recursive` holds the parent library's mutex while iterating the registry's already-loaded entries. Each entry gets `.lock()`-ed to compare names. The previous code skipped only the immediate `new_dlhandle` self; it did not skip ancestors held by outer recursive frames. Loading libright (depth 1) and then recursing into libright's deps (depth 2) tried to lock libdiamond in the inner scan, which deadlocked: libdiamond was held by the outer frame. Fix: thread a `BTreeSet` of ancestor handles down the recursion and skip every entry whose handle is in that set. The ancestor is by definition not a candidate for "already loaded by a sibling" so the skip is functionally safe. Fix 2 -- dlopen now also handles transitive load races ------------------------------------------------------ Once the deadlock is gone, a second issue surfaces: the pre-loop `retain` only sees libraries loaded *before* the current frame started. In a diamond, libbase is loaded by libleft's recursion *during* libdiamond's frame, after libdiamond's retain has already run. Without a re-check, the outer loop would then open libbase a second time (creating two distinct in-memory copies with their own globals) or trip the `unreachable!()` when the VFS reuses the file descriptor. Fix: before each `DynamicLibrary::open` call, re-scan the registry for a matching name and bind the existing copy if found. Fix 3 -- dlclose strong_count assertion for diamond --------------------------------------------------- The BFS unload loop extracts each library from the registry when `Arc::strong_count == 2` (registry + current pop). For a diamond, libbase has three Arc references at the time it is first popped (registry + libleft.dependencies + libright.dependencies), so `extract_if` correctly returns 0 entries -- but the assertion demanded exactly 1, panicking the kernel. A later iteration of the loop pops libright/libleft, drops their dependencies (releasing the extra libbase ref), pushes libbase again, and the second pop succeeds at the now-correct refcount. Fix: replace the strict `assert_eq!(== 1)` with an early `continue` when `extract_if` returns zero entries, keeping the (now relaxed) "at most one" assertion to catch any future runtime invariant violation. Validation ---------- A companion `dlfcn-diamond-c` test suite proves both fixes end-to-end on a standalone Nanvix VM: === dlfcn diamond DT_NEEDED tests === PASS: dlopen(libright.so) [depth-2 chain] PASS: dlopen(libdiamond.so) PASS: re-dlopen(libdiamond.so) returns same handle 3 passed, 0 failed All previously passing dlfcn tests (dlfcn-c, dlfcn-init-runpath-c, dlfcn-pie-c) still pass; full posix-tests suite passes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/libs/syscall/src/dlfcn/syscall/dlclose.rs | 20 ++++- src/libs/syscall/src/dlfcn/syscall/dlopen.rs | 88 +++++++++++++++++-- 2 files changed, 102 insertions(+), 6 deletions(-) diff --git a/src/libs/syscall/src/dlfcn/syscall/dlclose.rs b/src/libs/syscall/src/dlfcn/syscall/dlclose.rs index 864efe6b2c..59c2540583 100644 --- a/src/libs/syscall/src/dlfcn/syscall/dlclose.rs +++ b/src/libs/syscall/src/dlfcn/syscall/dlclose.rs @@ -117,10 +117,28 @@ pub fn dlclose(handle: &DlHandle) -> Result<(), Error> { }) .collect(); + // `extract_if` may legitimately return zero entries when this + // library still has live dependents elsewhere in the dependency + // tree. This is the diamond `DT_NEEDED` case -- + // + // libdiamond.so -> libleft.so -> libbase.so + // -> libright.so -> libbase.so + // + // While unloading libright, BFS pushes libbase onto the work + // list. By the time we pop libbase, libleft has not yet unloaded + // and still holds an Arc to libbase, so `strong_count` is 3 + // (registry + libleft.dependencies + our pop). We skip libbase + // for now; a later iteration will pop libleft, drop its + // dependencies (releasing the extra libbase reference), push + // libbase a second time, and the subsequent pop will succeed. + if dep_dlfile.is_empty() { + continue; + } assert_eq!( dep_dlfile.len(), 1, - "dlclose(): expected to remove exactly one dynamic library file" + "dlclose(): expected to remove exactly one dynamic library file (the empty case is \ + handled above)" ); // Collect all dependencies of the dynamic library file. diff --git a/src/libs/syscall/src/dlfcn/syscall/dlopen.rs b/src/libs/syscall/src/dlfcn/syscall/dlopen.rs index 0281624206..8cdaa1435e 100644 --- a/src/libs/syscall/src/dlfcn/syscall/dlopen.rs +++ b/src/libs/syscall/src/dlfcn/syscall/dlopen.rs @@ -162,6 +162,7 @@ fn load_all_dependencies( dlfiles: &mut MutexGuard<'_, BTreeMap>>>, new_dlhandle: &DlHandle, new_dlfile: &mut MutexGuard<'_, DynamicLibrary>, + ancestors: &mut BTreeSet, ) -> Result<(), Error> { // Snapshot the loader's `DT_RUNPATH` entries so they are visible to // `resolve_library_path` while probing every dependency below. The @@ -177,13 +178,44 @@ fn load_all_dependencies( .collect(); // Bind to already loaded dependencies and remove them from the list. + // + // The closure below calls `dlfile.lock()` on every other entry in the + // registry while looking for a matching name. Those locks must skip + // BOTH the current library (`new_dlhandle`) AND every ancestor still + // held by an outer recursive frame — otherwise any non-trivial + // recursive load (e.g. `libA → libB`, and especially diamond graphs + // like the one below) deadlocks: + // + // dlopen(libdiamond.so) // holds libdiamond lock + // -> recurse into libright.so // also holds libright lock + // -> retain() iterates dlfiles // tries to lock libdiamond + // ^ blocks forever, libdiamond is still held by the + // outer frame. + // + // The lookup is purely advisory (we want to know if a sibling already + // bound a dependency with the same name), so skipping ancestors is + // safe in the common acyclic case: an ancestor by definition cannot + // have been loaded by a prior iteration of the same frame's + // dependency list. + // + // Known limitation: a legitimate `DT_NEEDED` *cycle* + // (`libA → libB → libA`) is not detected here. With the deadlock + // fixed, an inner frame asking for an ancestor's name will fall + // through to opening a fresh copy of that library on a new handle, + // producing unbounded recursion / duplicate library instances. + // Cycles in `DT_NEEDED` are pathological and not produced by any + // sane toolchain; detecting and rejecting them cleanly is left as + // a follow-up (would require carrying the ancestor *names* through + // the recursion to compare without locking, since locking the + // ancestor to read its name would reintroduce the deadlock). dependencies.retain(|dependency| { // Resolve bare name so we can match against loaded libraries // that were opened with a full path. let resolved_dep: String = super::resolve_library_path(dependency, Some(&runpaths)); for (dlhandle, dlfile) in dlfiles.iter() { - // Check if need to skip the dynamic library itself. - if dlhandle == new_dlhandle { + // Skip the dynamic library itself and any ancestor held by + // an outer frame's lock — locking them would deadlock. + if dlhandle == new_dlhandle || ancestors.contains(dlhandle) { continue; } @@ -218,6 +250,45 @@ fn load_all_dependencies( // Resolve bare library names to full paths using search directories. let resolved_dep: String = super::resolve_library_path(&dependency, Some(&runpaths)); + // Re-check the registry before opening: a prior iteration of + // this same loop may have already loaded this dependency + // transitively. This is the diamond case -- + // + // libdiamond.so -> libleft.so -> libbase.so + // -> libright.so -> libbase.so + // + // After processing libleft (and recursing into it, which loads + // libbase), libbase is now in the registry. When the outer loop + // returns to process libright, the closure-based `retain` above + // never ran for libbase because it was not yet loaded at the + // time libright's frame started; we must catch the diamond + // here instead. Without this check the loader would either + // open libbase a second time (producing two distinct + // in-memory copies) or trip the `unreachable!()` below when + // the VFS reuses the underlying file descriptor. + let already_loaded: Option>> = + dlfiles.iter().find_map(|(dlhandle, dlfile)| { + if dlhandle == new_dlhandle || ancestors.contains(dlhandle) { + return None; + } + let loaded_file: spin::MutexGuard<'_, DynamicLibrary> = dlfile.lock(); + let loaded_name: &str = loaded_file.name(); + if loaded_name == dependency.as_str() || loaded_name == resolved_dep { + Some(dlfile.clone()) + } else { + None + } + }); + if let Some(existing) = already_loaded { + ::syslog::debug!( + "load_all_dependencies_recursive(): dependency '{}' loaded transitively \ + during this dlopen call; binding to existing copy", + dependency + ); + new_dlfile.bind_dependency(dependency, existing)?; + continue; + } + // Open and pre-load the dynamic library file. let dep_dlfile: DynamicLibrary = DynamicLibrary::open(&resolved_dep)?; let handle: DlHandle = dep_dlfile.handle(); @@ -230,9 +301,15 @@ fn load_all_dependencies( new_dlfile.bind_dependency(dependency.clone(), dep_dlfile.clone())?; - // Load dependencies of the new dynamic library file. + // Load dependencies of the new dynamic library file. Mark the + // current library as an ancestor so the recursive frame does + // not try to lock our still-held mutex (would deadlock on + // diamond DT_NEEDED graphs). + ancestors.insert(*new_dlhandle); let mut dlfile: MutexGuard<'_, DynamicLibrary> = dep_dlfile.lock(); - load_all_dependencies_recursive(dlfiles, &handle, &mut dlfile)?; + let result = load_all_dependencies_recursive(dlfiles, &handle, &mut dlfile, ancestors); + ancestors.remove(new_dlhandle); + result?; } Ok(()) @@ -240,7 +317,8 @@ fn load_all_dependencies( let mut new_dlfile = new_dlfile.lock(); let new_dlhandle = new_dlfile.handle(); - load_all_dependencies_recursive(dlfiles, &new_dlhandle, &mut new_dlfile)?; + let mut ancestors: BTreeSet = BTreeSet::new(); + load_all_dependencies_recursive(dlfiles, &new_dlhandle, &mut new_dlfile, &mut ancestors)?; Ok(()) }