From f2e487b1e64ab0e39d2ea7c59cc6ba5339eaa4c6 Mon Sep 17 00:00:00 2001 From: Zack Radisic <56137411+zackradisic@users.noreply.github.com> Date: Sun, 27 Jul 2025 16:54:39 -0700 Subject: [PATCH] Remove `ZigString.Slice.clone(...)` (#21410) ### What does this PR do? Removes `ZigString.Slice.clone(...)` and replaces all of its usages with `.cloneIfNeeded(...)` which is what it did anyway (why did this alias exist in the first place?) Anyone reading code that sees `.clone(...)` would expect it to clone the underlying string. This makes it _extremely_ easy to write code which looks okay but actually results in a use-after-free: ```zig const out: []const u8 = out: { const string = bun.String.cloneUTF8("hello friends!"); defer string.deref(); const utf8_slice = string.toUTF8(bun.default_allocator); defer utf8_slice.deinit(); // doesn't actually clone const cloned = utf8_slice.clone(bun.default_allocator) catch bun.outOfMemory(); break :out cloned.slice(); }; std.debug.print("Use after free: {s}!\n", .{out}); ``` (This is a simplification of an actual example from the codebase) --- src/bun.js/api/filesystem_router.zig | 8 ++++---- src/bun.js/api/glob.zig | 2 +- src/bun.js/bindings/JSValue.zig | 9 +++++---- src/bun.js/bindings/ZigStackFrame.zig | 3 +-- src/bun.js/bindings/ZigString.zig | 22 ---------------------- src/bun.js/webcore/Blob.zig | 10 +++++----- src/string.zig | 2 +- 7 files changed, 17 insertions(+), 39 deletions(-) diff --git a/src/bun.js/api/filesystem_router.zig b/src/bun.js/api/filesystem_router.zig index f3a0da0695..1d2d52a139 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)).clone(allocator) catch unreachable).slice()[1..]); + extensions.appendAssumeCapacity(((try val.toSlice(globalThis, allocator)).cloneIfNeeded(allocator) catch unreachable).slice()[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)).clone(allocator) catch unreachable; + asset_prefix_slice = (try asset_prefix.toSlice(globalThis, allocator)).cloneIfNeeded(allocator) catch unreachable; } const orig_log = vm.transpiler.resolver.log; var log = Log.Log.init(allocator); @@ -271,7 +271,7 @@ pub const FileSystemRouter = struct { var path: ZigString.Slice = brk: { if (argument.isString()) { - break :brk (try argument.toSlice(globalThis, globalThis.allocator())).clone(globalThis.allocator()) catch unreachable; + break :brk (try argument.toSlice(globalThis, globalThis.allocator())).cloneIfNeeded(globalThis.allocator()) catch unreachable; } if (argument.isCell()) { @@ -294,7 +294,7 @@ pub const FileSystemRouter = struct { 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()).clone(globalThis.allocator()) catch unreachable; + path = ZigString.init(URL.parse(path.slice()).pathname).toSliceFast(globalThis.allocator()).cloneIfNeeded(globalThis.allocator()) catch unreachable; prev_path.deinit(); } diff --git a/src/bun.js/api/glob.zig b/src/bun.js/api/glob.zig index f01ca494fc..a2b8bcb755 100644 --- a/src/bun.js/api/glob.zig +++ b/src/bun.js/api/glob.zig @@ -24,7 +24,7 @@ const ScanOpts = struct { 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.clone(allocator); + const cwd_str = try cwd_str_raw.cloneIfNeeded(allocator); break :cwd_str cwd_str.ptr[0..cwd_str.len]; } diff --git a/src/bun.js/bindings/JSValue.zig b/src/bun.js/bindings/JSValue.zig index 581ab51224..c9516d8de7 100644 --- a/src/bun.js/bindings/JSValue.zig +++ b/src/bun.js/bindings/JSValue.zig @@ -1146,14 +1146,15 @@ pub const JSValue = enum(i64) { /// Convert a JSValue to a string, potentially calling `toString` on the /// JSValue in JavaScript. Can throw an error. + /// + /// This keeps the WTF::StringImpl alive if it was originally a latin1 + /// ASCII-only string. + /// + /// Otherwise, it will be cloned using the allocator. 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(); - // This keeps the WTF::StringImpl alive if it was originally a latin1 - // ASCII-only string. - // - // Otherwise, it will be cloned using the allocator. return str.toUTF8(allocator); } diff --git a/src/bun.js/bindings/ZigStackFrame.zig b/src/bun.js/bindings/ZigStackFrame.zig index fdf933d7a0..8b9a153d13 100644 --- a/src/bun.js/bindings/ZigStackFrame.zig +++ b/src/bun.js/bindings/ZigStackFrame.zig @@ -19,8 +19,7 @@ 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); - defer slicer.deinit(); - frame.function_name = (try slicer.clone(allocator)).slice(); + frame.function_name = (try slicer.cloneIfNeeded(allocator)).slice(); } if (!this.source_url.isEmpty()) { diff --git a/src/bun.js/bindings/ZigString.zig b/src/bun.js/bindings/ZigString.zig index 12a8a91bff..3bf4704818 100644 --- a/src/bun.js/bindings/ZigString.zig +++ b/src/bun.js/bindings/ZigString.zig @@ -138,18 +138,6 @@ pub const ZigString = extern struct { return strings.isAllASCII(this.slice()); } - pub fn clone(this: ZigString, allocator: std.mem.Allocator) !ZigString { - var sliced = this.toSlice(allocator); - if (!sliced.isAllocated()) { - var str = ZigString.init(try allocator.dupe(u8, sliced.slice())); - str.mark(); - str.markUTF8(); - return str; - } - - return this; - } - extern fn ZigString__toJSONObject(this: *const ZigString, *jsc.JSGlobalObject) callconv(.C) jsc.JSValue; pub fn toJSONObject(this: ZigString, globalThis: *jsc.JSGlobalObject) JSValue { @@ -377,16 +365,6 @@ pub const ZigString = extern struct { return .{ .allocator = .init(allocator), .ptr = duped.ptr, .len = this.len }; } - // TODO: this is identical to `cloneIfNeeded` - pub fn clone(this: Slice, allocator: std.mem.Allocator) OOM!Slice { - if (this.isAllocated()) { - return Slice{ .allocator = this.allocator, .ptr = this.ptr, .len = this.len }; - } - - const duped = try allocator.dupe(u8, this.ptr[0..this.len]); - return Slice{ .allocator = NullableAllocator.init(allocator), .ptr = duped.ptr, .len = this.len }; - } - pub fn cloneIfNeeded(this: Slice, allocator: std.mem.Allocator) !Slice { if (this.isAllocated()) { return this; diff --git a/src/bun.js/webcore/Blob.zig b/src/bun.js/webcore/Blob.zig index 37d6abf530..4a1869ab53 100644 --- a/src/bun.js/webcore/Blob.zig +++ b/src/bun.js/webcore/Blob.zig @@ -590,7 +590,7 @@ export fn Blob__setAsFile(this: *Blob, path_str: *bun.String) *Blob { if (this.store) |store| { if (store.data == .bytes) { if (store.data.bytes.stored_name.len == 0) { - var utf8 = path_str.toUTF8WithoutRef(bun.default_allocator).clone(bun.default_allocator) catch unreachable; + var utf8 = path_str.toUTF8WithoutRef(bun.default_allocator).cloneIfNeeded(bun.default_allocator) catch unreachable; store.data.bytes.stored_name = bun.PathString.init(utf8.slice()); } } @@ -1713,7 +1713,7 @@ pub fn JSDOMFile__construct_(globalThis: *jsc.JSGlobalObject, callframe: *jsc.Ca switch (store_.data) { .bytes => |*bytes| { bytes.stored_name = bun.PathString.init( - (name_value_str.toUTF8WithoutRef(bun.default_allocator).clone(bun.default_allocator) catch bun.outOfMemory()).slice(), + (name_value_str.toUTF8WithoutRef(bun.default_allocator).cloneIfNeeded(bun.default_allocator) catch bun.outOfMemory()).slice(), ); }, .s3, .file => { @@ -1726,7 +1726,7 @@ pub fn JSDOMFile__construct_(globalThis: *jsc.JSGlobalObject, callframe: *jsc.Ca .data = .{ .bytes = Blob.Store.Bytes.initEmptyWithName( bun.PathString.init( - (name_value_str.toUTF8WithoutRef(bun.default_allocator).clone(bun.default_allocator) catch bun.outOfMemory()).slice(), + (name_value_str.toUTF8WithoutRef(bun.default_allocator).cloneIfNeeded(bun.default_allocator) catch bun.outOfMemory()).slice(), ), allocator, ), @@ -2459,7 +2459,7 @@ pub fn pipeReadableStreamToBlob(this: *Blob, globalThis: *jsc.JSGlobalObject, re break :brk .{ .path = ZigString.Slice.fromUTF8NeverFree( store.data.file.pathlike.path.slice(), - ).clone( + ).cloneIfNeeded( bun.default_allocator, ) catch bun.outOfMemory(), }; @@ -2702,7 +2702,7 @@ pub fn getWriter( break :brk .{ .path = ZigString.Slice.fromUTF8NeverFree( store.data.file.pathlike.path.slice(), - ).clone( + ).cloneIfNeeded( globalThis.allocator(), ) catch bun.outOfMemory(), }; diff --git a/src/string.zig b/src/string.zig index d17ae1e652..fddb9b320a 100644 --- a/src/string.zig +++ b/src/string.zig @@ -89,7 +89,7 @@ pub const String = extern struct { } } - return .{ @constCast((try utf8_slice.clone(allocator)).slice()), true }; + return .{ @constCast((try utf8_slice.cloneIfNeeded(allocator)).slice()), true }; }, .StaticZigString => return .{ try this.value.StaticZigString.toOwnedSlice(allocator), false }, else => return .{ &[_]u8{}, false },