From bfdd5736e3ace6ba8f3bfb5df5e6797703ae073c Mon Sep 17 00:00:00 2001 From: Enrique Saurez Date: Thu, 4 Jun 2026 14:33:10 -0700 Subject: [PATCH 1/2] [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 ---------- Companion test suite at esaurez/posix-tests `dlfcn-diamond-c` 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 | 19 ++++- src/libs/syscall/src/dlfcn/syscall/dlopen.rs | 75 +++++++++++++++++-- 2 files changed, 88 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..f38f19d1dd 100644 --- a/src/libs/syscall/src/dlfcn/syscall/dlclose.rs +++ b/src/libs/syscall/src/dlfcn/syscall/dlclose.rs @@ -117,10 +117,27 @@ 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 at most one dynamic library file" ); // 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..04337e9e19 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,31 @@ 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 a diamond `DT_NEEDED` + // graph deadlocks, e.g.: + // + // 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: an ancestor by definition cannot have been loaded by a prior + // iteration of the same frame's dependency list. 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 +237,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 +288,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 +304,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(()) } From 78dd76dc7fc28578db66482ae668fbe4606fa1ec Mon Sep 17 00:00:00 2001 From: esaurez Date: Fri, 5 Jun 2026 17:16:10 -0700 Subject: [PATCH 2/2] [posix] E: Implement pthread_once and accept pthread_key_create destructors Two related libposix gaps that together block any OpenSSL 3 consumer: 1. pthread_once was a no-op stub (nanvix#513) that returned 0 without ever invoking init_routine. Every consumer that uses pthread_once for lazy initialization (OpenSSL, libstdc++ static init, glib, ...) silently never initialized. OpenSSL 3 routes every internal one-time init through this mechanism; ossl_init_base never runs, EVP_MD_fetch returns NULL, _hashlib.openssl_sha256() raises UnsupportedDigestmodError, SSL_CTX_new returns NULL with empty error queue. 2. pthread_key_create rejected non-null destructors with OperationNotSupported. OpenSSL's ossl_init_thread always passes a destructor (defensive practice) so the call fails. With pthread_once fixed, this is the very next step in OpenSSL's init chain. Mechanics: - pthread_once: 4-state machine encoded in init_executed (NEVER_RUN=0, DONE=1, IN_PROGRESS=2, WAIT=3) -- musl's encoding, future-compatible with the multi-threaded futex upgrade documented in the source comments. Uses volatile read/write + compiler_fence for memory ordering. Rust Drop guard provides POSIX cancellation semantics (reset to NEVER_RUN on panic/unwind). Sanity-checks is_initialized==1 to catch PTHREAD_ONCE_INIT misuse. Recursive same-control calls log a warning and return 0 (POSIX undefined behavior; our choice over deadlock). - pthread_key_create: drop the destructor-is-some hard error. Log a one-line warning and continue. Worst case is a per-thread resource leak at thread exit, which is benign in the single-threaded user-space model. Once thread-exit destructor support lands, the warning will get removed. - pthread_once_t accessor methods (init_executed_ptr, is_initialized, etc.) added to sysapi so libposix can read/write the state-machine word without making the struct fields pub. Validation: - OpenSSL 3.5.0 standalone C probe goes PROBE_FAIL -> PROBE_PASS (EVP_MD_fetch and OSSL_PROVIDER_load both succeed). - Python smoke tests on cpython.elf: OPENSSL_CHAIN_PASS (SSLContext, _hashlib.openssl_sha256 direct call -- the Phase-3 documented init-order bug is fixed) LXML_CHAIN_PASS (regression) CTYPES_CHAIN_PASS (regression) Closes nanvix/nanvix#513. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/libs/posix/src/pthread/mod.rs | 167 +++++++++++++++++- src/libs/sysapi/src/sys_types.rs | 55 ++++++ .../pthread/bindings/pthread_key_create.rs | 14 +- 3 files changed, 231 insertions(+), 5 deletions(-) diff --git a/src/libs/posix/src/pthread/mod.rs b/src/libs/posix/src/pthread/mod.rs index 8e84bc97cb..d0cd3b10aa 100644 --- a/src/libs/posix/src/pthread/mod.rs +++ b/src/libs/posix/src/pthread/mod.rs @@ -368,6 +368,33 @@ pub extern "C" fn pthread_equal(thread1: pthread_t, thread2: pthread_t) -> c_int // pthread_once() //================================================================================================== +/// State constants used internally by `pthread_once()` for the +/// `init_executed` field of `pthread_once_t`. +/// +/// # Description +/// +/// `init_executed` doubles as a state-machine word. The four +/// values directly mirror musl libc's encoding in +/// `src/thread/pthread_once.c`, which lets us upgrade to a +/// futex-based multi-threaded implementation without changing +/// the state semantics. +/// +/// In a future multi-threaded model: +/// +/// - `ONCE_NEVER_RUN` → CAS to `ONCE_IN_PROGRESS` and run `init`. +/// - `ONCE_IN_PROGRESS` → CAS to `ONCE_WAIT` and block on futex. +/// - `ONCE_WAIT` → continue waiting on the futex word. +/// - `ONCE_DONE` → fast-path return. +/// +/// In the current single-threaded model only `NEVER_RUN`, +/// `IN_PROGRESS`, and `DONE` are reachable; `ONCE_WAIT` is +/// reserved for the future upgrade. +const ONCE_NEVER_RUN: c_int = 0; +const ONCE_DONE: c_int = 1; +const ONCE_IN_PROGRESS: c_int = 2; +#[allow(dead_code)] +const ONCE_WAIT: c_int = 3; + /// /// # Description /// @@ -396,8 +423,144 @@ pub unsafe extern "C" fn pthread_once( once_control: *mut pthread_once_t, init_routine: Option, ) -> c_int { - // TODO: https://github.com/nanvix/nanvix/issues/513 - ::syslog::debug!("pthread_once(): not implemented"); + // Argument validation. + if once_control.is_null() { + ::syslog::warn!("pthread_once(): null once_control"); + return ErrorCode::InvalidArgument.get(); + } + let Some(init_fn) = init_routine else { + ::syslog::warn!("pthread_once(): null init_routine"); + return ErrorCode::InvalidArgument.get(); + }; + + // Sanity check: `is_initialized` must equal 1 after + // `PTHREAD_ONCE_INIT`. Any other value indicates a caller + // bug (e.g. a forgotten or corrupted static initializer). + // + // SAFETY: `once_control` was checked non-null above and is + // assumed to point to a valid `pthread_once_t` per POSIX + // contract. + let once: &mut pthread_once_t = unsafe { &mut *once_control }; + if once.is_initialized() != pthread_once_t::IS_INITIALIZED_VALUE { + ::syslog::warn!( + "pthread_once(): once_control not initialized with PTHREAD_ONCE_INIT" + ); + return ErrorCode::InvalidArgument.get(); + } + + // Fast path: already done. + // + // # Memory ordering + // + // POSIX requires that on return from `pthread_once`, the + // effects of `init_routine` are visible. On the fast path + // this is provided by the volatile read + acquire compiler + // fence: the read cannot be hoisted, and the fence prevents + // subsequent loads from being reordered before the + // observation of `ONCE_DONE`. + // + // On the current single-threaded target this fence is a + // no-op at runtime (no SMP), but it is required for + // correctness when the multi-threaded upgrade is performed. + let state_ptr: *mut c_int = once.init_executed_ptr(); + { + let current = unsafe { ::core::ptr::read_volatile(state_ptr) }; + if current == ONCE_DONE { + ::core::sync::atomic::compiler_fence(::core::sync::atomic::Ordering::Acquire); + return 0; + } + if current == ONCE_IN_PROGRESS { + // POSIX (APPLICATION USAGE) says recursive calls on + // the same `once_control` from inside `init_routine` + // are undefined and shall not return. In a + // single-threaded model the only way to reach this + // state is a recursive call: log and return success + // without re-running (the in-flight init will finish + // when control unwinds). + ::syslog::warn!( + "pthread_once(): recursive call on the same once_control \ + (in-progress) -- not re-running init" + ); + return 0; + } + } + + // Slow path: transition NEVER_RUN -> IN_PROGRESS, run init, + // transition IN_PROGRESS -> DONE. + // + // # Multi-threaded upgrade + // + // Replace the volatile write with a CAS: + // + // loop { + // match cas(state_ptr, NEVER_RUN, IN_PROGRESS) { + // Ok(_) => break, // we are the initializer + // Err(DONE) => return 0, // someone else finished + // Err(IN_PROGRESS) + // | Err(WAIT) => { + // cas(state_ptr, IN_PROGRESS, WAIT); + // futex_wait(state_ptr, WAIT); + // continue; + // } + // _ => unreachable!(), + // } + // } + // + // and replace the final `write_volatile(DONE)` with an + // atomic release-store followed by `futex_wake_all`. + unsafe { ::core::ptr::write_volatile(state_ptr, ONCE_IN_PROGRESS) }; + + // POSIX cancellation semantics: + // + // "If init_routine is a cancellation point and is canceled, + // the effect on once_control shall be as if pthread_once() + // was never called." + // + // Rust does not have `pthread_cleanup_push`, but a Drop guard + // gives equivalent behavior across both panic-unwind and + // future cancellation paths: if `init_fn` panics or unwinds, + // `OnceGuard::drop` resets the state to `NEVER_RUN` so the + // next call retries from scratch. + // + // In ST mode without unwinding there is no waiter to wake; + // in the MT upgrade `OnceGuard::drop` would also call + // `futex_wake_all` on the state pointer. + struct OnceGuard { + state_ptr: *mut c_int, + completed: bool, + } + impl ::core::ops::Drop for OnceGuard { + fn drop(&mut self) { + if !self.completed { + // SAFETY: `state_ptr` was validated by the + // caller of `pthread_once` and remains live + // for the duration of this call. + unsafe { + ::core::ptr::write_volatile(self.state_ptr, ONCE_NEVER_RUN); + } + } + } + } + let mut guard = OnceGuard { + state_ptr, + completed: false, + }; + + // SAFETY: `init_fn` is a non-null `extern "C" fn()` per the + // POSIX contract; the caller is responsible for ensuring it + // does not violate Rust's aliasing rules on shared state. + unsafe { init_fn() }; + + guard.completed = true; + drop(guard); + + // Release fence: ensures all stores performed by `init_fn` + // are globally visible before `ONCE_DONE` becomes observable + // on other CPUs. No-op at runtime in ST/single-CPU mode; + // required for correctness once SMP/MT lands. + ::core::sync::atomic::compiler_fence(::core::sync::atomic::Ordering::Release); + unsafe { ::core::ptr::write_volatile(state_ptr, ONCE_DONE) }; + 0 } diff --git a/src/libs/sysapi/src/sys_types.rs b/src/libs/sysapi/src/sys_types.rs index 163e4da858..8202c9b4cc 100644 --- a/src/libs/sysapi/src/sys_types.rs +++ b/src/libs/sysapi/src/sys_types.rs @@ -301,6 +301,61 @@ impl pthread_once_t { /// Size of `pthread_once_t` structure. pub const SIZE: usize = Self::SIZE_OF_IS_INITIALIZED + Self::SIZE_OF_INIT_EXECUTED; + + /// Sentinel value of `is_initialized` set by `PTHREAD_ONCE_INIT`. + pub const IS_INITIALIZED_VALUE: c_int = 1; + + /// Returns the value of the `is_initialized` field. + /// + /// # Description + /// + /// `is_initialized` is set to `1` by `PTHREAD_ONCE_INIT`. A + /// non-`1` value indicates that the caller forgot to use + /// `PTHREAD_ONCE_INIT` and the `pthread_once_t` is uninitialized. + pub fn is_initialized(&self) -> c_int { + self.is_initialized + } + + /// Returns the value of the `init_executed` field. + /// + /// # Description + /// + /// `init_executed` is used by `pthread_once()` as a state + /// machine to coordinate calling `init_routine` exactly once + /// per `pthread_once_t` object. See the implementation in + /// `libposix` for the state encoding. + pub fn init_executed(&self) -> c_int { + self.init_executed + } + + /// Sets the value of the `init_executed` field. + /// + /// # Description + /// + /// See `init_executed()` for the state encoding. This setter + /// is used by `pthread_once()` to transition between states. + pub fn set_init_executed(&mut self, value: c_int) { + self.init_executed = value; + } + + /// Returns a mutable raw pointer to the `init_executed` field. + /// + /// # Description + /// + /// `pthread_once()` uses this pointer with `ptr::read_volatile` + /// / `ptr::write_volatile` to implement the state-machine + /// transitions without the compiler optimising the loads or + /// stores away. It is the same pointer that a future + /// multi-threaded implementation would pass to `futex_wait` + /// / `futex_wake_all`. + /// + /// # Safety + /// + /// The returned pointer is only valid while the caller holds + /// a reference to the underlying `pthread_once_t`. + pub fn init_executed_ptr(&mut self) -> *mut c_int { + ::core::ptr::addr_of_mut!(self.init_executed) + } } #[derive(Debug, Clone, Copy)] diff --git a/src/libs/syscall/src/pthread/bindings/pthread_key_create.rs b/src/libs/syscall/src/pthread/bindings/pthread_key_create.rs index 7e397cff4b..c29e6fb569 100644 --- a/src/libs/syscall/src/pthread/bindings/pthread_key_create.rs +++ b/src/libs/syscall/src/pthread/bindings/pthread_key_create.rs @@ -32,10 +32,18 @@ pub unsafe extern "C" fn pthread_key_create( return ErrorCode::InvalidArgument.get(); } - // Check if destructor is not null. + // Destructors are not yet supported (would require per-thread + // cleanup on thread exit, which Nanvix's single-threaded + // user-space model has no use for). Rather than refuse the + // call -- which breaks libraries like OpenSSL that always pass + // a non-null destructor as a defensive measure -- we accept + // the call, log a one-line warning, and silently drop the + // destructor. Worst case is a per-thread resource leak at + // thread exit, which is benign in a process-lifetime model. if destructor.is_some() { - ::syslog::warn!("pthread_key_create(): destructors are not supported"); - return ErrorCode::OperationNotSupported.get(); + ::syslog::warn!( + "pthread_key_create(): destructor ignored (not yet supported)" + ); } // Create key.