Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions src/jsc/bindings/JSFFIFunction.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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<Zig::GlobalObject>(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
Expand Down
16 changes: 12 additions & 4 deletions src/jsc/bindings/NodeVMSourceTextModule.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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 {};
Expand All @@ -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<string>"_str, specifierValue.isEmpty() ? jsUndefined() : specifierValue);
return {};
}

WTF::String specifier = specifierValue.toWTFString(globalObject);
RETURN_IF_EXCEPTION(scope, {});
NodeVMModule* moduleNative = dynamicDowncast<NodeVMModule>(moduleNativeValue);
NodeVMModule* moduleNative = moduleNativeValue.isEmpty() ? nullptr : dynamicDowncast<NodeVMModule>(moduleNativeValue);
if (!moduleNative) {
Bun::ERR::INVALID_THIS(scope, globalObject, "Module"_s);
return {};
Expand Down
5 changes: 4 additions & 1 deletion src/jsc/bindings/webcore/JSWorker.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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, {});
}
}
Expand Down
5 changes: 4 additions & 1 deletion src/jsc/bindings/webcore/Worker.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down
62 changes: 39 additions & 23 deletions src/runtime/webcore/Blob.rs
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,9 @@ pub type Ref = bun_ptr::ExternalShared<Blob>;
/// 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;

Expand Down Expand Up @@ -767,6 +769,10 @@ impl BlobExt for Blob {
writer.write_int_le::<u32>(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::<u64>(self.size.get())?;
self.resolve_size();
store.serialize(writer)?;
}
Expand Down Expand Up @@ -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 => {
Expand All @@ -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;
}

Expand Down Expand Up @@ -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())
}
Expand All @@ -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());
Expand Down Expand Up @@ -4192,6 +4183,10 @@ fn _on_structured_clone_deserialize<B: AsRef<[u8]>>(
let store_tag = store::SerializeTag::from_raw(reader.read_int_le::<u8>()?)
.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<u64> = None;

let blob: *mut Blob = match store_tag {
store::SerializeTag::Bytes => 'bytes: {
let bytes_len = reader.read_int_le::<u32>()?;
Expand Down Expand Up @@ -4231,6 +4226,9 @@ fn _on_structured_clone_deserialize<B: AsRef<[u8]>>(
}
store::SerializeTag::File => 'file: {
use crate::node::types::PathOrFileDescriptorSerializeTag;
if version >= 4 {
file_size = Some(reader.read_int_le::<u64>()?);
}
let pathlike_tag =
PathOrFileDescriptorSerializeTag::from_raw(reader.read_int_le::<u8>()?)
.ok_or(crate::Error::InvalidValue)?;
Expand Down Expand Up @@ -4319,6 +4317,12 @@ fn _on_structured_clone_deserialize<B: AsRef<[u8]>>(
// `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 {
Expand Down Expand Up @@ -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
Expand Down
59 changes: 59 additions & 0 deletions test/js/node/vm/vm.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
39 changes: 39 additions & 0 deletions test/js/node/worker_threads/worker_threads.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Comment on lines +749 to +751

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Preserve crash diagnostics from the subprocess.

The test captures stderr but discards it, so an ASAN/JSC crash may surface only as a generic stdout or exit-code mismatch. Include stderr and signal information in the failure outcome while keeping the success assertion focused on "done" and exit code 0.

Based on learnings, worker-thread crash-detection tests should preserve stdout, stderr, exitCode, and signalCode together.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/js/node/worker_threads/worker_threads.test.ts` around lines 749 - 751,
Update the subprocess assertions around proc.stdout, proc.stderr, and
proc.exited to preserve stdout, stderr, exitCode, and signalCode in failure
diagnostics. Keep the success assertions focused on stdout.trim() equaling
"done" and exitCode equaling 0, while ensuring stderr and signal information are
included when those assertions fail.

Source: Learnings

});

test("partially transferred FileHandles are restored when a later transfer throws", async () => {
const dir = tmpdirSync("worker-fh-transfer");
const file = join(dir, "x.txt");
Expand Down
55 changes: 55 additions & 0 deletions test/js/web/fetch/blob.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
Comment on lines +681 to +689

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Add a clone.size assertion to catch the EOF-clamp regression on the receiving side.

This test only checks decoded content, which passes regardless of whether clone.size is correctly clamped (file reads stop at physical EOF either way). Add an explicit size assertion on the clone to actually verify the structured-clone receiver clamps the window to EOF, not just the sender.

✅ Suggested addition
     const clone = structuredClone(s);
     expect(await clone.text()).toBe("56789");
+    expect(clone.size).toBe(5);
     // Serializing resolves the original's size, clamping the window to EOF.
     expect(s.size).toBe(5);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
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);
});
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");
expect(clone.size).toBe(5);
// Serializing resolves the original's size, clamping the window to EOF.
expect(s.size).toBe(5);
});
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/js/web/fetch/blob.test.ts` around lines 681 - 689, Extend the “slice end
beyond EOF clamps to the file size” test after structured cloning to assert that
clone.size equals 5. Keep the existing clone.text() assertion and sender-size
assertion unchanged, using the clone created from s to verify EOF clamping on
the receiving side.

});
Loading