Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
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
49 changes: 39 additions & 10 deletions src/jsc/JSModuleLoader.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use core::ptr::NonNull;

use crate::{JSGlobalObject, JSInternalPromise, JsError, JsResult};
use bun_core::String as BunString;

Expand Down Expand Up @@ -28,33 +30,60 @@ impl JSModuleLoader {
/// Raw-pointer variant of `load_and_evaluate_module`. Returns the FFI
/// `*mut JSInternalPromise` directly so callers that need to store or pass
/// a mutable cell pointer don't launder provenance through `&T -> *mut T`.
///
/// Every load failure comes back as a rejected promise (see
/// [`Self::reject_with_thrown_exception`]); `None` only while the VM is
/// being terminated, with the termination exception left pending.

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.

If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

pub fn load_and_evaluate_module_ptr(
global_object: *mut JSGlobalObject,
module_name: Option<&BunString>,
) -> Option<core::ptr::NonNull<JSInternalPromise>> {
) -> Option<NonNull<JSInternalPromise>> {
// `JSGlobalObject` is an opaque ZST handle; `opaque_ref` is the
// centralised zero-byte deref proof (panics on null).
core::ptr::NonNull::new(JSC__JSModuleLoader__loadAndEvaluateModule(
JSGlobalObject::opaque_ref(global_object),
let global = JSGlobalObject::opaque_ref(global_object);
NonNull::new(JSC__JSModuleLoader__loadAndEvaluateModule(
global,
module_name,
))
.or_else(|| Self::reject_with_thrown_exception(global))
}

/// Raw-pointer variant of `Self::import`. Returns the FFI
/// `*mut JSInternalPromise` directly so callers that need to store or pass
/// a mutable cell pointer (e.g. `VirtualMachine::pending_internal_promise`)
/// don't launder provenance through `&T -> *mut T`. Mirrors
/// [`Self::load_and_evaluate_module_ptr`].
/// [`Self::load_and_evaluate_module_ptr`], including how failures are
/// reported: `Err` only while the VM is being terminated.

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.

If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

pub fn import_ptr(
global_object: *mut JSGlobalObject,
module_name: &BunString,
) -> JsResult<core::ptr::NonNull<JSInternalPromise>> {
) -> JsResult<NonNull<JSInternalPromise>> {
// `JSGlobalObject` is an opaque ZST handle; `opaque_ref` is the
// centralised zero-byte deref proof (panics on null).
core::ptr::NonNull::new(JSModuleLoader__import(
JSGlobalObject::opaque_ref(global_object),
module_name,
))
.ok_or(JsError::Thrown)
let global = JSGlobalObject::opaque_ref(global_object);
NonNull::new(JSModuleLoader__import(global, module_name))
.or_else(|| Self::reject_with_thrown_exception(global))
.ok_or(JsError::Thrown)
}

/// JSC resolves the specifier before it has a promise to reject
/// (Completion.cpp `loadAndEvaluateModule`, JSModuleLoader.cpp
/// `requestImportModule`), so an unresolvable one is thrown and the binding
/// returns null. One way to get there is a path whose bytes are not valid
/// UTF-8: once it is a JS string it no longer names the file. The callers
/// report load failures from the promise, so deliver the error that way,
/// marked handled like the loader's own promises so the unhandled
/// rejection tracker does not report it a second time.

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.

If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

fn reject_with_thrown_exception(global: &JSGlobalObject) -> Option<NonNull<JSInternalPromise>> {
let exception = global.try_take_exception()?;
// `try_take_exception` leaves a termination exception pending; so do we.
if exception.is_termination_exception() {
return None;
}
let promise = JSInternalPromise::create(global);
promise
.reject_as_handled(global, exception.to_error().unwrap_or(exception))
.ok()?;
Some(NonNull::from(promise))
}
}
22 changes: 9 additions & 13 deletions src/jsc/ResolveMessage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -186,22 +186,18 @@ impl ResolveMessage {
let _ = write!(&mut out, "Module not found '{}'", BStr::new(specifier));
return out;
}
if bun_resolver::is_package_path(specifier)
let what = if bun_resolver::is_package_path(specifier)
&& !strings::contains_char(specifier, b'/')
{
let _ = write!(
&mut out,
"Cannot find package '{}' from '{}'",
BStr::new(specifier),
BStr::new(referrer),
);
"package"
} else {
let _ = write!(
&mut out,
"Cannot find module '{}' from '{}'",
BStr::new(specifier),
BStr::new(referrer),
);
"module"
};
let _ = write!(&mut out, "Cannot find {what} '{}'", BStr::new(specifier));
// Entry points and preloads are loaded directly, not imported
// from anywhere; Node words those the same way.

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.

If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

if !referrer.is_empty() {
let _ = write!(&mut out, " from '{}'", BStr::new(referrer));
}
return out;
}
Expand Down
16 changes: 16 additions & 0 deletions src/runtime/cli/test_command.rs
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,23 @@ mod bun_test {
}
}

/// The report declares `encoding="UTF-8"`, but file paths, the hostname and the
/// CI env vars are raw OS bytes: ill-formed sequences become U+FFFD, as the
/// console already prints them (`bstr::BStr`).

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.

If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

pub(crate) fn escape_xml(str_: &[u8], writer: &mut impl bun_io::Write) -> crate::Result<()> {
if strings::is_valid_utf8(str_) {
return escape_xml_utf8(str_, writer);
}
for chunk in str_.utf8_chunks() {
escape_xml_utf8(chunk.valid().as_bytes(), writer)?;
if !chunk.invalid().is_empty() {
writer.write_all("\u{FFFD}".as_bytes())?;
}
}
Ok(())
}

fn escape_xml_utf8(str_: &[u8], writer: &mut impl bun_io::Write) -> crate::Result<()> {
let mut last: usize = 0;
let mut i: usize = 0;
let len = str_.len();
Expand Down
6 changes: 4 additions & 2 deletions src/runtime/jsc_hooks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -712,8 +712,10 @@ fn generate_entry_point(_vm: &VirtualMachine, watch: bool, entry_path: &[u8]) ->
/// preload promise if any, else null.
///
/// Error mapping: resolver `Failure` returns the resolver error,
/// `Pending`/`NotFound` returns `error.ModuleNotFound`,
/// `JSModuleLoader.import` throwing returns `error.JSError`.
/// `Pending`/`NotFound` returns `error.ModuleNotFound`. A preload the module
/// loader cannot load comes back as a rejected promise like any other failing
/// preload; `JSModuleLoader.import` only fails (`error.JSError`) when the VM is
/// being terminated.

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.

If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

///
/// # Safety
/// `vm` is the live per-thread VM.
Expand Down
31 changes: 29 additions & 2 deletions test/cli/run/preload-test.test.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { spawnSync } from "bun";
import { describe, expect, test } from "bun:test";
import { mkdirSync, realpathSync } from "fs";
import { bunEnv, bunExe } from "harness";
import { mkdirSync, realpathSync, writeFileSync } from "fs";
import { bunEnv, bunExe, canCreateNonUtf8FileNames, tempDir } from "harness";
import { tmpdir } from "os";
import { join } from "path";
const preloadModule = `
Expand Down Expand Up @@ -210,4 +210,31 @@ plugin({
expect(exitCode).toBe(1);
}
});

test.skipIf(!canCreateNonUtf8FileNames())(
"throws an error when the preloaded module's path is not valid UTF-8",
async () => {
// The resolver finds the file, but its path does not survive becoming a
// module specifier, so loading it fails; that failure has to be reported.
using dir = tempDir("bun-preload-non-utf8", { "main.js": `console.log("RAN main");` });
writeFileSync(
Buffer.concat([Buffer.from(`${dir}/p`), Buffer.from([0xff]), Buffer.from(".js")]),
`console.log("RAN preload");`,
);

await using proc = Bun.spawn({
// A JS string cannot carry the raw byte into argv; the shell can.
cmd: ["sh", "-c", `exec "$0" --preload "./$(printf 'p\\377.js')" main.js`, bunExe()],
cwd: String(dir),
stderr: "pipe",
stdout: "pipe",
env: bunEnv,
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);

expect(stdout).toBe("");
expect(stderr).toMatch(/error: Cannot find module '[^']*\/p\uFFFD\.js'\n/);
expect(exitCode).toBe(1);
},
);
});
74 changes: 73 additions & 1 deletion test/cli/test/bun-test.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { spawnSync } from "bun";
import { beforeAll, describe, expect, it, test } from "bun:test";
import { bunEnv, bunExe, tempDir, tempDirWithFiles, tmpdirSync } from "harness";
import { bunEnv, bunExe, canCreateNonUtf8FileNames, tempDir, tempDirWithFiles, tmpdirSync } from "harness";
import { mkdirSync, rmSync, writeFileSync } from "node:fs";
import { dirname, join, resolve } from "node:path";

Expand Down Expand Up @@ -1749,3 +1749,75 @@ describe.concurrent("test file discovery (scanner)", () => {
expect(exitCode).toBe(0);
});
});

// Such a path does not survive becoming a module specifier, so the file cannot
// be loaded (Node cannot load it either); what is tested here is that this is
// reported as a failed file and the rest of the run still happens. The names
// need Buffer paths, so they cannot be part of the tempDir tree.
describe.concurrent.skipIf(!canCreateNonUtf8FileNames())("files whose path is not valid UTF-8", () => {
const invalidByte = Buffer.from([0xff]);

test("a scanned test file that cannot be loaded fails without stopping the run", async () => {
using dir = tempDir("scanner-non-utf8-path", {
"a_first.test.ts": `import { test } from "bun:test"; test("a", () => { console.log("RAN a_first"); });`,
"z_last.test.ts": `import { test } from "bun:test"; test("z", () => { console.log("RAN z_last"); });`,
});
const source = `import { test } from "bun:test"; test("t", () => { console.log("RAN unloadable"); });`;
const root = Buffer.from(String(dir) + "/");
// Once in the file name itself, once in a directory on the way to the file.
writeFileSync(Buffer.concat([root, Buffer.from("b"), invalidByte, Buffer.from(".test.ts")]), source);
const subdir = Buffer.concat([root, Buffer.from("dir"), invalidByte]);
mkdirSync(subdir);
writeFileSync(Buffer.concat([subdir, Buffer.from("/inner.test.ts")]), source);

await using proc = Bun.spawn({
cmd: [bunExe(), "test"],
env: bunEnv,
cwd: String(dir),
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);

expect(stdout).toContain("RAN a_first");
expect(stdout).toContain("RAN z_last");
expect(stdout).not.toContain("RAN unloadable");
// Each unloadable file gets its header and a load error naming it (the
// invalid byte prints as U+FFFD), with no "from" since nothing imported it.
expect(stderr).toContain("\nb\uFFFD.test.ts:\n");
expect(stderr).toMatch(/error: Cannot find module '[^']*\/b\uFFFD\.test\.ts'\n/);
expect(stderr).toContain("\ndir\uFFFD/inner.test.ts:\n");
expect(stderr).toMatch(/error: Cannot find module '[^']*\/dir\uFFFD\/inner\.test\.ts'\n/);
expect(stderr).toContain(" 2 pass");
expect(stderr).toContain(" 2 fail");
expect(stderr).toContain("Ran 4 tests across 4 files.");
expect(exitCode).toBe(1);
});

test("--preload naming a file that cannot be loaded reports the error", async () => {
using dir = tempDir("preload-non-utf8-path", {
"a.test.ts": `import { test } from "bun:test"; test("a", () => { console.log("RAN a"); });`,
});
writeFileSync(
Buffer.concat([Buffer.from(String(dir) + "/p"), invalidByte, Buffer.from(".ts")]),
`console.log("RAN preload");`,
);

await using proc = Bun.spawn({
// A JS string cannot carry the raw byte into argv; the shell can.
cmd: ["sh", "-c", `exec "$0" test --preload "./$(printf 'p\\377.ts')"`, bunExe()],
env: bunEnv,
cwd: String(dir),
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);

expect(stdout).not.toContain("RAN preload");
expect(stdout).not.toContain("RAN a");
expect(stderr).toMatch(/error: Cannot find module '[^']*\/p\uFFFD\.ts'\n/);
expect(stderr).toContain(" 0 pass");
expect(stderr).toContain(" 1 fail");
expect(exitCode).toBe(1);
});
});
57 changes: 56 additions & 1 deletion test/cli/test/parallel.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,16 @@
import { expect, test } from "bun:test";
import { bunEnv, bunExe, isASAN, isDebug, isWindows, normalizeBunSnapshot, tempDir, tls } from "harness";
import {
bunEnv,
bunExe,
canCreateNonUtf8FileNames,
isASAN,
isDebug,
isWindows,
normalizeBunSnapshot,
tempDir,
tls,
} from "harness";
import { writeFileSync } from "node:fs";

test("--parallel: each worker has a unique JEST_WORKER_ID and BUN_TEST_WORKER_ID", async () => {
// Sleep so worker 0 is busy when workers 1/2 come online and pick up the
Expand Down Expand Up @@ -1134,6 +1145,50 @@ test("--parallel --reporter=junit emits a synthetic suite for crashed files", as
expect({ innerTests, innerFail }).toEqual({ innerTests: outerTests, innerFail: outerFail });
});

test.skipIf(!canCreateNonUtf8FileNames())(
"--parallel --reporter=junit writes a crashed file's non-UTF-8 path as U+FFFD",
async () => {
using dir = tempDir("parallel-junit-non-utf8-path", {
// Every worker exits while setting up its first file, so both files end
// up in the synthetic crashed suite, the one place the coordinator writes
// a path itself. (A file with such a path cannot be loaded at all, so it
// never gets a regular suite.)
"exit-preload.js": `process.exit(7);`,
"ok.test.js": `import {test} from "bun:test"; test("ok", () => {});`,
});
writeFileSync(
Buffer.concat([Buffer.from(String(dir) + "/b"), Buffer.from([0xff]), Buffer.from(".test.js")]),
`import {test} from "bun:test"; test("t", () => {});`,
);
const out = String(dir) + "/out.xml";
await using proc = Bun.spawn({
cmd: [
bunExe(),
"test",
"--parallel=2",
"--preload",
"./exit-preload.js",
"--reporter=junit",
`--reporter-outfile=${out}`,
],
env: { ...bunEnv, BUN_TEST_PARALLEL_SCALE_MS: "0" },
cwd: String(dir),
stderr: "pipe",
stdout: "pipe",
});
const [, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect(stderr).toContain("b\uFFFD.test.js (worker crashed: exit code 7)");

// The report declares encoding="UTF-8"; decoding it strictly is the first
// thing any XML parser does with it.
const xml = new TextDecoder("utf-8", { fatal: true }).decode(await Bun.file(out).bytes());
expect(xml).toContain('<testsuite name="b\uFFFD.test.js" file="b\uFFFD.test.js"');
expect(xml).toContain('<testcase name="(worker crashed)" classname="b\uFFFD.test.js">');
expect(xml).toContain('<testsuite name="ok.test.js"');
expect(exitCode).toBe(1);
},
);

test("--parallel: SIGTERM on coordinator kills workers and their grandchildren", async () => {
const grandchild = `
require("fs").appendFileSync(process.env.PIDS, "grandchild=" + process.pid + "\\n");
Expand Down
27 changes: 27 additions & 0 deletions test/harness.ts
Original file line number Diff line number Diff line change
Expand Up @@ -276,6 +276,33 @@ export function canBuildNodeAddons(): boolean {
return canBuildNodeAddonsCached;
}

let canCreateNonUtf8FileNamesCached: boolean | undefined;

/**
* Whether the temp filesystem stores file names that are not valid UTF-8 byte
* for byte. Linux filesystems do, macOS's do not, and Windows names are UTF-16,
* so the situation does not exist there. Such a name can only be created by
* passing the path as a Buffer.
*/
export function canCreateNonUtf8FileNames(): boolean {
if (canCreateNonUtf8FileNamesCached === undefined) {
if (isWindows) {
canCreateNonUtf8FileNamesCached = false;
} else {
const dir = fs.mkdtempSync(join(os.tmpdir(), "bun-non-utf8-name-probe-"));
try {
fs.writeFileSync(Buffer.concat([Buffer.from(dir + "/"), Buffer.from([0xff])]), "");
canCreateNonUtf8FileNamesCached = fs.readdirSync(dir, { encoding: "buffer" }).some(name => name.includes(0xff));
} catch {
canCreateNonUtf8FileNamesCached = false;
} finally {
fs.rmSync(dir, { recursive: true, force: true });
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
}
}
return canCreateNonUtf8FileNamesCached;
}

export function shellExe(): string {
return isWindows ? "pwsh" : "bash";
}
Expand Down
Loading
Loading