diff --git a/src/jsc/JSModuleLoader.rs b/src/jsc/JSModuleLoader.rs index dba3cfb2a96b..d6e231fe5f5e 100644 --- a/src/jsc/JSModuleLoader.rs +++ b/src/jsc/JSModuleLoader.rs @@ -1,3 +1,5 @@ +use core::ptr::NonNull; + use crate::{JSGlobalObject, JSInternalPromise, JsError, JsResult}; use bun_core::String as BunString; @@ -28,33 +30,53 @@ 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`. + /// A failed load is a rejected promise; `None` only on VM termination. pub fn load_and_evaluate_module_ptr( global_object: *mut JSGlobalObject, module_name: Option<&BunString>, - ) -> Option> { + ) -> Option> { // `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`]; `Err` only on VM termination. pub fn import_ptr( global_object: *mut JSGlobalObject, module_name: &BunString, - ) -> JsResult> { + ) -> JsResult> { // `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 that + /// failure alone is thrown, and the binding returns null. Callers report load + /// failures from the promise. Handled, like the loader's own promises: the + /// caller reports it, not the rejection tracker. + fn reject_with_thrown_exception(global: &JSGlobalObject) -> Option> { + let exception = global.try_take_exception()?; + // Still pending (`try_take_exception` does not clear it); leave it to the caller. + 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)) } } diff --git a/src/jsc/ResolveMessage.rs b/src/jsc/ResolveMessage.rs index 594da9ebbbcd..306fe01b2a68 100644 --- a/src/jsc/ResolveMessage.rs +++ b/src/jsc/ResolveMessage.rs @@ -186,22 +186,17 @@ 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)); + // Empty for an entry point or preload; Node omits it there too. + if !referrer.is_empty() { + let _ = write!(&mut out, " from '{}'", BStr::new(referrer)); } return out; } diff --git a/src/jsc/bindings/ZigGlobalObject.cpp b/src/jsc/bindings/ZigGlobalObject.cpp index 3022c09cafe3..9cee353bccf6 100644 --- a/src/jsc/bindings/ZigGlobalObject.cpp +++ b/src/jsc/bindings/ZigGlobalObject.cpp @@ -3609,7 +3609,7 @@ JSC::Identifier GlobalObject::moduleLoaderResolve(JSGlobalObject* jsGlobalObject Zig::GlobalObject* globalObject = static_cast(jsGlobalObject); ErrorableString res; - res.success = false; + memset(&res, 0, sizeof(res)); BunString keyZ; if (key.isString()) { @@ -3682,7 +3682,9 @@ JSC::Identifier GlobalObject::moduleLoaderResolve(JSGlobalObject* jsGlobalObject return result; } else { auto scope = DECLARE_THROW_SCOPE(globalObject->vm()); - throwException(scope, res.result.err, globalObject); + // `res` is not written when the resolver threw (e.g. a plugin's onResolve); keep that exception. + if (!scope.exception()) + throwException(scope, res.result.err, globalObject); return globalObject->vm().propertyNames->emptyIdentifier; } } diff --git a/src/runtime/cli/test_command.rs b/src/runtime/cli/test_command.rs index 7ea42e2be5e9..c7d99eca4e84 100644 --- a/src/runtime/cli/test_command.rs +++ b/src/runtime/cli/test_command.rs @@ -141,7 +141,22 @@ mod bun_test { } } +/// Paths, the hostname and CI env vars are raw OS bytes; the report is declared +/// UTF-8, so ill-formed sequences become U+FFFD as on the console (`bstr::BStr`). 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(); diff --git a/src/runtime/jsc_hooks.rs b/src/runtime/jsc_hooks.rs index cad7564a5787..37f8345657d7 100644 --- a/src/runtime/jsc_hooks.rs +++ b/src/runtime/jsc_hooks.rs @@ -712,8 +712,8 @@ 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 load the module loader +/// refuses is a rejected promise, `error.JSError` means VM termination. /// /// # Safety /// `vm` is the live per-thread VM. diff --git a/test/cli/run/preload-test.test.js b/test/cli/run/preload-test.test.js index 72f9ddd9538a..a1451e193004 100644 --- a/test/cli/run/preload-test.test.js +++ b/test/cli/run/preload-test.test.js @@ -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 { bunEnv, bunExe, tempDir } from "harness"; import { tmpdir } from "os"; import { join } from "path"; const preloadModule = ` @@ -210,4 +210,55 @@ plugin({ expect(exitCode).toBe(1); } }); + + async function run(dir, ...args) { + await using proc = Bun.spawn({ + cmd: [bunExe(), ...args], + cwd: String(dir), + stderr: "pipe", + stdout: "pipe", + env: bunEnv, + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + return { stdout, stderr, exitCode }; + } + + // A preload is handed to the module loader as the specifier itself; node: + // names skip the file resolver, so this is the loader's own refusal, which + // used to be reported as nothing more than "Error occurred loading entry point: JSError". + test("reports a preloaded node: module that does not exist", async () => { + using dir = tempDir("bun-preload-missing-builtin", { "main.js": `console.log("RAN main");` }); + const result = await run(dir, "--preload", "node:does_not_exist", "main.js"); + + expect(result).toEqual({ + stdout: "", + stderr: expect.stringContaining("error: No such built-in module: node:does_not_exist\n"), + exitCode: 1, + }); + }); + + // Resolving the entry point runs the plugin; its exception used to be + // replaced by an uninitialized one, which crashed the process. + test("reports the error thrown by a preloaded plugin's onResolve for the entry point", async () => { + using dir = tempDir("bun-preload-plugin-throws", { + "plugin.js": ` + Bun.plugin({ + name: "refuse", + setup(build) { + build.onResolve({ filter: /main\\.js$/ }, () => { + throw new Error("refused by onResolve"); + }); + }, + }); + `, + "main.js": `console.log("RAN main");`, + }); + const result = await run(dir, "--preload", "./plugin.js", "./main.js"); + + expect(result).toEqual({ + stdout: "", + stderr: expect.stringContaining("error: refused by onResolve\n"), + exitCode: 1, + }); + }); }); diff --git a/test/cli/test/bun-test.test.ts b/test/cli/test/bun-test.test.ts index 4bc66e446ba0..286686b056da 100644 --- a/test/cli/test/bun-test.test.ts +++ b/test/cli/test/bun-test.test.ts @@ -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, isWindows, tempDir, tempDirWithFiles, tmpdirSync } from "harness"; import { mkdirSync, rmSync, writeFileSync } from "node:fs"; import { dirname, join, resolve } from "node:path"; @@ -1749,3 +1749,119 @@ describe.concurrent("test file discovery (scanner)", () => { expect(exitCode).toBe(0); }); }); + +// A scanned file, and each --preload, is handed to the module loader as the +// specifier itself. When that specifier does not resolve, the failure used to +// end the whole run: the file's header, then exit 1 with nothing else printed. +// Each case below is one way to make such a specifier; what is asserted is that +// the failure is reported under the file and the other files still run. +describe.concurrent("test files and preloads the module loader cannot resolve", () => { + const passing = (name: string) => + `import { test } from "bun:test"; test("t", () => { console.log("RAN ${name}"); });`; + + async function runBunTest(dir: string, ...args: string[]) { + await using proc = Bun.spawn({ + cmd: [bunExe(), "test", ...args], + env: bunEnv, + cwd: dir, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + return { stdout, stderr, exitCode }; + } + + test("a preload plugin whose onResolve throws for a file fails that file with the thrown error", async () => { + using dir = tempDir("unresolvable-plugin-throw", { + "plugin.ts": ` + Bun.plugin({ + name: "refuse", + setup(build) { + build.onResolve({ filter: /refused\\.test\\.ts$/ }, () => { + throw new Error("refused by onResolve"); + }); + }, + }); + `, + "a_first.test.ts": passing("a_first"), + "refused.test.ts": passing("refused"), + "z_last.test.ts": passing("z_last"), + }); + const { stdout, stderr, exitCode } = await runBunTest(String(dir), "--preload", "./plugin.ts"); + + expect(stdout).toContain("RAN a_first"); + expect(stdout).toContain("RAN z_last"); + expect(stdout).not.toContain("RAN refused"); + expect(stderr).toContain("\nrefused.test.ts:\n"); + expect(stderr).toContain("error: refused by onResolve\n"); + expect(stderr).toContain(" 2 pass"); + expect(stderr).toContain(" 1 fail"); + expect(stderr).toMatch(/\bacross 3 files\./); + expect(exitCode).toBe(1); + }); + + // '?' is not a legal file name character on Windows. Elsewhere the loader + // reads it as the start of a query string, so the file itself is not found. + test.skipIf(isWindows)("a file whose name contains '?' fails without stopping the run", async () => { + using dir = tempDir("unresolvable-question-mark", { + "a_first.test.ts": passing("a_first"), + "what?.test.ts": passing("what"), + "z_last.test.ts": passing("z_last"), + }); + const { stdout, stderr, exitCode } = await runBunTest(String(dir)); + + expect(stdout).toContain("RAN a_first"); + expect(stdout).toContain("RAN z_last"); + expect(stdout).not.toContain("RAN what"); + expect(stderr).toContain("\nwhat?.test.ts:\n"); + // Named as the entry point it was loaded as: no "from", nothing imported it. + expect(stderr).toMatch(/error: Cannot find module '[^']*\/what\?\.test\.ts'\n/); + expect(stderr).toContain(" 2 pass"); + expect(stderr).toContain(" 1 fail"); + expect(stderr).toMatch(/\bacross 3 files\./); + expect(exitCode).toBe(1); + }); + + // Such a name only survives as raw bytes; as a specifier it holds U+FFFD and + // no longer names the file (Node cannot load it either). Buffer paths are the + // only way to create one, so these files cannot be part of the tempDir tree. + test.skipIf(!canCreateNonUtf8FileNames())("file and directory names that are not valid UTF-8", async () => { + using dir = tempDir("unresolvable-non-utf8", { + "a_first.test.ts": passing("a_first"), + "z_last.test.ts": passing("z_last"), + }); + const invalidByte = Buffer.from([0xff]); + const root = Buffer.from(String(dir) + "/"); + writeFileSync(Buffer.concat([root, Buffer.from("b"), invalidByte, Buffer.from(".test.ts")]), passing("unloadable")); + const subdir = Buffer.concat([root, Buffer.from("dir"), invalidByte]); + mkdirSync(subdir); + writeFileSync(Buffer.concat([subdir, Buffer.from("/inner.test.ts")]), passing("unloadable")); + const { stdout, stderr, exitCode } = await runBunTest(String(dir)); + + expect(stdout).toContain("RAN a_first"); + expect(stdout).toContain("RAN z_last"); + expect(stdout).not.toContain("RAN unloadable"); + 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).toMatch(/\bacross 4 files\./); + expect(exitCode).toBe(1); + }); + + // node: specifiers skip the file resolver, so the module loader is the first + // thing that rejects this one. + test("--preload of a node: module that does not exist reports it", async () => { + using dir = tempDir("unresolvable-node-preload", { "a.test.ts": passing("a") }); + const { stdout, stderr, exitCode } = await runBunTest(String(dir), "--preload", "node:does_not_exist"); + + expect(stdout).not.toContain("RAN a"); + expect(stderr).toContain("\na.test.ts:\n"); + expect(stderr).toContain("error: No such built-in module: node:does_not_exist\n"); + expect(stderr).toContain(" 0 pass"); + expect(stderr).toContain(" 1 fail"); + expect(exitCode).toBe(1); + }); +}); diff --git a/test/cli/test/parallel.test.ts b/test/cli/test/parallel.test.ts index 39d0cd082d2f..916942d23ce5 100644 --- a/test/cli/test/parallel.test.ts +++ b/test/cli/test/parallel.test.ts @@ -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 @@ -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(''); + expect(xml).toContain(' { const grandchild = ` require("fs").appendFileSync(process.env.PIDS, "grandchild=" + process.pid + "\\n"); diff --git a/test/harness.ts b/test/harness.ts index a6c07623ec17..c891d92e450b 100644 --- a/test/harness.ts +++ b/test/harness.ts @@ -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 = tmpdirSync("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 }); + } + } + } + return canCreateNonUtf8FileNamesCached; +} + export function shellExe(): string { return isWindows ? "pwsh" : "bash"; } diff --git a/test/js/junit-reporter/junit.test.js b/test/js/junit-reporter/junit.test.js index d5a2ebe87d8b..7272b1fcb224 100644 --- a/test/js/junit-reporter/junit.test.js +++ b/test/js/junit-reporter/junit.test.js @@ -1,6 +1,6 @@ import { file, spawn } from "bun"; import { describe, expect, it } from "bun:test"; -import { bunEnv, bunExe, tempDir } from "harness"; +import { bunEnv, bunExe, isWindows, tempDir } from "harness"; import { join } from "node:path"; const xml2js = require("xml2js"); @@ -389,6 +389,46 @@ describe("junit reporter", () => { expect(proc.exitCode).toBe(1); }); + it.skipIf(isWindows)("produces well-formed XML when a value taken from the OS is not valid UTF-8", async () => { + await using tmpDir = tempDir("junit-non-utf8", { + "package.json": "{}", + "ok.test.js": 'import { test } from "bun:test";\ntest("ok", () => {});\n', + }); + + const junitPath = join(tmpDir, "junit.xml"); + // The commit property is copied from the environment as raw bytes, like + // file paths and the hostname are. A JS string cannot hold such a byte, so + // the shell puts it into the variable (on Windows the environment is + // UTF-16, so the situation does not exist there). + await using proc = spawn( + [ + "sh", + "-c", + `GITHUB_SHA="$(printf 'abc\\377')" exec "$0" test --reporter=junit --reporter-outfile "$1"`, + bunExe(), + junitPath, + ], + { + cwd: tmpDir, + env: { ...bunEnv, BUN_DEBUG_QUIET_LOGS: "1" }, + stdout: "pipe", + stderr: "pipe", + }, + ); + const [, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stderr).toContain(" 1 pass"); + + // The report declares encoding="UTF-8", so it has to decode as such; the + // invalid byte is written as U+FFFD, as the console already prints it. + const xmlContent = new TextDecoder("utf-8", { fatal: true }).decode(await file(junitPath).bytes()); + const result = await new Promise((resolve, reject) => { + xml2js.parseString(xmlContent, { strict: true }, (err, r) => (err ? reject(err) : resolve(r))); + }); + const properties = result.testsuites.testsuite[0].properties[0].property; + expect(properties.find(p => p.$.name === "commit").$.value).toBe("abc\uFFFD"); + expect(exitCode).toBe(0); + }); + it("produces well-formed XML when test names contain control characters", async () => { await using tmpDir = tempDir("junit-ctrl", { "package.json": "{}",