Open
Conversation
yonas
pushed a commit
that referenced
this pull request
Apr 16, 2026
…-sh#29330) ## What Adds an early-return at the top of `ResumableSink.cancel()` when `status == .done`, so `onEnd` fires at most once. Fixes oven-sh#20740 Fixes oven-sh#21463 ## Why When a `fetch()` with a `ReadableStream` request body is aborted, `ResumableSink.cancel()` is called from `FetchTasklet.abortListener()` (FetchTasklet.zig:1203). The HTTP thread then completes with failure and `onProgressUpdate`'s reject path calls `sink.cancel()` a second time at FetchTasklet.zig:576 (and `onBodyReceived` at :342) — `this.sink` is only nulled in `clearSink←clearData←deinit`. `cancel()` (ResumableSink.zig:228) guarded against re-entry only for `status == .piped`. For the JS-route sink it relied on `#js_this.tryGet()` returning null after `detachJS()`, but `JSRef.downgrade()` (JSRef.zig:153-160) preserves the wrapper value as `.weak = <wrapper>`, and `tryGet()` (JSRef.zig:111) returns non-null for any non-empty weak. So the second `cancel()` re-enters the block and re-invokes `onEnd` → `FetchTasklet.writeEndRequest` → unconditional `defer this.deref()` (FetchTasklet.zig:1286). That second deref releases the single ref taken in `startRequestStream()` (FetchTasklet.zig:300) twice. Ref-count math: init(1) + queue(1) + startRequestStream(1) = 3 → cancel#1 deref → 2 → derefFromThread → 1 → cancel#2 deref → 0 → `deinit()`/`destroy()` runs *inside* `onProgressUpdate`, then its defer at :471-477 does `this.mutex.unlock()` + `this.deref()` on freed memory. `jsEnd()` already has an `isDetached()` guard for the same reason; `cancel()` was missing the equivalent. Using `status == .done` (rather than `isDetached()`) keeps the `.piped` branch reachable since piped sinks never set `#js_this` to `.strong`. ## Test `test/js/web/fetch/fetch-abort-stream-body.test.ts` reproduces the use-after-free in a debug/ASAN build: ``` [fetchtasklet] abortListener [fetchtasklet] writeEndRequest hasError? true <- cancel #1 [fetchtasklet] callback success=false ... [fetchtasklet] onProgressUpdate [fetchtasklet] onReject [fetchtasklet] writeEndRequest hasError? true <- cancel #2 (over-deref) [fetchtasklet] deinit ==40720==ERROR: AddressSanitizer: use-after-poison ... in onProgressUpdate ```
yonas
pushed a commit
that referenced
this pull request
Apr 22, 2026
…yToRoot (oven-sh#29483) Fuzzilli found a use-after-poison in the runtime auto-install path. `enqueueDependencyToRoot` passed `&lockfile.buffers.dependencies.items[dep_id]` into `enqueueDependencyWithMainAndSuccessFn`. When the manifest for the requested package is already cached (on disk or in memory) but the extracted tarball is not, control reaches `getOrPutResolvedPackageWithFindResult`, which calls `Lockfile.Package.fromNPM`. That grows `buffers.dependencies` via `ensureUnusedCapacity` to make room for the package's own dependencies, reallocating the backing storage. The subsequent `.extract` branch then read `dependency.behavior.isRequired()` from the freed buffer. ``` #0 getOrPutResolvedPackageWithFindResult PackageManagerEnqueue.zig:1520 dependency.behavior.isRequired() #1 getOrPutResolvedPackage PackageManagerEnqueue.zig:1778 #2 enqueueDependencyWithMainAndSuccessFn PackageManagerEnqueue.zig:523 #3 enqueueDependencyToRoot PackageManagerEnqueue.zig:321 #4 Resolver.enqueueDependencyToResolve resolver.zig:2356 ... #14 Bun__resolveSync #15 functionImportMeta__resolveSyncPrivate (runtime require() path) ``` Two changes: - `enqueueDependencyToRoot` now copies the `Dependency` to the stack before taking its address, matching every other caller of `enqueueDependencyWithMainAndSuccessFn` (`processDependencyListItem`, `processPeerDependencyList`, etc.). - The one read that ran after `fromNPM` now uses the `behavior` parameter that was already passed by value, instead of re-dereferencing `dependency`. Repro (debug/ASAN only): auto-install a package with a warm on-disk manifest but no extracted tarball — `fromNPM` appending even a single dependency forces a realloc of the one-entry buffer. The new test warms the cache, removes the extracted tarballs, and runs `require()` via `-e` so it goes through `Bun__resolveSync` → `enqueueDependencyToRoot`.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Updates libarchive to version v3.8.4
Compare: libarchive/libarchive@9525f90...d114cee
Auto-updated by this workflow