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 17, 2026
) ## Problem Fuzzilli hit a flaky SIGSEGV (fingerprint `2519cad1804eace1`) from: ```js const v13 = Bun.jest().vi; try { v13.mock("function f2() {\n const v6 = new ArrayBuffer();\n ...\n}"); } catch (e) {} Bun.gc(true); ``` `JSMock__jsModuleMock` calls `Bun__resolveSyncWithSource` on the specifier before validating the callback, which sends the garbage string through the resolver. The resolver's auto-install gate at `loadNodeModules` only checks `esm_ != null`; `ESModule.Package.parse` accepts anything that doesn't start with `.` or contain `\` / `%`, so the whole function source is treated as a package name. `enqueueDependencyToRoot` then calls `PackageManager.sleepUntil`, which re-enters `EventLoop.tick()` from inside a call that is itself running inside an event-loop tick: ``` #0 ConcurrentTask.PackedNextPtr.atomicLoadPtr #1 UnboundedQueue(ConcurrentTask).popBatch #3 event_loop.tickConcurrentWithCount #7 AnyEventLoop.tick #8 PackageManager.sleepUntil #9 PackageManager.enqueueDependencyToRoot #10 Resolver.resolveAndAutoInstall #16 Bun__resolveSyncWithSource #17 JSMock__jsModuleMock ``` The same path is reachable from `Bun.resolveSync`, `import()`, and `require.resolve` with any user-provided string. ## Fix Gate the auto-install branch on `strings.isNPMPackageName(esm_.?.name)`. That validator already exists and is used by `bun link`, `bun pm view`, and the bundler; it rejects newlines, spaces, braces, and anything else that could never be a registry package. Specifiers failing the check fall straight through to `.not_found` — the same result the registry fetch would eventually produce — without initializing the package manager or ticking the event loop. This is a resolver-level fix, so it covers every entry point (not just `mock.module`). It also avoids spurious network requests for garbage specifiers; on this container a single resolve of a multi-line specifier dropped from ~275ms to ~16ms. ## Tests - `test/js/bun/resolve/resolve-autoinstall-invalid-name.test.ts` stands up a local registry and verifies zero manifest requests for a set of invalid names with `--install=force`, plus a positive control that a valid name still hits the registry. - `test/js/bun/test/mock/mock-module-non-string.test.ts` gains a case for `mock.module` with newline / whitespace / bracket specifiers (with and without a callback). - Existing `test/cli/run/run-autoinstall.test.ts` (11 tests) and `test/js/bun/test/mock/mock-module.test.ts` all pass. Related: oven-sh#28945, oven-sh#28956, oven-sh#28500, oven-sh#28511. Fingerprint: `2519cad1804eace1`
yonas
pushed a commit
that referenced
this pull request
Apr 22, 2026
…uild-cpp) (oven-sh#29545) ## What Adds WebKit-style unified-source bundling to the C++ build and expands the precompiled header. At configure time, `scripts/build/unified.ts` writes `UnifiedSource-<dir>-<n>.cpp` wrappers that `#include` 16 sibling `.cpp` files each, then compiles those instead of the originals. Combined with adding `ZigGlobalObject.h` + `BunClientData.h` to the PCH and dropping a pair of bogus header-level explicit template instantiations, this collapses **547 → 82** translation units. ## Why `-ftime-trace` + ClangBuildAnalyzer on a release `cpp-only` build showed **83 % of compile time is frontend parsing** — every tiny `.cpp` re-parses `ZigGlobalObject.h`, `BunClientData.h`, and the JSDOM converter headers. JSC builds in ~3 min in CI with far more C++ because it bundles 8 files per TU; we were compiling each file standalone. ## Numbers Release `cpp-only`, deps cached, 64-core Linux: | | TUs | CPU time | wall | | --------------------- | --- | -------- | ---- | | main | 547 | 3613 s | — | | `--unifiedSources=off`| 547 | 3464 s | 1:24 | | **this PR** | **82** | **866 s** | **0:40** | ClangBuildAnalyzer: frontend 3004 s → 1260 s → ~550 s; backend 609 s → 407 s. The `JSValueInWrappedObject::visit` template (previously the #1 instantiation at 234 s) no longer appears. ## Knobs - `--unifiedSources=false` restores per-file compilation (useful when iterating on a single `.cpp` and you don't want its 15 bundle-mates recompiling). - `--timeTrace=true` adds `-ftime-trace` so you can re-profile with ClangBuildAnalyzer. - `compile_commands.json` still has an entry per original `.cpp`, so clangd keeps working on individual files. ## Source changes exposed by bundling - `JSValueInWrappedObject.h` — drop `template void ...visit(...)` explicit instantiations from the header (re-instantiated by every includer at ~3.2 s each; upstream WebKit doesn't have them). - `root.h` — add `BunClientData.h` + `ZigGlobalObject.h` to the PCH (parsed once instead of ~130×). - `v8_compatibility_assertions.h` — `__LINE__` → `__COUNTER__` so the namespace-rebinding macro generates unique names per TU. - `V8Context.h` — fix bogus `namespace shim { class Isolate; }` forward decl that was creating a phantom `v8::shim::Isolate` shadowing `v8::Isolate` (real bug, only became visible when shim/ files shared a TU). - Two `#pragma std::once_flag` typos → `#pragma once`; one missing `#pragma once`. - Qualify a handful of `Exception`/`SourceProvider`/`call` references with `JSC::` so they don't become ambiguous when bundled with files that pull in `WebCore::Exception`. - `JSStringDecoder.cpp` — include `JSBufferEncodingType.h` directly instead of relying on a sibling. - `ProcessBindingBuffer.cpp` — `#undef PROCESS_BINDING_NOT_IMPLEMENTED` at EOF so it doesn't leak into the next file in its bundle. ## Excluded from bundling - 16 `webcrypto/CryptoAlgorithm*.cpp` files that share file-static helper names (`aesAlgorithm`, `cryptEncrypt`, `ALG128`, …) — upstream WebKit also compiles these standalone. - `webcore/JSWasmStreamingCompiler.cpp`, `JSDOMPromiseDeferred.cpp`, `JSMessageEventCustom.cpp`, `JSMIMEType.cpp` — wrap types whose `toJS`/`wrapperKey` overloads aren't ADL-reachable, so they rely on ordinary lookup at template-def time. - A handful of large TUs (`ZigGlobalObject.cpp`, `bindings.cpp`, …) that already saturate a core and shouldn't be serialized with siblings. No runtime behaviour change — same code, fewer redundant header parses. ## Follow-ups (not in this PR) - Windows PCH (`/Yc`/`/Yu` plumbing in `compile.ts` is TODO'd). - `ErrorCode.h` pulls in all of `ZigGlobalObject.h` for ~170 TUs that only need forward decls — only matters for `--unifiedSources=false` now. - `BunClientData.h`'s `unique_ptr<ExtendedDOMIsoSubspaces>` instantiates a heavy destructor in every includer; an out-of-line dtor would cut another ~30 s. --------- Co-authored-by: root <root@ip-10-0-2-234.us-west-2.compute.internal> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
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