Skip to content
Merged
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
1 change: 1 addition & 0 deletions bench/sourcemap/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
big-module.generated.ts
52 changes: 52 additions & 0 deletions bench/sourcemap/internal-sourcemap-bench.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
// Microbench for the InternalSourceMap stack-trace remapping path. Compare
// against a baseline build by running:
//
// bun run build:release bench/sourcemap/internal-sourcemap-bench.ts
//
// On first run this generates a ~150k-line .ts module next to this file so the
// stack frames being remapped live inside a large SavedSourceMap entry.

import fs from "node:fs";
import path from "node:path";

const ITER = 10_000;
const BIG_LINES = 150_000;

const flag = process.argv[2] ?? "bench";
const mb = (n: number) => (n / 1048576).toFixed(2) + " MB";

const bigPath = path.join(import.meta.dir, "big-module.generated.ts");
if (!fs.existsSync(bigPath)) {
let src = "let v: number = 0;\n";
for (let i = 0; i < BIG_LINES; i++) src += `v = (v + ${i % 97}) | 0;\n`;
src += "export function go(): string { return new Error('e').stack!; }\n";
fs.writeFileSync(bigPath, src);
}

const rssBefore = process.memoryUsage().rss;
const tLoad0 = performance.now();
const { go } = require(bigPath) as { go: () => string };
const tLoad1 = performance.now();
const rssAfterLoad = process.memoryUsage().rss;

// First .stack: triggers full VLQ decode in the old path.
const tFirst0 = performance.now();
let s = go();
const tFirst1 = performance.now();
const rssAfterFirst = process.memoryUsage().rss;

const t0 = performance.now();
for (let i = 0; i < ITER; i++) s = go();
const t1 = performance.now();
const rssAfterLoop = process.memoryUsage().rss;

console.log(`[${flag}] load big module: ${(tLoad1 - tLoad0).toFixed(2)} ms`);
console.log(`[${flag}] first new Error().stack: ${(tFirst1 - tFirst0).toFixed(3)} ms`);
console.log(
`[${flag}] ${ITER}x new Error().stack: ${(t1 - t0).toFixed(2)} ms (${(((t1 - t0) * 1000) / ITER).toFixed(2)} µs/op)`,
);
console.log(
`[${flag}] rss before/afterLoad/afterFirst/afterLoop: ${mb(rssBefore)} / ${mb(rssAfterLoad)} / ${mb(rssAfterFirst)} / ${mb(rssAfterLoop)}`,
);
console.log(`[${flag}] rss delta load→firstStack: ${mb(rssAfterFirst - rssAfterLoad)}`);
void s;
38 changes: 19 additions & 19 deletions src/StandaloneModuleGraph.zig
Original file line number Diff line number Diff line change
Expand Up @@ -264,25 +264,22 @@
defer init_lock.unlock();

return switch (this.*) {
.none => null,
.parsed => |map| map,
.serialized => |serialized| {
var stored = switch (SourceMap.Mapping.parse(
bun.default_allocator,
serialized.mappingVLQ(),
null,
std.math.maxInt(i32),
std.math.maxInt(i32),
.{},
)) {
.success => |x| x,
.fail => {
this.* = .none;
return null;
},
const blob = serialized.mappingBlob();
if (blob.len < SourceMap.InternalSourceMap.header_size) {
this.* = .none;
return null;
}
const ism = SourceMap.InternalSourceMap{ .data = blob.ptr };
var stored: SourceMap.ParsedSourceMap = .{
.ref_count = .init(),
.internal = ism,
.input_line_count = ism.inputLineCount(),
};
Comment thread
coderabbitai[bot] marked this conversation as resolved.

const source_files = serialized.sourceFileNames();

Check failure on line 282 in src/StandaloneModuleGraph.zig

View check run for this annotation

Claude / Claude Code Review

mappingBlob() slices map.bytes without bounds check (corrupt --compile exec panics)

mappingBlob() slices map.bytes[start..][0..head.map_bytes_length] without validating that start <= map.bytes.len or that start + head.map_bytes_length <= map.bytes.len. For a truncated or malformed --compile embedded section this panics in ReleaseSafe or produces UB in ReleaseFast, before the blob.len < header_size guard in LazySourceMap.load() has any chance to run. Add bounds validation inside mappingBlob() before slicing.
Comment thread
claude[bot] marked this conversation as resolved.
Comment on lines 267 to 285

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.

🔴 In ParsedSourceMap.deinit(), the external_source_names cleanup block (lines 113–117 of src/sourcemap/ParsedSourceMap.zig) lacks the is_standalone_module_graph guard that the adjacent ism.deinit() call correctly has. For standalone module graph PSMs, external_source_names elements are borrowed sub-slices of serialized.bytes (not heap-allocated), and the backing allocation is alloc(?[]u8, source_files.len * 2) while external_source_names only covers the first half — so if deinit() were called, it would both free non-heap pointers and pass a size-N slice to the allocator when the allocation was 2N, causing heap corruption. Currently latent because parsed.ref() at StandaloneModuleGraph.zig:306 keeps the refcount ≥ 2; add the same \!this.is_standalone_module_graph guard to the external_source_names block.

Extended reasoning...

What the bug is and how it manifests

In LazySourceMap.load() (StandaloneModuleGraph.zig:286), a single allocation of 2N elements is made:

const slices = bun.default_allocator.alloc(?[]u8, source_files.len * 2);

The first half becomes file_names (a slice of length N), and each element is set to src.slice(serialized.bytes) — a borrowed sub-slice pointing into the standalone binary blob, not a separate heap allocation. This is then stored:

stored.external_source_names = file_names;  // N borrowed pointers
stored.is_standalone_module_graph = true;
parsed.ref(); // never free

The specific code path that triggers it

ParsedSourceMap.deinit() (src/sourcemap/ParsedSourceMap.zig:113–117) contains:

if (this.external_source_names.len > 0) {
    for (this.external_source_names) |name|
        allocator.free(name);              // (1) frees borrowed blob pointers
    allocator.free(this.external_source_names); // (2) size-N free of a 2N allocation
}

This has no is_standalone_module_graph guard. Contrast with the ism.deinit() call immediately above it (line 109), which this PR correctly guarded:

if (this.internal) |ism| {
    if (\!this.is_standalone_module_graph) ism.deinit();  // ← guarded
}

The inconsistency — adding the guard for ISM but not for external_source_names — is the bug introduced by this PR.

Why existing code doesn't prevent it

The protection is entirely behavioral: parsed.ref() // never free at StandaloneModuleGraph.zig:306 leaves the refcount at 2, so deinit() never reaches zero in normal operation. But this is a fragile invariant: any future refcount refactor, test harness that calls deref(), or leak-finder that attempts balanced ref/deref would trigger both heap corruptions silently in ReleaseFast builds.

What the impact would be

If deinit() were called on a standalone PSM:

  1. allocator.free(name) for each name in external_source_names — each name is a slice into serialized.bytes (mmap'd or embedded binary data), not a heap allocation. Freeing a non-heap pointer is immediate heap corruption.
  2. allocator.free(this.external_source_names) passes a []const u8 of length N to the allocator, but the original allocation was alloc(?[]u8, 2*N). Passing the wrong size to free corrupts allocator metadata in tcmalloc/mimalloc.

Step-by-step proof

  1. bun build --compile --sourcemap=inline produces an executable with embedded SerializedSourceMap.
  2. At runtime, LazySourceMap.load() reaches the .serialized branch: allocates slices[0..2N], sets file_names = @ptrCast(slices[0..N]), fills each element with src.slice(serialized.bytes) (borrowed pointer into the embedded section), sets stored.external_source_names = file_names, stored.is_standalone_module_graph = true, then calls parsed.ref() bringing refcount to 2.
  3. Suppose a future deref() is added (or the ref() is removed as a cleanup): refcount reaches 0, deinit() is called.
  4. ism.deinit() is skipped (correctly, because is_standalone_module_graph == true).
  5. allocator.free(name) runs on each borrowed pointer → heap corruption.
  6. allocator.free(this.external_source_names) passes the wrong size → heap corruption.

How to fix it

Add the same guard to the external_source_names block:

if (this.external_source_names.len > 0 and \!this.is_standalone_module_graph) {
    for (this.external_source_names) |name|
        allocator.free(name);
    allocator.free(this.external_source_names);
}

This mirrors exactly the pattern already used for ism.deinit() on line 109.

const slices = bun.handleOom(bun.default_allocator.alloc(?[]u8, source_files.len * 2));

const file_names: [][]const u8 = @ptrCast(slices[0..source_files.len]);
Expand Down Expand Up @@ -1358,14 +1355,15 @@

/// Source map serialization in the bundler is specially designed to be
/// loaded in memory as is. Source contents are compressed with ZSTD to
/// reduce the file size, and mappings are stored as uncompressed VLQ.
/// reduce the file size, and mappings are stored as an InternalSourceMap
/// blob (varint deltas + sync points) so lookups need no decode pass.
pub const SerializedSourceMap = struct {
bytes: []const u8,

/// Following the header bytes:
/// - source_files_count number of StringPointer, file names
/// - source_files_count number of StringPointer, zstd compressed contents
/// - the mapping data, `map_vlq_length` bytes
/// - the InternalSourceMap blob, `map_bytes_length` bytes
/// - all the StringPointer contents
pub const Header = extern struct {
source_files_count: u32,
Expand All @@ -1376,7 +1374,7 @@
return @ptrCast(map.bytes.ptr);
}

pub fn mappingVLQ(map: SerializedSourceMap) []const u8 {
pub fn mappingBlob(map: SerializedSourceMap) []const u8 {
const head = map.header();
const start = @sizeOf(Header) + head.source_files_count * @sizeOf(StringPointer) * 2;
return map.bytes[start..][0..head.map_bytes_length];
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
Expand Down Expand Up @@ -1466,14 +1464,16 @@
}

const map_vlq: []const u8 = mappings_str.data.e_string.slice(arena);
const map_blob = SourceMap.InternalSourceMap.fromVLQ(arena, map_vlq, 0) catch
return error.InvalidSourceMap;
Comment thread
claude[bot] marked this conversation as resolved.

try out.writeInt(u32, sources_paths.items.len, .little);
try out.writeInt(u32, @intCast(map_vlq.len), .little);
try out.writeInt(u32, @intCast(map_blob.len), .little);

const string_payload_start_location = @sizeOf(u32) +
@sizeOf(u32) +
@sizeOf(bun.StringPointer) * sources_content.items.len * 2 + // path + source
map_vlq.len;
map_blob.len;

for (sources_paths.items.slice()) |item| {
if (item.data != .e_string)
Expand Down Expand Up @@ -1519,7 +1519,7 @@
try out.writeInt(u32, slice.length, .little);
}

try out.writeAll(map_vlq);
try out.writeAll(map_blob);

bun.assert(header_list.items.len == string_payload_start_location);
}
Expand Down
3 changes: 2 additions & 1 deletion src/bun.js/RuntimeTranspilerCache.zig
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@
/// Version 16: Added typeof undefined minification optimization.
/// Version 17: Removed transpiler import rewrite for bun:test. Not bumping it causes test/js/bun/http/req-url-leak.test.ts to fail with SyntaxError: Export named 'expect' not found in module 'bun:test'.
/// Version 18: Include ESM record (module info) with an ES Module, see #15758
const expected_version = 18;
/// Version 19: Sourcemap blob is InternalSourceMap (varint stream + sync points), not VLQ.
const expected_version = 19;

const debug = Output.scoped(.cache, .visible);
const MINIMUM_CACHE_SIZE = 50 * 1024;
Expand Down
Loading
Loading