From 7bf67e78d7665f34f8680bd1ac1147f449fcce8d Mon Sep 17 00:00:00 2001 From: "taylor.fish" Date: Thu, 23 Oct 2025 13:17:51 -0700 Subject: [PATCH] Fix incorrect/suspicious uses of `ZigString.Slice.cloneIfNeeded` (#23937) `ZigString.Slice.cloneIfNeeded` does *not* guarantee that the returned slice will have been allocated by the provided allocator, which makes it very easy to use this method incorrectly. (For internal tracking: fixes ENG-21284) --- src/bun.js/api/filesystem_router.zig | 22 ++++++++++++++++----- src/bun.js/api/glob.zig | 21 +++++++++++--------- src/bun.js/api/server/ServerConfig.zig | 8 ++------ src/bun.js/bindings/JSValue.zig | 10 ++++++++-- src/bun.js/bindings/ZigStackFrame.zig | 6 +++++- src/bun.js/bindings/ZigString.zig | 13 +++++++++---- src/bun.js/webcore/Blob.zig | 27 +++++++++++--------------- src/string.zig | 12 +++++++++++- 8 files changed, 75 insertions(+), 44 deletions(-) diff --git a/src/bun.js/api/filesystem_router.zig b/src/bun.js/api/filesystem_router.zig index 2b1147239b..bd7e8b8118 100644 --- a/src/bun.js/api/filesystem_router.zig +++ b/src/bun.js/api/filesystem_router.zig @@ -87,7 +87,7 @@ pub const FileSystemRouter = struct { return globalThis.throwInvalidArguments("Expected fileExtensions to be an Array of strings", .{}); } if (try val.getLength(globalThis) == 0) continue; - extensions.appendAssumeCapacity(((try val.toSlice(globalThis, allocator)).cloneIfNeeded(allocator) catch unreachable).slice()[1..]); + extensions.appendAssumeCapacity((try val.toUTF8Bytes(globalThis, allocator))[1..]); } } @@ -99,7 +99,7 @@ pub const FileSystemRouter = struct { return globalThis.throwInvalidArguments("Expected assetPrefix to be a string", .{}); } - asset_prefix_slice = (try asset_prefix.toSlice(globalThis, allocator)).cloneIfNeeded(allocator) catch unreachable; + asset_prefix_slice = try (try asset_prefix.toSlice(globalThis, allocator)).cloneIfBorrowed(allocator); } const orig_log = vm.transpiler.resolver.log; var log = Log.Log.init(allocator); @@ -165,6 +165,10 @@ pub const FileSystemRouter = struct { router.config.dir = fs_router.base_dir.?.slice(); fs_router.base_dir.?.ref(); + // TODO: Memory leak? We haven't freed `asset_prefix_slice`, but we can't do so because the + // underlying string is borrowed in `fs_router.router.config.asset_prefix_path`. + // `FileSystemRouter.deinit` frees `fs_router.asset_prefix`, but that's a clone of + // `asset_prefix_slice`. The original is not freed. return fs_router; } @@ -271,7 +275,7 @@ pub const FileSystemRouter = struct { var path: ZigString.Slice = brk: { if (argument.isString()) { - break :brk (try argument.toSlice(globalThis, globalThis.allocator())).cloneIfNeeded(globalThis.allocator()) catch unreachable; + break :brk try (try argument.toSlice(globalThis, globalThis.allocator())).cloneIfBorrowed(globalThis.allocator()); } if (argument.isCell()) { @@ -289,13 +293,14 @@ pub const FileSystemRouter = struct { }; if (path.len == 0 or (path.len == 1 and path.ptr[0] == '/')) { + path.deinit(); path = ZigString.Slice.fromUTF8NeverFree("/"); } if (strings.hasPrefixComptime(path.slice(), "http://") or strings.hasPrefixComptime(path.slice(), "https://") or strings.hasPrefixComptime(path.slice(), "file://")) { const prev_path = path; - path = ZigString.init(URL.parse(path.slice()).pathname).toSliceFast(globalThis.allocator()).cloneIfNeeded(globalThis.allocator()) catch unreachable; - prev_path.deinit(); + defer prev_path.deinit(); + path = try .initDupe(globalThis.allocator(), URL.parse(path.slice()).pathname); } const url_path = URLPath.parse(path.slice()) catch |err| { @@ -319,6 +324,13 @@ pub const FileSystemRouter = struct { this.asset_prefix, this.base_dir.?, ) catch unreachable; + + // TODO: Memory leak? We haven't freed `path`, but we can't do so because the underlying + // string is borrowed in `result.route_holder.pathname` and `result.route_holder.query_string` + // (see `Routes.matchPageWithAllocator`, which does not clone these fields but rather + // directly reuses parts of the `URLPath`, which itself borrows from `path`). + // `MatchedRoute.deinit` doesn't free any fields of `route_holder`, so the string is not + // freed. return result.toJS(globalThis); } diff --git a/src/bun.js/api/glob.zig b/src/bun.js/api/glob.zig index c393448566..44e459c3ff 100644 --- a/src/bun.js/api/glob.zig +++ b/src/bun.js/api/glob.zig @@ -18,20 +18,24 @@ const ScanOpts = struct { error_on_broken_symlinks: bool, fn parseCWD(globalThis: *JSGlobalObject, allocator: std.mem.Allocator, cwdVal: jsc.JSValue, absolute: bool, comptime fnName: string) bun.JSError![]const u8 { - const cwd_str_raw = try cwdVal.toSlice(globalThis, allocator); - if (cwd_str_raw.len == 0) return ""; + const cwd_string: bun.String = try .fromJS(cwdVal, globalThis); + defer cwd_string.deref(); + if (cwd_string.isEmpty()) return ""; + + const cwd_str: []const u8 = cwd_str: { + const cwd_utf8 = cwd_string.toUTF8WithoutRef(allocator); - const cwd_str = cwd_str: { // If its absolute return as is - if (ResolvePath.Platform.auto.isAbsolute(cwd_str_raw.slice())) { - const cwd_str = try cwd_str_raw.cloneIfNeeded(allocator); - break :cwd_str cwd_str.ptr[0..cwd_str.len]; + if (ResolvePath.Platform.auto.isAbsolute(cwd_utf8.slice())) { + break :cwd_str (try cwd_utf8.cloneIfBorrowed(allocator)).slice(); } + defer cwd_utf8.deinit(); var path_buf2: [bun.MAX_PATH_BYTES * 2]u8 = undefined; if (!absolute) { - const cwd_str = ResolvePath.joinStringBuf(&path_buf2, &[_][]const u8{cwd_str_raw.slice()}, .auto); + const parts: []const []const u8 = &.{cwd_utf8.slice()}; + const cwd_str = ResolvePath.joinStringBuf(&path_buf2, parts, .auto); break :cwd_str try allocator.dupe(u8, cwd_str); } @@ -47,9 +51,8 @@ const ScanOpts = struct { const cwd_str = ResolvePath.joinStringBuf(&path_buf2, &[_][]const u8{ cwd, - cwd_str_raw.slice(), + cwd_utf8.slice(), }, .auto); - break :cwd_str try allocator.dupe(u8, cwd_str); }; diff --git a/src/bun.js/api/server/ServerConfig.zig b/src/bun.js/api/server/ServerConfig.zig index 8a1caca83d..5ba1941675 100644 --- a/src/bun.js/api/server/ServerConfig.zig +++ b/src/bun.js/api/server/ServerConfig.zig @@ -803,13 +803,9 @@ pub fn fromJS( if (id.isUndefinedOrNull()) { args.allow_hot = false; } else { - const id_str = try id.toSlice( - global, - bun.default_allocator, - ); - + const id_str = try id.toUTF8Bytes(global, bun.default_allocator); if (id_str.len > 0) { - args.id = (id_str.cloneIfNeeded(bun.default_allocator) catch unreachable).slice(); + args.id = id_str; } else { args.allow_hot = false; } diff --git a/src/bun.js/bindings/JSValue.zig b/src/bun.js/bindings/JSValue.zig index 8dfff1c386..9a208b2d4b 100644 --- a/src/bun.js/bindings/JSValue.zig +++ b/src/bun.js/bindings/JSValue.zig @@ -1187,7 +1187,6 @@ pub const JSValue = enum(i64) { pub fn toSlice(this: JSValue, global: *JSGlobalObject, allocator: std.mem.Allocator) JSError!ZigString.Slice { const str = try bun.String.fromJS(this, global); defer str.deref(); - return str.toUTF8(allocator); } @@ -1195,6 +1194,13 @@ pub const JSValue = enum(i64) { return getZigString(this, global).toSliceZ(allocator); } + /// The returned slice is always owned by `allocator`. + pub fn toUTF8Bytes(this: JSValue, global: *JSGlobalObject, allocator: std.mem.Allocator) JSError![]u8 { + const str: bun.String = try .fromJS(this, global); + defer str.deref(); + return str.toUTF8Bytes(allocator); + } + pub fn toJSString(this: JSValue, globalThis: *JSGlobalObject) bun.JSError!*JSString { return bun.cpp.JSC__JSValue__toStringOrNull(this, globalThis); } @@ -1242,7 +1248,7 @@ pub const JSValue = enum(i64) { allocator: std.mem.Allocator, ) ?ZigString.Slice { var str = this.toJSString(globalThis) catch return null; - return str.toSlice(globalThis, allocator).cloneIfNeeded(allocator) catch { + return str.toSliceClone(globalThis, allocator) catch { globalThis.throwOutOfMemory() catch {}; // TODO: properly propagate exception upwards return null; }; diff --git a/src/bun.js/bindings/ZigStackFrame.zig b/src/bun.js/bindings/ZigStackFrame.zig index 4082b86f25..5a5039f220 100644 --- a/src/bun.js/bindings/ZigStackFrame.zig +++ b/src/bun.js/bindings/ZigStackFrame.zig @@ -23,7 +23,11 @@ pub const ZigStackFrame = extern struct { var frame: api.StackFrame = comptime std.mem.zeroes(api.StackFrame); if (!this.function_name.isEmpty()) { var slicer = this.function_name.toUTF8(allocator); - frame.function_name = (try slicer.cloneIfNeeded(allocator)).slice(); + frame.function_name = (try slicer.cloneIfBorrowed(allocator)).slice(); + // TODO: Memory leak? `frame.function_name` may have just been allocated by this + // function, but it doesn't seem like we ever free it. Changing to `toUTF8Owned` would + // make the ownership clearer, but would also make the memory leak worse without an + // additional free. } if (!this.source_url.isEmpty()) { diff --git a/src/bun.js/bindings/ZigString.zig b/src/bun.js/bindings/ZigString.zig index 9a954969b6..2ea8bf825f 100644 --- a/src/bun.js/bindings/ZigString.zig +++ b/src/bun.js/bindings/ZigString.zig @@ -330,6 +330,10 @@ pub const ZigString = extern struct { }; } + pub fn initDupe(allocator: std.mem.Allocator, input: []const u8) OOM!Slice { + return .init(allocator, try allocator.dupe(u8, input)); + } + pub fn byteLength(this: *const Slice) usize { return this.len; } @@ -394,7 +398,7 @@ pub const ZigString = extern struct { } /// Note that the returned slice is not guaranteed to be allocated by `allocator`. - pub fn cloneIfNeeded(this: Slice, allocator: std.mem.Allocator) bun.OOM!Slice { + pub fn cloneIfBorrowed(this: Slice, allocator: std.mem.Allocator) bun.OOM!Slice { if (this.isAllocated()) { return this; } @@ -642,7 +646,7 @@ pub const ZigString = extern struct { if (this.len == 0) return Slice.empty; if (is16Bit(&this)) { - const buffer = this.toOwnedSlice(allocator) catch unreachable; + const buffer = bun.handleOom(this.toOwnedSlice(allocator)); return Slice{ .allocator = NullableAllocator.init(allocator), .ptr = buffer.ptr, @@ -662,7 +666,7 @@ pub const ZigString = extern struct { if (this.len == 0) return Slice.empty; if (is16Bit(&this)) { - const buffer = this.toOwnedSlice(allocator) catch unreachable; + const buffer = bun.handleOom(this.toOwnedSlice(allocator)); return Slice{ .allocator = NullableAllocator.init(allocator), .ptr = buffer.ptr, @@ -671,7 +675,7 @@ pub const ZigString = extern struct { } if (!this.isUTF8() and !strings.isAllASCII(untagged(this._unsafe_ptr_do_not_use)[0..this.len])) { - const buffer = this.toOwnedSlice(allocator) catch unreachable; + const buffer = bun.handleOom(this.toOwnedSlice(allocator)); return Slice{ .allocator = NullableAllocator.init(allocator), .ptr = buffer.ptr, @@ -685,6 +689,7 @@ pub const ZigString = extern struct { }; } + /// The returned slice is always allocated by `allocator`. pub fn toSliceClone(this: ZigString, allocator: std.mem.Allocator) OOM!Slice { if (this.len == 0) return Slice.empty; diff --git a/src/bun.js/webcore/Blob.zig b/src/bun.js/webcore/Blob.zig index 68c85a3138..173d9df9fb 100644 --- a/src/bun.js/webcore/Blob.zig +++ b/src/bun.js/webcore/Blob.zig @@ -542,8 +542,7 @@ const URLSearchParamsConverter = struct { buf: []u8 = "", globalThis: *jsc.JSGlobalObject, pub fn convert(this: *URLSearchParamsConverter, str: ZigString) void { - var out = bun.handleOom(str.toSlice(this.allocator).cloneIfNeeded(this.allocator)); - this.buf = @constCast(out.slice()); + this.buf = bun.handleOom(str.toOwnedSlice(this.allocator)); } }; @@ -628,8 +627,8 @@ export fn Blob__setAsFile(this: *Blob, path_str: *bun.String) void { if (this.store) |store| { if (store.data == .bytes) { if (store.data.bytes.stored_name.len == 0) { - var utf8 = path_str.toUTF8WithoutRef(bun.default_allocator).cloneIfNeeded(bun.default_allocator) catch unreachable; - store.data.bytes.stored_name = bun.PathString.init(utf8.slice()); + const utf8 = path_str.toUTF8Bytes(bun.default_allocator); + store.data.bytes.stored_name = bun.PathString.init(utf8); } } } @@ -1738,7 +1737,7 @@ pub fn JSDOMFile__construct_(globalThis: *jsc.JSGlobalObject, callframe: *jsc.Ca switch (store_.data) { .bytes => |*bytes| { bytes.stored_name = bun.PathString.init( - bun.handleOom(name_value_str.toUTF8WithoutRef(bun.default_allocator).cloneIfNeeded(bun.default_allocator)).slice(), + name_value_str.toUTF8Bytes(bun.default_allocator), ); }, .s3, .file => { @@ -1750,9 +1749,7 @@ pub fn JSDOMFile__construct_(globalThis: *jsc.JSGlobalObject, callframe: *jsc.Ca blob.store = Blob.Store.new(.{ .data = .{ .bytes = Blob.Store.Bytes.initEmptyWithName( - bun.PathString.init( - bun.handleOom(name_value_str.toUTF8WithoutRef(bun.default_allocator).cloneIfNeeded(bun.default_allocator)).slice(), - ), + bun.PathString.init(name_value_str.toUTF8Bytes(bun.default_allocator)), allocator, ), }, @@ -2483,11 +2480,10 @@ pub fn pipeReadableStreamToBlob(this: *Blob, globalThis: *jsc.JSGlobalObject, re break :brk .{ .fd = store.data.file.pathlike.fd }; } else { break :brk .{ - .path = ZigString.Slice.fromUTF8NeverFree( - store.data.file.pathlike.path.slice(), - ).cloneIfNeeded( + .path = bun.handleOom(ZigString.Slice.initDupe( bun.default_allocator, - ) catch |err| bun.handleOom(err), + store.data.file.pathlike.path.slice(), + )), }; } }; @@ -2723,11 +2719,10 @@ pub fn getWriter( break :brk .{ .fd = store.data.file.pathlike.fd }; } else { break :brk .{ - .path = ZigString.Slice.fromUTF8NeverFree( - store.data.file.pathlike.path.slice(), - ).cloneIfNeeded( + .path = bun.handleOom(ZigString.Slice.initDupe( bun.default_allocator, - ) catch |err| bun.handleOom(err), + store.data.file.pathlike.path.slice(), + )), }; } }; diff --git a/src/string.zig b/src/string.zig index 6572b8e5cf..4c294314c7 100644 --- a/src/string.zig +++ b/src/string.zig @@ -107,7 +107,7 @@ pub const String = extern struct { else .unknown; // string was 16-bit; may or may not be all ascii - const owned_slice = try utf8_slice.cloneIfNeeded(allocator); + const owned_slice = try utf8_slice.cloneIfBorrowed(allocator); // `owned_slice.allocator` is guaranteed to be `allocator`. break :blk .{ owned_slice.mut(), ascii_status }; }, @@ -768,6 +768,16 @@ pub const String = extern struct { return ZigString.Slice.empty; } + /// Equivalent to calling `toUTF8WithoutRef` followed by `cloneIfBorrowed`. + pub fn toUTF8Owned(this: String, allocator: std.mem.Allocator) ZigString.Slice { + return bun.handleOom(this.toUTF8WithoutRef(allocator).cloneIfBorrowed(allocator)); + } + + /// The returned slice is always allocated by `allocator`. + pub fn toUTF8Bytes(this: String, allocator: std.mem.Allocator) []u8 { + return this.toUTF8Owned(allocator).mut(); + } + /// use `byteSlice` to get a `[]const u8`. pub fn toSlice(this: *String, allocator: std.mem.Allocator) SliceWithUnderlyingString { defer this.* = .empty;