dtolnay / dtolnay/inventory

Does this library have expectations of soundness when it comes to rust dependencies being loaded in dylibs?

Open
#92 2 comments 0 reactions 0 assignees View on GitHub
Dominant language
Rust
Stars
1.3k
Forks
59
PR merge metrics
No merged PRs in 30d

Description

> [!NOTE]
> This finding was identified during an agentic unsafe Rust code review performed by Gemini AI, followed by human review and verification.

During an agentic audit of popular crates, I found what _might be_ a safety issue in this crate but only triggerable via the complicated path of dynamically loading a Rust library with a plugin, and then unloading it later.

Dynamic loading is rare, and unloading a loaded library is also rare. That said, in the context of "plugins", dynamic loading is likely to be more common.

Is that the type of thing this crate cares about? I can finish polishing up the issue summary below if so.

Original issue body

The `inventory` crate manages global typed plugin registries backed by intrusive singly linked lists. In `src/lib.rs`, each `Registry` stores the list head pointer in an atomic variable (`head: AtomicPtr`):

https://github.com/dtolnay/inventory/blob/d11c2796c973ee61fa971419b61cdeb500914941/src/lib.rs#L174-L176

When plugins are submitted via `ErasedNode::submit` (invoked by `submit!` macro static constructors), `Registry::submit` inserts the node's static pointer into the linked list head:
https://github.com/dtolnay/inventory/blob/d11c2796c973ee61fa971419b61cdeb500914941/src/lib.rs#L235-L263

When plugins are submitted inside dynamically loaded shared libraries (`dylib` or `cdylib`), OS dynamic linkers execute the static constructor sections (`.init_array` or `.CRT$XCU`) upon library loading (`dlopen`), inserting the library's plugin nodes into the host application's global `Registry`. If the application subsequently unloads the shared library via `dlclose` (or when dropping a `libloading::Library`), the operating system unmaps the memory pages containing the library's data segment, including the submitted `Node` and its underlying plugin `value`. However, `Registry` provides no unregistration hooks or destructors (`__cxa_atexit`) to remove unloaded nodes from the linked list. Consequently, `Registry.head` retains dangling pointers to unmapped memory.

When safe public Rust code subsequently calls `inventory::iter::`, `into_iter()` loads `head` and dereferences the unmapped memory:
https://github.com/dtolnay/inventory/blob/d11c2796c973ee61fa971419b61cdeb500914941/src/lib.rs#L331-L338

This causes an immediate Use-After-Free (dangling pointer dereference and memory corruption or segmentation fault). While dynamic library unloading inherently involves unsafe FFI boundaries, intrusive global linked lists without unregistration hooks create an unmitigated safety hazard in dynamic plugin loading architectures. Furthermore, the public trait method `ErasedNode::submit` (`src/lib.rs:195`) lacks `# Safety` documentation outlining required caller lifetime invariants.

Full Audit Report

# Unsafe Rust Review: `inventory` (`v0_3`)

## Overall Safety Assessment

The `inventory` crate provides distributed typed plugin registration for Rust applications. It allows individual source files across disparate crates to submit plugin instances into a unified registry without requiring a central list. Under the hood, this is achieved by emitting static items containing plugin pointers (`static __INVENTORY: Node`) into OS-specific static constructor linker sections (`.init_array` on Linux/Android/FreeBSD, `.CRT$XCU` on Windows, and `__DATA,__mod_init_func` on macOS).

From an architectural standpoint, `inventory` improves significantly upon arbitrary startup execution crates (such as `ctor`). Plugin values submitted via `inventory::submit!` are evaluated at compile time as const initializers for static items (`value: &{ expr }`), ensuring that user-provided plugin constructors run during Compile-Time Function Evaluation (CTFE) rather than before `main()`. Consequently, runtime panics during startup constructor invocation are structurally impossible. At runtime, the OS loader executes generated wrapper functions (`__ctor`) that perform a simple lock-free singly linked list insertion (`Registry::submit`).

Despite this clean design, the overall safety risk is **Moderate to High in dynamic loading contexts**. Because the crate relies on global linked lists and static constructor sections without corresponding teardown or unregistration mechanisms (`__cxa_atexit`), dynamically unloading shared libraries (`dlclose`) containing submitted plugins inevitably leaves dangling pointers in the registry. Furthermore, the codebase contains multiple `unsafe` operations and macro-generated blocks that lack formal `// SAFETY:` proofs or re-submission invariants.

## Critical Findings

### Dangling pointer dereference / Use-After-Free when plugins are submitted in dynamically unloaded shared libraries (`dlclose`) (Unsoundness) 🔴 ⚠️

- **Priority**: 🔴 High
- **Threat Vector**: ⚠️ Accidental Misuse

- **Bug Type**: Double Free / UAF

When plugins are submitted via `inventory::submit!` inside a dynamically loaded shared library (`cdylib` or `dylib`), loading the library at runtime via `dlopen` invokes the OS loader, which executes the library's static constructor section (`.init_array` or `.CRT$XCU`).

During constructor invocation, the generated `__ctor` wrapper calls `Registry::submit(&__INVENTORY)`, inserting the library's static `__INVENTORY` node into the singly linked list of the global `Registry` (which resides in the host application or core library defining the plugin type `T`).

If the application subsequently unloads the shared library via `dlclose`, the operating system unmaps the memory pages containing `libplugin.so`'s data segment—including `__INVENTORY`, its `value` reference, and its `next` pointer. However, `Registry` provides no unregistration hooks or destructors (`__cxa_atexit`) to remove unloaded nodes from the linked list.

Consequently, `REGISTRY.head` in the host executable retains a pointer to unmapped memory. When safe Rust code subsequently calls `inventory::iter::`, the iterator traverses `REGISTRY.head` and dereferences the dangling pointer to read `node.value` and `node.next`. This results in a segmentation fault and undefined behavior (use-after-free) triggered entirely via safe public APIs.

## Fishy Findings

### 1. Lack of re-submission and cycle protection on non-Wasm platforms 🟡 🧪

- **Priority**: 🟡 Low
- **Threat Vector**: 🧪 Contrived Setup

- **Bug Type**: Data Race

On WebAssembly targets, `Node` includes an `initialized: AtomicBool` field to ensure constructor idempotency:

```rust

#[cfg(target_family = "wasm")]
if new.initialized.swap(true, Ordering::Relaxed) {
return;
}

```

This safeguard prevents duplicate calls to `__wasm_call_ctors` from corrupting the linked list. However, this check is explicitly conditionally compiled out on all non-Wasm platforms (Linux, macOS, Windows, etc.) under the assumption that standard OS dynamic linkers execute constructor sections exactly once.

If `ErasedNode::submit` is invoked manually by unsafe code, or if a non-standard OS loader re-executes `.init_array`, submitting an already-registered `Node` mutates `*node.next.get()` without synchronization while active readers may be traversing the list (data race). Furthermore, it creates a circular reference (`node.next = node`), causing subsequent invocations of `inventory::iter::` to loop infinitely.

### 2. Silently dropped plugins in static library archives (`.a` / `.lib`) 🟡 ⚠️

- **Priority**: 🟡 Low
- **Threat Vector**: ⚠️ Accidental Misuse

- **Bug Type**: Silent Linker Symbol Discarding

While `submit!` emits `#[used]` on `static __CTOR`, this compiler attribute only preserves the symbol during compiler dead-code elimination (DCE). When Rust crates using `submit!` are compiled into static library archives (`.a` or `.lib`) and linked into external C/C++ or Rust binaries, standard linkers (including MSVC `link.exe` without `/WHOLEARCHIVE` or GNU `ld` without `--whole-archive`) resolve symbols strictly on demand.

If no exported symbols from the object file containing `__CTOR` are explicitly referenced by the main binary, the linker discards the entire object file. As a result, submitted plugins are silently omitted from `.init_array` without compiler warnings or build failures.

## Missing Safety Comments

The codebase lacks formal `// SAFETY:` comments or `# Safety` docstrings across almost all internal `unsafe` boundaries. Below are the exact file:line locations in `src/lib.rs` requiring rigorous proof comments:

### 1. `src/lib.rs:189` — `unsafe impl Sync for Node` 🟡

The justification comment on line 187 (`// The value is Sync, and next is only mutated during submit...`) lacks a formal `// SAFETY:` prefix and complete happens-before memory ordering justification.

***Proposed Comment**:

```rust

// SAFETY: `Node` contains `value: &'static dyn ErasedNode` (Sync), `initialized: AtomicBool` (Sync),
// and `next: UnsafeCell>` (!Sync). `next` is only mutated in `Registry::submit`
// prior to the node being published to `Registry.head` via Release CAS. Once published, `next` is
// only read by `Iter` (which loads `head` with Acquire ordering). Because each node is submitted
// at most once, writes to `next` happen-before all reads, preventing data races.

```

### 2. `src/lib.rs:200` — `T::registry().submit(node)` inside `ErasedNode` 🔴

***Proposed Comment**:

```rust

// SAFETY: `ErasedNode::submit`'s precondition requires `*node.value` to be of concrete type `Self` (`T`).
// `T::registry()` returns the static registry declared for `T` in `collect!`. Thus, `node.value`
// matches the plugin type managed by this registry, satisfying `Registry::submit`'s precondition.

```

### 3. `src/lib.rs:251` — `*new.next.get() = head.as_ref()` inside `Registry::submit` 🔴

***Proposed Comment**:

```rust

// SAFETY:
// 1. `new.next.get()` returns a valid, properly aligned pointer to `new.next`. Since `new` has not
// yet been published to `self.head`, no other thread can concurrently read or write `new.next`.
// 2. `head.as_ref()`: `self.head` is initialized to null and only updated via atomic CAS to pointers
// derived from `&'static Node`. Therefore, if `head` is non-null, it points to a valid `'static Node`.

```

### 4. `src/lib.rs:306-307` — `unsafe impl Send/Sync for void_iter::Iter` 🔴

***Proposed Comment**:

```rust

// SAFETY: `void_iter::Iter` contains `Void` (an uninhabited enum), making `Iter` uninhabited.
// Because values of this struct can never exist at runtime, implementing `Send` and `Sync` unconditionally
// cannot lead to data races or thread-unsafe pointer transfers across thread boundaries.

```

### 5. `src/lib.rs:335` — `node: unsafe { head.as_ref() }` inside `into_iter` 🟡

The informal comment on line 334 (`// Head pointer is always null or valid &'static Node.`) lacks the `// SAFETY:` prefix and proof invariant derivation.

***Proposed Comment**:

```rust

// SAFETY: `self.head` is initialized to null and only ever mutated in `Registry::submit` via CAS
// storing pointers to valid `&'static Node` instances. Thus, `head` is either null or valid `'static Node`.

```

### 6. `src/lib.rs:367` — `*node.next.get()` and pointer cast inside `Iter::next` 🔴

***Proposed Comment**:

```rust

// SAFETY:
// 1. `*node.next.get()`: `node` is reachable from `Registry.head`. All reachable nodes have completed
// submission before publication, so `next` is immutable and safe to read concurrently.
// 2. `&*value_ptr`: Every node in `T::registry()` was submitted via `::submit`,
// guaranteeing `*node.value` is concrete type `T`. Casting `*const dyn ErasedNode` to `*const T`
// and dereferencing to `&'static T` is valid and properly aligned.

```

### 7. `src/lib.rs:524` — `ErasedNode::submit` inside macro-generated `__ctor` 🔴

***Proposed Comment**:

```rust

// SAFETY: `__INVENTORY` is constructed with `value: &{ $value }` where `$value` is of type `$ty`.
// The compiler selects `<$ty as ErasedNode>::submit`, whose precondition requires `*__INVENTORY.value`
// to be of type `$ty`. Since `__INVENTORY.value` holds `&$value`, this precondition is satisfied.

```

### 8. Missing `# Safety` documentation on public trait `ErasedNode` (`src/lib.rs:193`) 🟠

`ErasedNode` declares `unsafe fn submit(&self, node: &'static Node);`. While marked `#[doc(hidden)]`, it is a public trait accessible to downstream crates. Its docstring lacks a `# Safety` section detailing required caller invariants.

***Proposed Doc Comment**:

```rust

/// # Safety
/// Callers must guarantee that:
/// 1. `*node.value` is of concrete type `Self`.
/// 2. `node` has not been previously submitted to any registry (to prevent circular lists and data races).
/// 3. The memory backing `node` remains allocated and mapped for the program lifetime (not dynamically unloaded).

```

Contributor guide

No contributing guide indexed for this repository

Research direction

Start in src/lib.rs at Registry::submit and inventory::iter::into_iter, then trace the pointers described in the issue through a dynamically loaded and unloaded plugin. Establish whether dlopen/dlclose is a supported scenario and document the expected safety boundary; done means the project has a decided response to the reported dangling-pointer behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
tooling
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Needs clarification
Newbie friendliness
28/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.