Turbopack: Fix unsound IntoIterator for ReadRef<T> (#94122)
### What?
Replaces the unsound by-value `IntoIterator` impl for `ReadRef<T>` in
`turbo-tasks` with a sound clone-free variant, and adapts the callers
across `turbopack-*` and `next-api`.
### Why?
The previous impl used `transmute_copy` to fabricate `&'static`-typed
items so it could expose them through the standard `Iterator` trait.
Those references were only really valid as long as the `ReadRef` inside
the iterator stayed alive — but `Iterator::Item` is a fixed associated
type, so once the items were stashed in futures, `Vec`s, or `serde_json`
map keys, the lifetime was completely unenforced.
This produced a latent use-after-free whenever something else
(turbo-tasks cell eviction, an intermediate `Drop`, etc.) released the
underlying storage between the iteration site and the next dereference.
The observed symptom was a panic in `RcStr::as_str` during JSON
serialization of `AssetHashesManifestAsset`'s manifest:
```
thread 'tokio-rt-worker' panicked at turbopack/crates/turbo-rcstr/src/lib.rs:132:52:
range end index 13 out of range for slice of length 7
```
The byte read at the inline-length position was junk left over from
freed/reused memory — `len = 13` is unreachable for any
legitimately-constructed inline `RcStr` (max inline length is 7 on
64-bit). The bug site was
`crates/next-api/src/project_asset_hashes_manifest.rs`, which consumed
an `OutputAssetsWithPaths` `ReadRef`, kept `&RcStr` references in
`asset_paths` past the `try_join` that dropped the iterator, then
serialized them.
### How?
**`turbopack/crates/turbo-tasks/src/read_ref.rs`** — new by-value impl:
```rust
pub struct ReadRefIter<T, I, J>
where
T: VcValueType,
I: Copy + 'static,
J: Iterator<Item = &'static I>,
{
iter: J,
_read_ref: ReadRef<T>,
}
impl<T, I, J> Iterator for ReadRefIter<T, I, J> /* … */ {
type Item = I;
fn next(&mut self) -> Option<I> { self.iter.next().copied() }
}
impl<T, I, J> IntoIterator for ReadRef<T>
where
T: VcValueType,
I: Copy + 'static,
J: Iterator<Item = &'static I> + 'static,
&'static VcReadTarget<T>: IntoIterator<Item = &'static I, IntoIter = J>,
{
type Item = I;
type IntoIter = ReadRefIter<T, I, J>;
fn into_iter(self) -> Self::IntoIter {
let r: &VcReadTarget<T> = &self;
// SAFETY: the fabricated `&'static` reference is only stored inside
// `iter`, which lives inside the returned `ReadRefIter` alongside
// the `ReadRef` that owns the data. `next()` only ever yields
// `Copy`-ed-out values — no reference (with the fake `'static`
// lifetime or otherwise) ever leaves the iterator. Struct-field drop
// order (`iter` then `_read_ref`) drops the borrow before the
// backing storage.
let r = unsafe { std::mem::transmute::<&VcReadTarget<T>, &'static VcReadTarget<T>>(r) };
ReadRefIter { iter: r.into_iter(), _read_ref: self }
}
}
```
Key properties:
- **No cloning.** Setup is one borrow + `transmute`; `next()` is
`Option::copied()` (bitwise copy via the `Copy` bound), not
`Clone::clone`. Nothing in the iterator clones the backing collection or
its elements.
- **Contained `unsafe`.** The fake `'static` reference never leaves
`ReadRefIter`. `Iterator::next` yields `I` by value, so the lifetime
never escapes into futures, `Vec`s, or other persistence outside the
iterator.
- **Drop order safe.** Struct fields drop in declaration order: `iter`
(and any borrows it holds) drops before `_read_ref` (the backing `Arc`).
- **`Copy` bound.** The impl is restricted to element types that are
`Copy` — `ResolvedVc<_>`, integer ids, owned-tuple-of-`Copy`, etc. For
non-`Copy` element types (`RcStr`, `FileSystemPath`, `PatternMatch`,
`(String, _)`, `(ModuleId, ReadRef<_>)`, …) callers iterate by reference
via the existing `IntoIterator for &'a ReadRef<T>` impl (`for x in
&read_ref` or `read_ref.iter()`).
The original buggy site in `project_asset_hashes_manifest.rs` now uses
`output_assets.iter()` and keeps `&'a RcStr` references in the manifest
struct. The borrow checker now enforces the lifetime that used to be
faked via `transmute` — `output_assets` outlives the references because
nothing consumes it, and there are no clones at the call site either.
**Caller adjustments.** Touching the impl forced a sweep of all call
sites that were implicitly leaning on the unsound shape (yielding
`&'static`-typed items as a stand-in for owned items). The fixes fall
into a small number of categories:
- Drop redundant `.copied()` / `.cloned()` / `|&x| f(x)` patterns after
`into_iter()` (items are owned `Copy` values now, no need to
deref-and-copy).
- Switch non-`Copy` element iteration to `&read_ref` / `read_ref.iter()`
(e.g. `PatternMatches`, `CodeAndIds`, `UnresolvedUrlReferences`,
`GraphEntries`, `Vec<RcStr>`).
- Reshape `crates/next-api/src/paths.rs` helpers from `impl
IntoIterator<Item = &ResolvedVc<_>>` to `impl IntoIterator<Item =
ResolvedVc<_>>` — `ResolvedVc` is `Copy`, so by-value is the natural
shape and it composes directly with the new by-value
`ReadRef::into_iter`. Callers in `app.rs`, `pages.rs`, `middleware.rs`,
`instrumentation.rs`, `font.rs` updated to match (either passing the
`ReadRef`/`Vec` directly, or `.iter().copied()` for borrowed sources).
- A few small follow-ups: `for (key, EndpointGroup { primary, .. }) in
&entrypoint_groups` in `routes_hashes_manifest.rs` (with a borrowed `&'l
str` key in the manifest); `compute_async_module_info_single(graph,
result)` (no `*graph`, it's already `Copy`); `&(ty, batch)` → `(ty,
batch)` destructures in `chunking/mod.rs`.
### Testing
- `cac` clean across the workspace.
- `ca clippy --all-targets` clean.
- `ca test -p turbo-tasks-backend` — all unit + integration tests pass.
- `ca test -p turbopack-tests --tests` — execution snapshot suite (218
passed, 0 failed, 1 ignored) and snapshot suite (87 passed, 0 failed).
Closes NEXT-
Fixes #
