Can wrong_ast_path be unsafe?

[Manishearth]

Discovered during an agentic unsafe audit.

swc/crates/swc_visit/src/lib.rs

Lines 602 to 606 in 0216889

	pub fn wrong_ast_path() {
	unsafe {
	debug_unreachable::debug_unreachable!("Wrong ast path");
	}
	}

This is straight up unsound, but it's "fine" because it's doc(hidden).

Having this be marked safe makes it harder to audit callers in other crates. Perhaps it should be unsafe or named wrong_ast_path_unsafe? When auditing a caller crate I'd like to know that the reachability is soundness-impacting.

---

Note

The full audit report below also contains additional minor findings (such as missing safety comments or undocumented FFI assumptions) that are probably worth fixing as well but not the primary goal of this issue. The audit report has not been human-reviewed, it may contain misleading claims.

Full Gemini Codebase Audit Report Appendix

Unsafe Rust Review: swc_visit (v2)

Overall Safety Assessment

swc_visit is a core visitor generation framework crate for the SWC compiler project, providing the Fold, VisitMut, and Visit AST traversal patterns along with path-aware visiting utilities. The crate exhibits a very low density of unsafe code, confined to functional container-mapping utilities (Map for Box<T> and MoveMap for Vec<T>) and an unreachable code helper (wrong_ast_path).

Architecturally, the Map and MoveMap implementations demonstrate sound zero-allocation functional programming techniques over heap allocations. By converting containers to raw pointers (Box::into_raw, Vec::as_mut_ptr) and leveraging ptr::read / ptr::write, the crate maps elements in place without allocating new container buffers. Crucially, the implementations maintain strict exception safety during closure panics by intentionally leaking container allocations or unmapped elements (Vec::set_len(0)), guaranteeing that unwinding never triggers double-frees or accesses to logically uninitialized memory.

However, the crate contains a direct soundness vulnerability in pub fn wrong_ast_path(), which exposes std::hint::unreachable_unchecked() to safe Rust callers. Furthermore, none of the unsafe blocks across the crate are documented with // SAFETY: proof obligations.

Critical Findings

1. Unsound exposure of std::hint::unreachable_unchecked() via safe public API 🔴 ⚠️

• Priority: 🔴 High

• Threat Vector: ⚠️ Accidental Misuse

• Bug Type: Missing Unsafe Qualifier

• Location: src/lib.rs:602-606

• Vulnerability: Safe public function calling debug_unreachable!(), triggering immediate Undefined Behavior in release builds.

• Analysis:

  pub fn wrong_ast_path() {
      unsafe {
          debug_unreachable::debug_unreachable!("Wrong ast path");
      }
  }

In release mode, debug_unreachable::debug_unreachable!() expands to std::hint::unreachable_unchecked(). Invoking std::hint::unreachable_unchecked() causes immediate Undefined Behavior. Because wrong_ast_path() is a safe public function (pub fn), any safe Rust code (such as external downstream crates or macro expansions) can invoke it without an unsafe block or safety proof. While the function is marked #[doc(hidden)] with a comment stating /// NOT A PUBLIC API, visibility attributes and doc comments do not enforce safety invariants in the Rust type system. Under Rust safety principles, any function that can trigger Undefined Behavior when called from safe code is unsound.

• Remediation: If calling this function represents an invariant violation whose unreachability must be upheld by the caller, it must be marked pub unsafe fn wrong_ast_path(), accompanied by a # Safety section specifying the exact proof obligations required of callers. Conversely, if it is intended to be called by safe macro expansions where panics are expected upon invalid AST paths, it should use standard panic!() or unreachable!() instead of debug_unreachable!().

Fishy Findings

1. Undocumented container leaks on closure panic in MoveMap 🟡 ⚠️

• Priority: 🟡 Low

• Threat Vector: ⚠️ Accidental Misuse

• Bug Type: Undocumented Memory Leak

• Location: src/util/move_map.rs:25-47, src/util/move_map.rs:49-93

• Analysis: MoveMap::move_map and MoveMap::move_flat_map set the vector length to 0 (self.set_len(0)) before iterating over elements and invoking user-supplied closures (FnMut). If a closure panics midway through traversal, the remaining unmapped vector elements are leaked (their memory remains allocated but Drop is never executed). While leaking memory is strictly safe in Rust (std::mem::forget), this behavior is completely undocumented on the MoveMap trait methods (unlike Map::map in map.rs, which explicitly documents # Memory leak). Downstream callers catching panics via std::panic::catch_unwind may unexpectedly leak resources.

Missing Safety Comments

1. src/lib.rs:603 🔴

• Missing: // SAFETY: proof obligation on internal unsafe block.

• Proposed Comment:

  // SAFETY: The caller guarantees via safety invariants that runtime execution never reaches this code path.

2. src/util/map.rs:26 🟡

• Missing: // SAFETY: proof obligation on Box<T> in-place mapping.

• Proposed Comment:

  // SAFETY:
  // - `p` was obtained immediately prior via `Box::into_raw(self)`, so it is non-null, properly aligned,
  //   and points to a valid, allocated heap allocation of type `T`.
  // - `ptr::read(p)` is safe because `p` is valid for reads and properly initialized.
  // - `f(...)` takes ownership of the moved `T`. If `f` panics, execution unwinds and `p` is leaked (not dropped),
  //   preventing double-free.
  // - `ptr::write(p, ...)` is safe because `p` points to valid allocated memory for `T` and is properly aligned.
  //   It overwrites the old slot without dropping the uninitialized duplicate left by `ptr::read`.
  // - `Box::from_raw(p)` is safe because `p` was originally allocated by `Box` via the global allocator with layout `T`,
  //   is non-null, and now contains a newly written, valid initialized `T`.

3. src/util/move_map.rs:29 🟡

• Missing: // SAFETY: proof obligation on Vec<T>::move_map.

• Proposed Comment:

  // SAFETY:
  // - `self.set_len(0)` ensures that if `f(item)` panics, the vector drops 0 elements during unwinding,
  //   preventing double-free or access to uninitialized memory.
  // - For `index in 0..old_len`, `self.as_mut_ptr().add(index)` is within the vector's allocated capacity
  //   because `old_len <= self.capacity()`. Thus the pointer is non-null, properly aligned, and valid for reads/writes.
  // - `ptr::read(item_ptr)` moves out the initialized element at `index`.
  // - `ptr::write(item_ptr, item)` writes the transformed element back into the exact same allocated slot.
  // - `self.set_len(old_len)` is safe because all indices from `0..old_len` have been overwritten with valid,
  //   initialized values of type `T`.

4. src/util/move_map.rs:56 🟡

• Missing: // SAFETY: proof obligation on Vec<T>::move_flat_map.

• Proposed Comment:

  // SAFETY:
  // - `self.set_len(0)` ensures that if closure `f` or iterator operations panic, the vector drops 0 elements
  //   during unwinding, leaking remaining elements to prevent double-free of moved items.
  // - `self.as_ptr().add(read_i)` is within allocated bounds because `read_i < old_len <= self.capacity()`.
  //   `ptr::read(...)` is valid because elements at `read_i..old_len` contain valid initialized values.
  // - `self.as_mut_ptr().add(write_i)` is valid for writes because `write_i <= capacity`.
  // - When `write_i == read_i`, indices `0..write_i` contain newly written items from iterator expansions,
  //   and indices `read_i..old_len` contain unread original items. Thus all `0..old_len` items are valid and initialized,
  //   making `self.set_len(old_len)` sound before calling `self.insert(...)`.
  // - At loop exit, exactly `write_i` items have been written to indices `0..write_i`, so `self.set_len(write_i)`
  //   safely exposes only initialized elements.