diff --git a/src/jsc/bindings/JSFFIFunction.cpp b/src/jsc/bindings/JSFFIFunction.cpp index beb3d6122e35..d5c2fd50dc3c 100644 --- a/src/jsc/bindings/JSFFIFunction.cpp +++ b/src/jsc/bindings/JSFFIFunction.cpp @@ -213,13 +213,16 @@ FFI_Callback_threadsafe_call(FFICallbackFunctionWrapper& wrapper, size_t argCoun for (size_t i = 0; i < argCount; ++i) argsVec.append(args[i]); - WebCore::ScriptExecutionContext::postTaskTo(wrapper.m_contextId, [argsVec = WTF::move(argsVec), protectedWrapper = Ref { wrapper }](WebCore::ScriptExecutionContext& ctx) mutable { + // Ref only once the context is found live (inside the map lock) and release via + // adoptRef in the task, so the last deref — destroying two JSC::Strong members — + // can only happen on the JS thread. On a dead/terminating context nothing is destroyed here. + WebCore::ScriptExecutionContext::postTaskTo(wrapper.m_contextId, [&wrapper] { wrapper.ref(); }, [argsVec = WTF::move(argsVec), wrapper = &wrapper](WebCore::ScriptExecutionContext& ctx) mutable { + auto protectedWrapper = adoptRef(*wrapper); auto* globalObject = uncheckedDowncast(ctx.jsGlobalObject()); JSC::MarkedArgumentBuffer arguments; for (size_t i = 0; i < argsVec.size(); ++i) arguments.appendWithCrashOnOverflow(JSC::JSValue::decode(argsVec[i])); - invokeFFICallback(globalObject, protectedWrapper->m_function.get(), arguments); - }); + invokeFFICallback(globalObject, protectedWrapper->m_function.get(), arguments); }); } extern "C" JSC::EncodedJSValue diff --git a/src/jsc/bindings/NodeVMSourceTextModule.cpp b/src/jsc/bindings/NodeVMSourceTextModule.cpp index ae87e5b014e9..67b09a97e8a9 100644 --- a/src/jsc/bindings/NodeVMSourceTextModule.cpp +++ b/src/jsc/bindings/NodeVMSourceTextModule.cpp @@ -318,11 +318,14 @@ JSValue NodeVMSourceTextModule::link(JSGlobalObject* globalObject, JSArray* spec { const unsigned length = specifiers->getArrayLength(); - ASSERT(length == moduleNatives->getArrayLength()); - VM& vm = globalObject->vm(); auto scope = DECLARE_THROW_SCOPE(vm); + if (length != moduleNatives->getArrayLength()) { + Bun::ERR::INVALID_ARG_VALUE(scope, globalObject, "moduleNatives"_s, moduleNatives, "must have the same length as \"specifiers\""_str); + return {}; + } + if (m_status != Status::Unlinked) { throwError(globalObject, scope, ErrorCode::ERR_VM_MODULE_STATUS, "Module must be unlinked before linking"_s); return {}; @@ -338,11 +341,16 @@ JSValue NodeVMSourceTextModule::link(JSGlobalObject* globalObject, JSArray* spec JSValue moduleNativeValue = moduleNatives->getDirectIndex(globalObject, i); RETURN_IF_EXCEPTION(scope, {}); - ASSERT(specifierValue.isString()); + // getDirectIndex returns an empty JSValue for holes; empty passes + // isCell() with a null cell, so it must be rejected before any use. + if (specifierValue.isEmpty() || !specifierValue.isString()) { + Bun::ERR::INVALID_ARG_TYPE(scope, globalObject, "specifiers"_str, "Array"_str, specifierValue.isEmpty() ? jsUndefined() : specifierValue); + return {}; + } WTF::String specifier = specifierValue.toWTFString(globalObject); RETURN_IF_EXCEPTION(scope, {}); - NodeVMModule* moduleNative = dynamicDowncast(moduleNativeValue); + NodeVMModule* moduleNative = moduleNativeValue.isEmpty() ? nullptr : dynamicDowncast(moduleNativeValue); if (!moduleNative) { Bun::ERR::INVALID_THIS(scope, globalObject, "Module"_s); return {}; diff --git a/src/jsc/bindings/webcore/JSWorker.cpp b/src/jsc/bindings/webcore/JSWorker.cpp index 50a94a365c2b..0e94c9adb507 100644 --- a/src/jsc/bindings/webcore/JSWorker.cpp +++ b/src/jsc/bindings/webcore/JSWorker.cpp @@ -174,7 +174,10 @@ template<> JSC::EncodedJSValue JSC_HOST_CALL_ATTRIBUTES JSWorkerDOMConstructor:: RETURN_IF_EXCEPTION(throwScope, {}); if (nameValue) { if (nameValue.isString()) { - options.name = nameValue.toWTFString(lexicalGlobalObject); + // isolatedCopy: m_options.name outlives this call and is read from + // the worker thread; it must not share a (possibly atomized) + // parent-heap StringImpl. + options.name = nameValue.toWTFString(lexicalGlobalObject).isolatedCopy(); RETURN_IF_EXCEPTION(throwScope, {}); } } diff --git a/src/jsc/bindings/webcore/Worker.cpp b/src/jsc/bindings/webcore/Worker.cpp index 296ab0872b38..142bd56baf62 100644 --- a/src/jsc/bindings/webcore/Worker.cpp +++ b/src/jsc/bindings/webcore/Worker.cpp @@ -861,7 +861,10 @@ JSValue createNodeWorkerThreadsBinding(Zig::GlobalObject* globalObject) // Main thread starts at 1 threadId = jsNumber(worker->clientIdentifier() - 1); - threadName = jsString(vm, options.name); + // isolatedCopy: this JSString lives in the worker heap; it must own a + // worker-local impl so its GC deref never races m_options.name's + // (non-atomic) refcount on the parent thread. + threadName = jsString(vm, options.name.isolatedCopy()); } if (!environmentData) { environmentData = JSMap::create(vm, globalObject->mapStructure()); diff --git a/src/runtime/webcore/Blob.rs b/src/runtime/webcore/Blob.rs index ea0e9a67c35d..3e921301c2e9 100644 --- a/src/runtime/webcore/Blob.rs +++ b/src/runtime/webcore/Blob.rs @@ -114,7 +114,9 @@ pub type Ref = bun_ptr::ExternalShared; /// 2: Added byte for whether it's a dom file, length and bytes for `stored_name`, /// and f64 for `last_modified`. /// 3: Added File name serialization for File objects (when is_jsdom_file is true) -const SERIALIZATION_VERSION: u8 = 3; +/// 4: Added the blob's `size` to file-backed stores so a sliced Bun.file() +/// keeps its window's end across structuredClone/postMessage +const SERIALIZATION_VERSION: u8 = 4; pub use bun_jsc::generated::JSBlob as js; @@ -767,6 +769,10 @@ impl BlobExt for Blob { writer.write_int_le::(stored_name.len() as u32)?; writer.write_all(stored_name)?; } else { + // Version 4: a file-backed slice's window end. Written before + // resolve_size() so an unresolved blob stays MAX_SIZE (unknown) + // on the wire and the receiver stats it locally, like v3. + writer.write_int_le::(self.size.get())?; self.resolve_size(); store.serialize(writer)?; } @@ -2338,15 +2344,7 @@ impl BlobExt for Blob { if store_size != MAX_SIZE { self.offset.set(store_size.min(offset)); let available = store_size - self.offset.get(); - // Only resolve an unknown size. A slice already has a concrete - // `size`; overwriting it with `store_size - offset` would widen - // the view to the end of the backing store. Clamp a known size - // to `available` so a bogus size can't report past the store end. - if self.size.get() == MAX_SIZE { - self.size.set(available); - } else { - self.size.set(self.size.get().min(available)); - } + self.size.set(window_size(self.size.get(), available)); } } store::DataTag::File => { @@ -2360,7 +2358,8 @@ impl BlobExt for Blob { let store_size = file.max_size; let offset = self.offset.get(); self.offset.set(store_size.min(offset)); - self.size.set(store_size.saturating_sub(offset)); + let available = store_size - self.offset.get(); + self.size.set(window_size(self.size.get(), available)); return; } @@ -2394,16 +2393,7 @@ impl BlobExt for Blob { if store_size != MAX_SIZE { let offset = store_size.min(offset); let available = store_size - offset; - // Matches `resolve_size`: a known size (e.g. a slice) is - // authoritative; only an unknown size falls back to the - // remainder of the backing store. Clamp to `available` so a - // bogus size can't report past the store end. - let size = if self.size.get() == MAX_SIZE { - available - } else { - self.size.get().min(available) - }; - return (offset, size); + return (offset, window_size(self.size.get(), available)); } (self.offset.get(), self.size.get()) } @@ -2415,8 +2405,9 @@ impl BlobExt for Blob { let file = store.data_mut().as_file(); if file.seekable.is_some() && file.max_size != MAX_SIZE { let store_size = file.max_size; - let offset = self.offset.get(); - return (store_size.min(offset), store_size.saturating_sub(offset)); + let offset = store_size.min(self.offset.get()); + let available = store_size - offset; + return (offset, window_size(self.size.get(), available)); } if file.seekable == Some(false) { return (self.offset.get(), self.size.get()); @@ -4192,6 +4183,10 @@ fn _on_structured_clone_deserialize>( let store_tag = store::SerializeTag::from_raw(reader.read_int_le::()?) .ok_or(crate::Error::InvalidValue)?; + // Version 4: file-backed records carry the blob's own size so a sliced + // Bun.file() keeps its window's end. MAX_SIZE means unknown. + let mut file_size: Option = None; + let blob: *mut Blob = match store_tag { store::SerializeTag::Bytes => 'bytes: { let bytes_len = reader.read_int_le::()?; @@ -4231,6 +4226,9 @@ fn _on_structured_clone_deserialize>( } store::SerializeTag::File => 'file: { use crate::node::types::PathOrFileDescriptorSerializeTag; + if version >= 4 { + file_size = Some(reader.read_int_le::()?); + } let pathlike_tag = PathOrFileDescriptorSerializeTag::from_raw(reader.read_int_le::()?) .ok_or(crate::Error::InvalidValue)?; @@ -4319,6 +4317,12 @@ fn _on_structured_clone_deserialize>( // `offset` comes from untrusted bytes. Clamp it so a crafted payload cannot // make shared_view() slice past the end of the backing store (OOB heap read). blob.offset.set(offset as SizeType); // intentional truncate + if let Some(size) = file_size { + // resolve_size() clamps this to the actual file size on first use. + if size != MAX_SIZE { + blob.size.set(size as SizeType); + } + } if let Some(store) = blob.store.get() { let store_size = store.size(); if store_size != MAX_SIZE { @@ -6227,6 +6231,18 @@ fn stat_to_js_mtime(stat: &bun_sys::Stat) -> jsc::JSTimeType { } } +/// Window clamp shared by the `resolve_size`/`resolved_size` arms: only an +/// unknown (`MAX_SIZE`) size resolves to the store's remainder; a concrete +/// size (a slice's window) is authoritative, clamped so a bogus or stale +/// value can't report past the end of the backing store. +fn window_size(current: SizeType, available: SizeType) -> SizeType { + if current == MAX_SIZE { + available + } else { + current.min(available) + } +} + /// resolve file stat like size, last_modified fn resolve_file_stat(store: &StoreRef) { // `StoreRef::data_mut` encapsulates the raw-pointer deref under the diff --git a/test/js/node/vm/vm.test.ts b/test/js/node/vm/vm.test.ts index 296971c440be..81e49e2e84b3 100644 --- a/test/js/node/vm/vm.test.ts +++ b/test/js/node/vm/vm.test.ts @@ -1058,6 +1058,65 @@ test("node:vm SourceTextModule.link() rejects non-module entries in the moduleNa expect(exitCode).toBe(0); }); +test("node:vm SourceTextModule.link() rejects holey and mismatched argument arrays", async () => { + // Holes in the argument arrays surface as empty JSValues from getDirectIndex, + // which pass isCell() with a null cell — link() must reject them (and a + // specifiers/moduleNatives length mismatch) instead of crashing. + const fixture = ` + const vm = require("node:vm"); + const mod = new vm.SourceTextModule('import { z } from "x"; export const w = z;'); + const kNative = Object.getOwnPropertySymbols(mod).find(s => s.description === "kNative"); + const native = mod[kNative]; + native.createModuleRecord(); + + const results = []; + const attempt = (label, specifiers, moduleNatives) => { + try { + native.link(specifiers, moduleNatives, 0); + results.push(label + ": returned"); + } catch (e) { + results.push(label + ": " + (e instanceof TypeError ? "TypeError" : e.constructor.name) + " " + e.code); + } + }; + + const dep = new vm.SourceTextModule("export const z = 1;"); + const depNative = dep[kNative]; + depNative.createModuleRecord(); + + attempt("holey both", new Array(1), new Array(1)); + attempt("holey specifiers", new Array(1), [depNative]); + attempt("holey moduleNatives", ["x"], new Array(1)); + attempt("length mismatch", ["x"], []); + attempt("non-string specifier", [42], [depNative]); + results.push("status: " + native.getStatus()); + attempt("valid", ["x"], [depNative]); + results.push("status: " + native.getStatus()); + console.log(results.join("\\n")); + `; + + await using proc = Bun.spawn({ + cmd: [bunExe(), "-e", fixture], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + + expect(stderr).toBe(""); + expect(normalizeBunSnapshot(stdout)).toMatchInlineSnapshot(` + "holey both: TypeError ERR_INVALID_ARG_TYPE + holey specifiers: TypeError ERR_INVALID_ARG_TYPE + holey moduleNatives: TypeError ERR_INVALID_THIS + length mismatch: TypeError ERR_INVALID_ARG_VALUE + non-string specifier: TypeError ERR_INVALID_ARG_TYPE + status: unlinked + valid: returned + status: unlinked" + `); + expect(exitCode).toBe(0); +}); + describe("node:vm SourceTextModule cyclic graph linking", () => { // Building a cyclic SourceTextModule graph and linking + evaluating each // module from inside the linker callback (instead of linking the whole graph diff --git a/test/js/node/worker_threads/worker_threads.test.ts b/test/js/node/worker_threads/worker_threads.test.ts index 21a5a8f72a66..cedccbbc66b8 100644 --- a/test/js/node/worker_threads/worker_threads.test.ts +++ b/test/js/node/worker_threads/worker_threads.test.ts @@ -712,6 +712,45 @@ test("transferred FileHandles are not neutered when name/filename validation rej } }); +test("worker name survives parent-side GC and terminate cycles", async () => { + // options.name is materialized as a worker-heap JSString, so it must not + // share a (possibly atomized) parent-heap StringImpl — both threads would + // ref/deref a non-atomic refcount. Stress the path in a subprocess so + // ASAN/debug assertions fail the test loudly. + const fixture = ` + const { Worker } = require("node:worker_threads"); + const src = \` + const { threadName, parentPort } = require("node:worker_threads"); + globalThis.keep = []; + for (let i = 0; i < 50; i++) keep.push(threadName + i); + keep.length = 0; + Bun.gc(true); + parentPort.postMessage(threadName); + \`; + for (let i = 0; i < 4; i++) { + // Object.keys returns strings backed by atomized property-name impls. + const holder = { ["workerNameStress" + i + "Abcdefghij"]: 1 }; + const name = Object.keys(holder)[0]; + const w = new Worker(src, { eval: true, name }); + const got = await new Promise((res, rej) => { w.on("message", res); w.on("error", rej); }); + if (got !== name) throw new Error("name mismatch: " + got); + await w.terminate(); + Bun.gc(true); + } + console.log("done"); + `; + + await using proc = Bun.spawn({ + cmd: [bunExe(), "-e", fixture], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stdout.trim()).toBe("done"); + expect(exitCode).toBe(0); +}); + test("partially transferred FileHandles are restored when a later transfer throws", async () => { const dir = tmpdirSync("worker-fh-transfer"); const file = join(dir, "x.txt"); diff --git a/test/js/web/fetch/blob.test.ts b/test/js/web/fetch/blob.test.ts index b40ff2870fe3..8aa907c12b92 100644 --- a/test/js/web/fetch/blob.test.ts +++ b/test/js/web/fetch/blob.test.ts @@ -633,3 +633,58 @@ test.skipIf(!isASAN).each(["Blob", "File"] as const)( }); }, ); + +// File-backed twin of "slice bounds are respected when streaming and serving" +// above: the File arm of resolve_size()/resolved_size() must not widen a +// sliced Bun.file() to the end of the file either. +describe("file-backed slice bounds are respected when streaming and serving", () => { + test("Bun.file(path).slice(start, end) streams only the slice", async () => { + using dir = tempDir("blob-file-slice", { "data.txt": "0123456789".repeat(10) }); + const s = Bun.file(`${dir}/data.txt`).slice(3, 7); + expect(await new Response(s).text()).toBe("3456"); + // Streaming must not mutate the slice either. + expect(s.size).toBe(4); + + let streamed = 0; + for await (const chunk of new Response(Bun.file(`${dir}/data.txt`).slice(0, 5)).body!) { + streamed += chunk.length; + } + expect(streamed).toBe(5); + }); + + test("content-length of a sliced Bun.file response body", async () => { + using dir = tempDir("blob-file-slice-cl", { "data.txt": "0123456789".repeat(10) }); + await using server = Bun.serve({ + port: 0, + fetch: () => new Response(Bun.file(`${dir}/data.txt`).slice(3, 7)), + }); + + const head = await fetch(server.url, { method: "HEAD" }); + expect(head.headers.get("content-length")).toBe("4"); + + const get = await fetch(server.url); + expect(get.headers.get("content-length")).toBe("4"); + expect(await get.text()).toBe("3456"); + }); + + test("structuredClone keeps the slice size and does not mutate the original", async () => { + using dir = tempDir("blob-file-slice-clone", { "data.txt": "0123456789".repeat(10) }); + const s = Bun.file(`${dir}/data.txt`).slice(0, 5); + expect(s.size).toBe(5); + const clone = structuredClone(s); + expect(clone.size).toBe(5); + expect(s.size).toBe(5); + expect(await clone.text()).toBe("01234"); + expect(await s.text()).toBe("01234"); + }); + + test("slice end beyond EOF clamps to the file size", async () => { + using dir = tempDir("blob-file-slice-eof", { "data.txt": "0123456789" }); + const s = Bun.file(`${dir}/data.txt`).slice(5, 5000); + expect(await new Response(s).text()).toBe("56789"); + const clone = structuredClone(s); + expect(await clone.text()).toBe("56789"); + // Serializing resolves the original's size, clamping the window to EOF. + expect(s.size).toBe(5); + }); +});