Skip to content
Closed
Show file tree
Hide file tree
Changes from 2 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
11 changes: 7 additions & 4 deletions src/runtime/bake/BakeGlobalObject.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@
#include "JavaScriptCore/Completion.h"
#include "JavaScriptCore/JSSourceCode.h"

// These (and BakeProdLoad below) return a BunString whose reference the caller
// owns: consume it with transferToWTFString(), which releases it. toWTFString()
// takes a second reference and leaks the one we were handed.
Comment thread
robobun marked this conversation as resolved.
Outdated
extern "C" BunString BakeProdResolve(JSC::JSGlobalObject*, BunString a, BunString b);
extern "C" BunString BakeToWindowsPath(BunString a);

Expand Down Expand Up @@ -45,7 +48,7 @@ bakeModuleLoaderImportModule(JSC::JSGlobalObject* global,
BunString result = BakeProdResolve(global, Bun::toString(refererString), Bun::toString(keyString));
RETURN_IF_EXCEPTION(scope, nullptr);

return JSC::importModule(global, JSC::Identifier::fromString(vm, result.toWTFString()),
return JSC::importModule(global, JSC::Identifier::fromString(vm, result.transferToWTFString()),
JSC::Identifier(), WTF::move(parameters), nullptr);
}

Expand All @@ -71,7 +74,7 @@ JSC::Identifier bakeModuleLoaderResolve(JSC::JSGlobalObject* jsGlobal,
BunString result = BakeProdResolve(global, Bun::toString(referrer.getString(global)), Bun::toString(keyString));
RETURN_IF_EXCEPTION(scope, vm.propertyNames->emptyIdentifier);

return JSC::Identifier::fromString(vm, result.toWTFString(BunString::ZeroCopy));
return JSC::Identifier::fromString(vm, result.transferToWTFString());
}
}

Expand Down Expand Up @@ -137,7 +140,7 @@ JSC::JSPromise* bakeModuleLoaderFetch(JSC::JSGlobalObject* globalObject,
JSC::SourceOrigin origin = JSC::SourceOrigin(WTF::URL(moduleKey));
JSC::SourceCode sourceCode = JSC::SourceCode(Bake::SourceProvider::create(
globalObject,
source.toWTFString(),
source.transferToWTFString(),
origin,
WTF::move(moduleKey),
WTF::TextPosition(),
Expand All @@ -159,7 +162,7 @@ JSC::JSPromise* bakeModuleLoaderFetch(JSC::JSGlobalObject* globalObject,
// it, because `moduleLoaderFetch(...)` may read the path from disk
// and so we need to give a Windows path to it.
auto temp = BakeToWindowsPath(Bun::toString(bakePrefixRemoved));
bakePrefixRemoved = temp.toWTFString();
bakePrefixRemoved = temp.transferToWTFString();
#endif
JSString* bakePrefixRemovedString = jsNontrivialString(vm, bakePrefixRemoved);
JSValue bakePrefixRemovedJsvalue = bakePrefixRemovedString;
Expand Down
6 changes: 4 additions & 2 deletions src/runtime/bake/BakeSourceProvider.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,8 @@ extern "C" JSC::JSPromise* BakeLoadModuleByKey(GlobalObject* global, JSC::JSStri
return JSC::loadAndEvaluateModule(global, key->getString(global), nullptr, nullptr);
}

// Both HMR patch entry points own `source` (DevServer.rs hands over a fresh copy)
// and release it through transferToWTFString().
Comment thread
robobun marked this conversation as resolved.
Outdated
extern "C" JSC::EncodedJSValue BakeLoadServerHmrPatch(GlobalObject* global, BunString source) {
JSC::VM&vm = global->vm();
auto scope = DECLARE_THROW_SCOPE(vm);
Expand All @@ -66,7 +68,7 @@ extern "C" JSC::EncodedJSValue BakeLoadServerHmrPatch(GlobalObject* global, BunS
JSC::SourceOrigin origin = JSC::SourceOrigin(WTF::URL(string));
JSC::SourceCode sourceCode = JSC::SourceCode(SourceProvider::create(
global,
source.toWTFString(),
source.transferToWTFString(),
origin,
WTF::move(string),
WTF::TextPosition(),
Expand All @@ -90,7 +92,7 @@ extern "C" JSC::EncodedJSValue BakeLoadServerHmrPatchWithSourceMap(GlobalObject*
// Use DevServerSourceProvider with the source map JSON
auto provider = DevServerSourceProvider::create(
global,
source.toWTFString(),
source.transferToWTFString(),
sourceMapJSONPtr,
sourceMapJSONLength,
origin,
Expand Down
3 changes: 3 additions & 0 deletions src/runtime/bake/DevServer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5560,6 +5560,8 @@ impl DevServer {
mod c {
use super::*;

/// `code` must carry its own reference (`clone_utf8`/`clone_latin1`); the
/// C++ side releases it.
Comment thread
robobun marked this conversation as resolved.
Outdated
pub(super) fn bake_load_server_hmr_patch(
global: &JSGlobalObject,
code: BunString,
Expand All @@ -5570,6 +5572,7 @@ mod c {
jsc::from_js_host_call(global, || BakeLoadServerHmrPatch(global, code))
}

/// Same ownership of `code` as `bake_load_server_hmr_patch`.
pub(super) fn bake_load_server_hmr_patch_with_source_map(
global: &JSGlobalObject,
code: BunString,
Expand Down
46 changes: 24 additions & 22 deletions src/runtime/bake/production.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ use bun_bundler::options::{self as bundler_options, OutputFile, SourceMapOption}
use bun_bundler::output_file::Index as OutputFileIndex;

use bun_collections::{AutoBitSet, StringArrayHashMap};
use bun_core::String as BunString;
use bun_core::{Global, Output};
use bun_core::{OwnedString, String as BunString};
use bun_dotenv as dotenv;
use bun_jsc::js_promise::{UnwrapMode, Unwrapped};
use bun_jsc::virtual_machine::VirtualMachine;
Expand Down Expand Up @@ -332,8 +332,9 @@ fn build_with_vm(ctx: Context, cwd: &[u8], pt: &mut PerThread) -> crate::Result<
}
};

let config_entry_point_string =
BunString::clone_utf8(config_entry_point.path_const().unwrap().text);
let config_entry_point_string = OwnedString::new(BunString::clone_utf8(
config_entry_point.path_const().unwrap().text,
));

let Some(config_promise) =
JSModuleLoader::load_and_evaluate_module_ptr(vm.global, Some(&config_entry_point_string))
Expand Down Expand Up @@ -657,7 +658,9 @@ fn build_with_vm(ctx: Context, cwd: &[u8], pt: &mut PerThread) -> crate::Result<
// Client files go to disk.
// Server files get loaded in memory.
// Populate indexes in `entry_points` to be looked up during prerendering
let mut module_keys: Vec<BunString> = vec![BunString::dead(); entry_points.files.count()];
let mut module_keys: Vec<OwnedString> = (0..entry_points.files.count())
.map(|_| OwnedString::new(BunString::dead()))
.collect();
let mut output_module_map: StringArrayHashMap<OutputFileIndex> = StringArrayHashMap::default();
let mut source_maps: StringArrayHashMap<OutputFileIndex> = StringArrayHashMap::default();
{
Expand Down Expand Up @@ -748,7 +751,7 @@ fn build_with_vm(ctx: Context, cwd: &[u8], pt: &mut PerThread) -> crate::Result<
BStr::new(without_prefix)
));
str.to_thread_safe();
module_keys[entry_point_index as usize] = str;
module_keys[entry_point_index as usize] = OwnedString::new(str);
}
}

Expand Down Expand Up @@ -825,15 +828,14 @@ fn build_with_vm(ctx: Context, cwd: &[u8], pt: &mut PerThread) -> crate::Result<

for (i, router_type) in router.types.iter().enumerate() {
if let Some(client_file) = router_type.client_file {
let str = BunString::create_format(format_args!(
let mut url = BunString::create_format(format_args!(
"{}{}",
BStr::new(public_path),
BStr::new(&pt.output_file(client_file).dest_path),
))
.to_js(global)
.map_err(js_err)?;
));
let url = jsc::bun_string_jsc::transfer_to_js(&mut url, global).map_err(js_err)?;
client_entry_urls
.put_index(global, u32::try_from(i).expect("int cast"), str)
.put_index(global, u32::try_from(i).expect("int cast"), url)
.map_err(js_err)?;
} else {
client_entry_urls
Expand Down Expand Up @@ -928,15 +930,15 @@ fn build_with_vm(ctx: Context, cwd: &[u8], pt: &mut PerThread) -> crate::Result<
debug_assert!(output_file.dest_path[0] != b'.');
// CSS chunks must be in contiguous order!!
debug_assert!(output_file.loader.is_css());
let mut url = BunString::create_format(format_args!(
"{}{}",
BStr::new(public_path),
BStr::new(&output_file.dest_path),
));
css_chunk_js_strings.push(
BunString::create_format(format_args!(
"{}{}",
BStr::new(public_path),
BStr::new(&output_file.dest_path),
))
.to_js(global)
.map_err(js_err)?
.protected(),
jsc::bun_string_jsc::transfer_to_js(&mut url, global)
.map_err(js_err)?
.protected(),
);
}

Expand Down Expand Up @@ -1104,12 +1106,12 @@ fn build_with_vm(ctx: Context, cwd: &[u8], pt: &mut PerThread) -> crate::Result<
}

// Init the items
let pattern_string = BunString::clone_utf8(pattern.slice());
let mut pattern_string = BunString::clone_utf8(pattern.slice());
route_patterns
.put_index(
global,
u32::try_from(nav_index).expect("int cast"),
pattern_string.to_js(global).map_err(js_err)?,
jsc::bun_string_jsc::transfer_to_js(&mut pattern_string, global).map_err(js_err)?,
)
.map_err(js_err)?;

Expand Down Expand Up @@ -1448,7 +1450,7 @@ pub struct PerThread {
pub(crate) entry_points: EntryPointMap,
pub(crate) bundled_outputs: Vec<OutputFile>,
/// Indexed by entry point index (OpaqueFileId)
pub(crate) module_keys: Vec<BunString>,
pub(crate) module_keys: Vec<OwnedString>,
/// Unordered
pub(crate) module_map: StringArrayHashMap<OutputFileIndex>,
pub(crate) source_maps: StringArrayHashMap<OutputFileIndex>,
Expand Down Expand Up @@ -1513,7 +1515,7 @@ impl PerThread {
vm: *mut VirtualMachine,
entry_points: EntryPointMap,
bundled_outputs: Vec<OutputFile>,
module_keys: Vec<BunString>,
module_keys: Vec<OwnedString>,
module_map: StringArrayHashMap<OutputFileIndex>,
source_maps: StringArrayHashMap<OutputFileIndex>,
) -> crate::Result<PerThread> {
Expand Down
131 changes: 130 additions & 1 deletion test/bake/dev/production.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { describe, expect, test } from "bun:test";
import { existsSync } from "fs";
import { bunEnv, bunExe } from "harness";
import { bunEnv, bunExe, isASAN, isWindows } from "harness";
import path from "path";
import { tempDirWithBakeDeps } from "../bake-harness";

Expand Down Expand Up @@ -594,4 +594,133 @@ export default function IndexPage() {
// Verify NO JavaScript imports are included in the HTML
expect(htmlContent).not.toContain('<script type="module"');
});

// Every BunString production.rs creates (module keys from BakeProdResolve,
// chunk sources from BakeProdLoad, the config path, route patterns, client
// entry URLs) carries a reference that exactly one consumer has to release:
// transferToWTFString() in BakeGlobalObject.cpp, transfer_to_js()/OwnedString
// in production.rs. The build used to read them with toWTFString()/to_js(),
// which take a reference of their own and leave that one behind, so every
// import edge resolved and every chunk loaded while prerendering leaked its
// string. Only LeakSanitizer can see that, and only with Malloc=1: WTF strings
// otherwise live in bmalloc, which LSan does not track.
describe.skipIf(!isASAN || isWindows)("strings created for the build are released", () => {
// Both pages statically import one shared chunk and dynamically import
// another, so the same "bake:/..." keys are resolved more than once through
// bakeModuleLoaderResolve (static imports) and bakeModuleLoaderImportModule
// (import()). Only a repeat resolution leaks a string LSan can report: the
// first one's string becomes the interned module key, which the atom table
// still points at.
const app = (aboutPageBody: string) => ({
"src/index.tsx": `export default { app: { framework: "react" } };`,
"components/Shared.tsx": `export function Shared({ page }: { page: string }) {
return <p>{"shared from " + page}</p>;
}`,
"components/lazy.ts": `export const lazy = "lazy";`,
"pages/index.tsx": `import { Shared } from "../components/Shared";

export default async function IndexPage() {
const { lazy } = await import("../components/lazy");
return <div>{"index " + lazy}<Shared page="index" /></div>;
}`,
"pages/about.tsx": `import { Shared } from "../components/Shared";

export default async function AboutPage() {
const { lazy } = await import("../components/lazy");
${aboutPageBody}
}`,
});

async function buildUnderLeakSanitizer(dir: string, env: Record<string, string> = {}): Promise<string> {
const { stderr } = await Bun.$`${bunExe()} build --app ./src/index.tsx`
.cwd(dir)
.env({
...bunEnv,
...env,
Malloc: "1",
ASAN_OPTIONS: "allow_user_segv_handler=1:disable_coredump=0:detect_leaks=1",
// With Malloc=1 about 20 bmalloc frames sit between malloc and the
// code that created the string; keep that code in view. log_threads
// makes the leak check announce itself even when it finds nothing.
LSAN_OPTIONS: "malloc_context_size=40:log_threads=1",
})
.quiet()
.throws(false);
const output = stderr.toString();
expect(output).toContain("Processing thread");
return output;
}

// LSan prints one record per allocation site: "Direct leak of N byte(s) in
// M object(s) allocated from:" followed by "#k 0x... in <function> <file>"
// frames. `bun build --app` still leaks unrelated process-lifetime bundler
// state, so neither the report as a whole nor the exit code LSan forces
// means anything here. Every BunString is created through bun_core::String
// and the BunString__* exports of BunString.cpp, so the records with those
// on the stack are exactly the strings this build created and never
// released. Each is reduced to the two frames above the string machinery,
// so a failure reads as "who created it".
const stringMachinery = /\bBunString__\w+|bun_core::string::/;
function leakedBunStrings(stderr: string): string[] {
return stderr
.split(/^(?=(?:Direct|Indirect) leak of )/m)
.filter(record => stringMachinery.test(record))
.map(record => {
const [header, ...lines] = record.split("\n");
const frames = lines.map(line => line.trim().replace(/^#\d+ 0x[0-9a-f]+ in /, ""));
const created = frames.findIndex(frame => stringMachinery.test(frame));
const creators = frames
.slice(created)
.filter(frame => !stringMachinery.test(frame))
.slice(0, 2)
.map(frame => frame.replace(/\(.*\) /, "() ").replace(/ \S*\/src\//, " src/"));
return [header, ...creators].join("\n");
});
}

test.concurrent(
"after a successful build",
async () => {
const dir = await tempDirWithBakeDeps(
"bake-production-string-leaks",
app(`return <div>{"about " + lazy}<Shared page="about" /></div>;`),
);

const stderr = await buildUnderLeakSanitizer(dir);

// Both pages rendered, so every import above was resolved and loaded.
const indexHtml = await Bun.file(path.join(dir, "dist", "index.html")).text();
const aboutHtml = await Bun.file(path.join(dir, "dist", "about", "index.html")).text();
expect(indexHtml).toContain("<div>index lazy<p>shared from index</p></div>");
expect(aboutHtml).toContain("<div>about lazy<p>shared from about</p></div>");
expect(leakedBunStrings(stderr)).toEqual([]);
},
// LSan symbolizes every record through llvm-symbolizer, which is slow
// against the debug binary.
60_000,
);

// A successful build exits without tearing its VM down, and the VM's module
// registry and JS strings keep most of these strings reachable (the chunk
// sources BakeProdLoad returns, the config path, the client entry URL), so
// a leaked reference to them goes unreported above. A failing build exits
// through the VM's exit path, so under BUN_DESTRUCT_VM_ON_EXIT the VM lets
// go of them and only a leaked reference would be left holding them.
test.concurrent(
"after a failed build tears the VM down",
async () => {
const dir = await tempDirWithBakeDeps(
"bake-production-string-leaks-teardown",
app(`throw new Error("about page failed to render");`),
);

const stderr = await buildUnderLeakSanitizer(dir, { BUN_DESTRUCT_VM_ON_EXIT: "1" });

// The build got as far as loading and running the page modules.
expect(stderr).toContain("about page failed to render");
expect(leakedBunStrings(stderr)).toEqual([]);
},
60_000,
);
});
});
Loading