Skip to content
Open
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
23 changes: 14 additions & 9 deletions src/js/internal/assert/assertion_error.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,14 @@
"use strict";

const { inspect } = require("internal/util/inspect");
const colors = require("internal/util/colors");
const { validateObject } = require("internal/validators");
const { myersDiff, printMyersDiff, printSimpleMyersDiff } = require("internal/assert/myers_diff") as typeof Internal;
let _inspect;
function inspect(_value, _opts) {
return (_inspect ??= require("internal/util/inspect").inspect).$apply(this, arguments);
}
let _myers;
function myers(): typeof Internal {
return (_myers ??= require("internal/assert/myers_diff"));
}

const ErrorCaptureStackTrace = Error.captureStackTrace;
const ObjectAssign = Object.assign;
Expand Down Expand Up @@ -110,8 +115,8 @@ function getColoredMyersDiff(actual, expected) {
const skipped = false;

// const diff = myersDiff(StringPrototypeSplit.$call(actual, ""), StringPrototypeSplit.$call(expected, ""));
const diff = myersDiff(actual, expected, false, false);
let message = printSimpleMyersDiff(diff);
const diff = myers().myersDiff(actual, expected, false, false);
let message = myers().printSimpleMyersDiff(diff);

if (skipped) {
message += "...";
Expand Down Expand Up @@ -220,8 +225,8 @@ function createErrDiff(actual, expected, operator, customMessage) {
const checkCommaDisparity = actual != null && typeof actual === "object";
let myersDiffMessage;
try {
const diff = myersDiff(inspectedActual, inspectedExpected, checkCommaDisparity, true);
myersDiffMessage = printMyersDiff(diff);
const diff = myers().myersDiff(inspectedActual, inspectedExpected, checkCommaDisparity, true);
myersDiffMessage = myers().printMyersDiff(diff);
} catch {
myersDiffMessage = undefined;
}
Expand Down Expand Up @@ -263,7 +268,7 @@ class AssertionError extends Error {
operator;

constructor(options) {
validateObject(options, "options");
require("internal/validators").validateObject(options, "options");
const {
message,
operator,
Expand Down Expand Up @@ -415,7 +420,7 @@ class AssertionError extends Error {
return `${this.name} [${this.code}]: ${this.message}`;
}

[inspect.custom](recurseTimes, ctx) {
[Symbol.for("nodejs.util.inspect.custom")](recurseTimes, ctx) {
// Long strings should not be fully inspected.
const tmpActual = this.actual;
const tmpExpected = this.expected;
Expand Down
4 changes: 2 additions & 2 deletions src/js/internal/cluster/child.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
const EventEmitter = require("node:events");
const Worker = require("internal/cluster/Worker");
const path = require("node:path");

const sendHelper = $newRustFunction("node_cluster_binding.rs", "sendHelperChild", 3);
const onInternalMessage = $newRustFunction("node_cluster_binding.rs", "onInternalMessageChild", 2);
Expand Down Expand Up @@ -62,7 +61,8 @@ cluster._getServer = function (obj, options, cb) {
let address = options.address;

// Resolve unix socket paths to absolute paths
if (options.port < 0 && typeof address === "string" && process.platform !== "win32") address = path.resolve(address);
if (options.port < 0 && typeof address === "string" && process.platform !== "win32")
address = require("node:path").resolve(address);

const indexesKey = ArrayPrototypeJoin.$call([address, options.port, options.addressType, options.fd], ":");

Expand Down
13 changes: 5 additions & 8 deletions src/js/internal/cluster/primary.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,6 @@
const EventEmitter = require("node:events");
const Worker = require("internal/cluster/Worker");
const RoundRobinHandle = require("internal/cluster/RoundRobinHandle");
const SharedHandle = require("internal/cluster/SharedHandle");
const path = require("node:path");
const { throwNotImplemented, kHandle } = require("internal/shared");
const { kHandle } = require("internal/shared");

const sendHelper = $newRustFunction("node_cluster_binding.rs", "sendHelperPrimary", 4);
const onInternalMessage = $newRustFunction("node_cluster_binding.rs", "onInternalMessagePrimary", 3);
Expand Down Expand Up @@ -240,7 +237,7 @@ function queryServer(worker, message) {

// Find shortest path for unix sockets because of the ~100 byte limit
if (message.port < 0 && typeof address === "string" && process.platform !== "win32") {
address = path.relative(process.cwd(), address);
address = require("node:path").relative(process.cwd(), address);

if (message.address.length < address.length) address = message.address;
}
Expand All @@ -260,11 +257,11 @@ function queryServer(worker, message) {
worker.emit("error", error);
return;
}
handle = new SharedHandle(key, address, message);
handle = new (require("internal/cluster/SharedHandle"))(key, address, message);
} else if (schedulingPolicy !== SCHED_RR) {
throwNotImplemented("node:cluster SCHED_NONE");
require("internal/shared").throwNotImplemented("node:cluster SCHED_NONE");
} else {
handle = new RoundRobinHandle(key, address, message);
handle = new (require("internal/cluster/RoundRobinHandle"))(key, address, message);
}

handles.set(key, handle);
Expand Down
3 changes: 1 addition & 2 deletions src/js/internal/errors.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
const { SafeArrayIterator } = require("internal/primordials");

const ArrayIsArray = Array.isArray;
const ArrayPrototypePush = Array.prototype.push;

Expand All @@ -11,6 +9,7 @@ function aggregateTwoErrors(innerError: Error | undefined, outerError: Error & {
ArrayPrototypePush.$call(outerErrors, innerError);
return outerError;
}
const { SafeArrayIterator } = require("internal/primordials");
const err = new AggregateError(new SafeArrayIterator([outerError, innerError]), outerError.message);
err.code = outerError.code;
return err;
Expand Down
36 changes: 19 additions & 17 deletions src/js/internal/fs/streams.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,21 @@ import type { FileSink } from "bun";
const { Readable, Writable, finished } = require("node:stream");
const fs: typeof import("node:fs") = require("node:fs");
const { read, write, fsync, writev } = fs;
const { FileHandle, kRef, kUnref, kFd } = (fs.promises as any).$data as {
FileHandle: { new (): FileHandle };
readonly kRef: unique symbol;
readonly kUnref: unique symbol;
readonly kFd: unique symbol;
};
let FileHandle: { new (): FileHandle };
let kRef, kUnref, kFd;
let fileHandlePrototypeRead, fileHandlePrototypeWrite, fileHandlePrototypeFsync, fileHandlePrototypeWritev;
function loadFileHandle() {
if (FileHandle === undefined) {
({ FileHandle, kRef, kUnref, kFd } = (require("node:fs/promises") as any).$data);
({
read: fileHandlePrototypeRead,
write: fileHandlePrototypeWrite,
fsync: fileHandlePrototypeFsync,
writev: fileHandlePrototypeWritev,
} = FileHandle.prototype);
}
return FileHandle;
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
type FileHandle = import("node:fs/promises").FileHandle & {
on(event: any, listener: any): FileHandle;
};
Expand All @@ -33,22 +42,13 @@ type FSStream = import("node:fs").ReadStream &
};
type FD = number;

const { validateInteger, validateInt32, validateFunction } = require("internal/validators");

const kIsPerformingIO = Symbol("kIsPerformingIO");
const kIoDone = Symbol("kIoDone");
// Bun supports a fast path for `createWriteStream("path.txt")` where instead of
// using `node:fs`, `Bun.file(...).writer()` is used instead.
const kWriteStreamFastPath = Symbol("kWriteStreamFastPath");
const kFs = Symbol("kFs");

const {
read: fileHandlePrototypeRead,
write: fileHandlePrototypeWrite,
fsync: fileHandlePrototypeFsync,
writev: fileHandlePrototypeWritev,
} = FileHandle.prototype;

const fileHandleStreamFs = (fh: FileHandle) => ({
// try to use the basic fs.read/write/fsync if available, since they are less
// abstractions. however, node.js allows patching the file handle, so this has
Expand Down Expand Up @@ -132,6 +132,7 @@ function ReadStream(this: FSStream, path, options): void {
return new ReadStream(path, options);
}

const { validateInteger, validateInt32, validateFunction } = require("internal/validators");
options = copyObject(getStreamOptions(options));

// Only buffers are supported.
Expand Down Expand Up @@ -161,7 +162,7 @@ function ReadStream(this: FSStream, path, options): void {
}
this.fd = fd;
this[kFs] = customFs || fs;
} else if (typeof fd === "object" && fd instanceof FileHandle) {
} else if (typeof fd === "object" && fd instanceof loadFileHandle()) {
if (options.fs) {
throw $ERR_METHOD_NOT_IMPLEMENTED("fs.FileHandle with custom fs operations");
}
Expand Down Expand Up @@ -393,6 +394,7 @@ function WriteStream(this: FSStream, path: string | null, options?: any): void {
}

let fastPath = options?.$fastPath;
const { validateInteger, validateInt32, validateFunction } = require("internal/validators");

options = copyObject(getStreamOptions(options));

Expand Down Expand Up @@ -422,7 +424,7 @@ function WriteStream(this: FSStream, path: string | null, options?: any): void {
}
this.fd = fd;
this[kFs] = customFs || fs;
} else if (typeof fd === "object" && fd instanceof FileHandle) {
} else if (typeof fd === "object" && fd instanceof loadFileHandle()) {
if (options.fs) {
throw $ERR_METHOD_NOT_IMPLEMENTED("fs.FileHandle with custom fs operations");
}
Expand Down
5 changes: 2 additions & 3 deletions src/js/internal/http/FakeSocket.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
const { kInternalSocketData, serverSymbol } = require("internal/http");
const { kAutoDestroyed } = require("internal/shared");
const { Duplex } = require("internal/stream");
const Duplex = require("internal/streams/duplex");

type FakeSocket = InstanceType<typeof FakeSocket>;
var FakeSocket = class Socket extends Duplex {
Expand Down Expand Up @@ -38,7 +37,7 @@ var FakeSocket = class Socket extends Duplex {
_destroy(_err, _callback) {
const socketData = this[kInternalSocketData];
if (!socketData) return; // sometimes 'this' is Socket not FakeSocket
if (!socketData[1]["req"][kAutoDestroyed]) socketData[1].end();
if (!socketData[1]["req"][require("internal/shared").kAutoDestroyed]) socketData[1].end();
}

_final(_callback) {}
Expand Down
113 changes: 72 additions & 41 deletions src/js/internal/primordials.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,17 @@ function uncurryThis(func) {
return FunctionPrototypeCall.bind(func);
}

const ArrayPrototypeForEach = uncurryThis(Array.prototype.forEach);
const ArrayPrototypeMap = uncurryThis(Array.prototype.map);
const ArrayPrototypeSymbolIterator = uncurryThis(Array.prototype[Symbol.iterator]);
const ArrayIteratorPrototypeNext = uncurryThis(Array.prototype[Symbol.iterator]().next);
const StringPrototypeSymbolIterator = uncurryThis(String.prototype[Symbol.iterator]);
const StringIteratorPrototypeNext = uncurryThis(
Reflect.getPrototypeOf(String.prototype[Symbol.iterator].$call("")).next,
);
const PromiseAll = Promise.all;
const PromiseResolve = Promise.$resolve.bind(Promise);

const copyProps = (src, dest) => {
ArrayPrototypeForEach(Reflect.ownKeys(src), key => {
if (!Reflect.getOwnPropertyDescriptor(dest, key)) {
Expand Down Expand Up @@ -77,26 +88,23 @@ const makeSafe = (unsafe, safe) => {
return safe;
};

const StringIterator = uncurryThis(String.prototype[Symbol.iterator]);
const StringIteratorPrototype = Reflect.getPrototypeOf(StringIterator(""));
const ArrayPrototypeForEach = uncurryThis(Array.prototype.forEach);
const ArrayPrototypeSymbolIterator = uncurryThis(Array.prototype[Symbol.iterator]);
const ArrayIteratorPrototypeNext = uncurryThis(Array.prototype[Symbol.iterator]().next);
const SafeArrayIterator = createSafeIterator(ArrayPrototypeSymbolIterator, ArrayIteratorPrototypeNext);
let SafeArrayIterator;
function getSafeArrayIterator() {
return (SafeArrayIterator ??= createSafeIterator(ArrayPrototypeSymbolIterator, ArrayIteratorPrototypeNext));
}

const ArrayPrototypeMap = Array.prototype.map;
const PromisePrototypeThen = $Promise.prototype.$then;

const arrayToSafePromiseIterable = (promises, mapFn) =>
new SafeArrayIterator(
ArrayPrototypeMap.$call(
const arrayToSafePromiseIterable = (promises, mapFn) => {
const SafeArrayIterator = getSafeArrayIterator();
return new SafeArrayIterator(
ArrayPrototypeMap(
promises,
(promise, i) =>
new Promise((a, b) => PromisePrototypeThen.$call(mapFn == null ? promise : mapFn(promise, i), a, b)),
),
);
const PromiseAll = Promise.all;
const PromiseResolve = Promise.$resolve.bind(Promise);
};
const SafePromiseAll = (promises, mapFn) => PromiseAll(arrayToSafePromiseIterable(promises, mapFn));
// Shared scheduler for SafePromiseAllReturnVoid/ReturnArrayLike: `returnVal`
// is null for the void variant (no result bookkeeping, resolves with nothing).
Expand Down Expand Up @@ -126,64 +134,87 @@ const SafePromiseAllReturnArrayLike = (promises, mapFn) => {
return safePromiseAllCollect(promises, mapFn, returnVal);
};

export default {
const primordials = {
Array,
SafeArrayIterator,
MapPrototypeGetSize: getGetter(Map, "size"),
Number,
Object,
RegExp,
SafeStringIterator: createSafeIterator(StringIterator, uncurryThis(StringIteratorPrototype.next)),
SafeMap: makeSafe(
SafePromiseAll,
SafePromiseAllReturnArrayLike,
SafePromiseAllReturnVoid,
String,
Uint8ClampedArray,
Uint8Array,
Uint16Array,
Uint32Array,
Int8Array,
Int16Array,
Int32Array,
Float16Array,
Float32Array,
Float64Array,
BigUint64Array,
BigInt64Array,
uncurryThis,
};

function defineLazy(name, initialize) {
Object.defineProperty(primordials, name, {
get() {
const value = initialize();
Reflect.defineProperty(primordials, name, { value, writable: true, enumerable: true, configurable: true });
return value;
},
enumerable: true,
configurable: true,
});
}

defineLazy("SafeArrayIterator", getSafeArrayIterator);
defineLazy("MapPrototypeGetSize", () => getGetter(Map, "size"));
defineLazy("SetPrototypeGetSize", () => getGetter(Set, "size"));
defineLazy("TypedArrayPrototypeGetLength", () => getGetter(Uint8Array, "length"));
defineLazy("TypedArrayPrototypeGetSymbolToStringTag", () => getGetter(Uint8Array, Symbol.toStringTag));
defineLazy("SafeStringIterator", () => createSafeIterator(StringPrototypeSymbolIterator, StringIteratorPrototypeNext));
defineLazy("SafeMap", () =>
makeSafe(
Map,
class SafeMap extends Map {
constructor(i) {
Comment on lines +172 to 183

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.

🟡 Wrapping SafeMap/SafeSet/SafeWeakMap/SafeWeakSet and the *PrototypeGetSize getters in defineLazy() moves makeSafe()/getGetter()'s prototype snapshot from primordials-load to first-property-access, so a builtin loading primordials no longer immunizes the Safe* entries it doesn't touch (e.g. readable.ts's SafeSet now snapshots on the first multi-dest .pipe() rather than at module load). Bun's primordials was already load-order-dependent and none of the affected sites are security boundaries, so this is a conscious perf-vs-tamper-resistance tradeoff worth acknowledging rather than a blocker — a cheap mitigation is to capture the raw Map/Set/WeakMap/WeakSet prototype refs into module-level consts (as already done for the iterator inputs) and have the lazy makeSafe/getGetter read from those.

Extended reasoning...

What changed

makeSafe(unsafe, safe) copies own-property descriptors from unsafe.prototype at call time (Reflect.getOwnPropertyDescriptor(unsafePrototype, key)), and getGetter(cls, name) reads cls.prototype.__lookupGetter__(name) at call time. Before this PR, both ran synchronously during internal/primordials module evaluation for every Safe* class and every *PrototypeGetSize/TypedArrayPrototypeGet* getter. After this PR, each is wrapped in defineLazy(), so the snapshot for a given entry is taken on that entry's first property access instead.

Why this widens the tamper window (narrowly)

Previously, loading internal/primordials for any reason snapshotted all Safe* classes atomically. Now each entry materializes independently. Two concrete deltas:

  1. Lost cross-module protection. require('node:readline') loads primordials but only touches SafeStringIterator. Before: that also built SafeWeakSet from the clean prototype. After: if user code then tampers WeakSet.prototype.has before require('node:assert') first destructures SafeWeakSet, makeSafe copies the tampered method.
  2. readable.ts's SafeSet moved from top-level destructure to inside pipe(). Before this PR the awaitDrainWriters SafeSet was snapshotted when internal/streams/readable loaded; now it snapshots the first time a Readable is piped to a second destination. User code that tampers Set.prototype.add between those two points now poisons the "safe" set.

SafeArrayIterator/SafeStringIterator are not affected: their factory/next inputs (ArrayPrototypeSymbolIterator, ArrayIteratorPrototypeNext, StringPrototypeSymbolIterator, StringIteratorPrototypeNext) are still captured eagerly at lines 39-46, so their lazy createSafeIterator() cannot observe tampering.

Addressing the refutation

One verifier argued this is not a real regression because (a) Bun's primordials was never bootstrap-loaded, so tamper-resistance was always load-order-dependent, and (b) every consumer destructures at its own module top-level, firing the lazy getter synchronously during that same require() — so per-consumer snapshot timing is unchanged.

Point (a) is correct and important: this file was never a hard tamper boundary. Pre-PR, user code that tampered Set.prototype before requiring any primordials-consuming builtin already got a tampered SafeSet. This PR does not create a new bug class — it widens an existing conditional window. That's why this is a nit, not a blocker.

Point (b) is mostly right but not universally: readable.ts's SafeSet destructure moved out of module top-level and into pipe() in this same PR, so for that consumer the snapshot genuinely moved from module-load to first-multi-dest-pipe. And the cross-module "loading primordials for A also protects B" property — while never a documented contract — was real defense-in-depth that is now gone.

Step-by-step example

  1. User's entrypoint: const { Readable } = require('stream') → loads internal/streams/readable, which loads internal/primordials. Pre-PR: SafeSet is built here from clean Set.prototype. Post-PR: SafeSet is not touched (readable.ts no longer top-level destructures it), so the lazy getter stays armed.
  2. User: Set.prototype.add = function () {}.
  3. User: readable.pipe(dest1); readable.pipe(dest2) → second pipe hits the kMultiAwaitDrain branch, does const { SafeSet } = require('internal/primordials') → lazy getter fires → makeSafe(Set, ...) runs → Reflect.getOwnPropertyDescriptor(Set.prototype, 'add') returns the tampered add.
  4. state.awaitDrainWriters.add(dest) is now a no-op; back-pressure bookkeeping silently breaks.

Pre-PR, step 1 would have snapshotted SafeSet before step 2 could tamper it.

Why nit, not normal

  • Bun's primary tamper-resistance mechanism is $-intrinsics, not this file — the header explicitly says "TODO: Use native code and JSC intrinsics… Do not use this file for new code".
  • The pre-PR guarantee was already load-order-dependent; this widens an imperfect window rather than opening a new one.
  • The most-used entries (SafeMap, SafeSet, the Map/Set/TypedArray getters) are still eagerly materialized whenever node:util loads, via inspect_globals' top-level destructure — the PR author preserved that on purpose.
  • None of the affected paths (awaitDrainWriters, assert's SafeWeakSet, calltracker's SafeWeakMap) are security boundaries.
  • The PR author explicitly acknowledged the changed guarantee (worker/messaging.ts comment updated from "at bootstrap" to "when this module loads").

Suggested cheap mitigation (optional)

Capture the raw prototypes into module-level consts alongside the iterator inputs already at lines 39-46 — e.g. const MapPrototype = Map.prototype, SetPrototype = Set.prototype, ... — and have the lazy makeSafe/getGetter closures read from those instead of the live Map/Set/WeakMap/WeakSet. That keeps all the deferred work (the ~4× Reflect.ownKeys walk + descriptor copy) lazy while restoring the pre-PR snapshot instant.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Verified the timing: at HEAD nothing loads internal/primordials before user code either (only lazy require sites in on-demand modules; no bootstrap requireId), and a Map.prototype.size tamper on line 1 of a script is already visible to MapPrototypeGetSize on release 1.4.0 — so the snapshot was always post-user-code; this only moves it from first-module-require to first-property-access. Where a module actually depends on capturing at its own load (assert's SafeMap/SafeSet/SafeWeakSet, readline's SafeStringIterator, worker_threads/messaging's SafeMap) the destructure stays at module top level. Left as-is.

super(i);
}
},
),
SafePromiseAll,
SafePromiseAllReturnArrayLike,
SafePromiseAllReturnVoid,
SafeSet: makeSafe(
);
defineLazy("SafeSet", () =>
makeSafe(
Set,
class SafeSet extends Set {
constructor(i) {
super(i);
}
},
),
SafeWeakSet: makeSafe(
);
defineLazy("SafeWeakSet", () =>
makeSafe(
WeakSet,
class SafeWeakSet extends WeakSet {
constructor(i) {
super(i);
}
},
),
SafeWeakMap: makeSafe(
);
defineLazy("SafeWeakMap", () =>
makeSafe(
WeakMap,
class SafeWeakMap extends WeakMap {
constructor(i) {
super(i);
}
},
),
SetPrototypeGetSize: getGetter(Set, "size"),
String,
TypedArrayPrototypeGetLength: getGetter(Uint8Array, "length"),
TypedArrayPrototypeGetSymbolToStringTag: getGetter(Uint8Array, Symbol.toStringTag),
Uint8ClampedArray,
Uint8Array,
Uint16Array,
Uint32Array,
Int8Array,
Int16Array,
Int32Array,
Float16Array,
Float32Array,
Float64Array,
BigUint64Array,
BigInt64Array,
uncurryThis,
};
);

export default primordials;
Loading
Loading