S3Client: don't double-free path on error after store takes ownership

src/bun.js/webcore/S3Client.zig

@@ -135,7 +135,6 @@ pub const S3Client = struct {
             }
             return globalThis.throwInvalidArguments("Expected a path", .{});
         };
-        errdefer path.deinit();
         const options = args.nextEat();
         var blob = Blob.new(try S3File.constructS3FileWithS3CredentialsAndOptions(globalThis, path, options, ptr.credentials, ptr.options, ptr.acl, ptr.storage_class, ptr.request_payer));
         return blob.toJS(globalThis);
@@ -151,7 +150,6 @@ pub const S3Client = struct {
             }
             return globalThis.throwInvalidArguments("Expected a path to presign", .{});
         };
-        errdefer path.deinit();
 
         const options = args.nextEat();
         var blob = try S3File.constructS3FileWithS3CredentialsAndOptions(globalThis, path, options, ptr.credentials, ptr.options, ptr.acl, ptr.storage_class, ptr.request_payer);
@@ -169,7 +167,6 @@ pub const S3Client = struct {
             }
             return globalThis.throwInvalidArguments("Expected a path to check if it exists", .{});
         };
-        errdefer path.deinit();
         const options = args.nextEat();
         var blob = try S3File.constructS3FileWithS3CredentialsAndOptions(globalThis, path, options, ptr.credentials, ptr.options, ptr.acl, ptr.storage_class, ptr.request_payer);
         defer blob.detach();
@@ -186,7 +183,6 @@ pub const S3Client = struct {
             }
             return globalThis.throwInvalidArguments("Expected a path to check the size of", .{});
         };
-        errdefer path.deinit();
         const options = args.nextEat();
         var blob = try S3File.constructS3FileWithS3CredentialsAndOptions(globalThis, path, options, ptr.credentials, ptr.options, ptr.acl, ptr.storage_class, ptr.request_payer);
         defer blob.detach();
@@ -203,7 +199,6 @@ pub const S3Client = struct {
             }
             return globalThis.throwInvalidArguments("Expected a path to check the stat of", .{});
         };
-        errdefer path.deinit();
         const options = args.nextEat();
         var blob = try S3File.constructS3FileWithS3CredentialsAndOptions(globalThis, path, options, ptr.credentials, ptr.options, ptr.acl, ptr.storage_class, ptr.request_payer);
         defer blob.detach();
@@ -217,8 +212,8 @@ pub const S3Client = struct {
         const path: jsc.Node.PathLike = try jsc.Node.PathLike.fromJS(globalThis, &args) orelse {
             return globalThis.ERR(.MISSING_ARGS, "Expected a path to write to", .{}).throw();
         };
-        errdefer path.deinit();
         const data = args.nextEat() orelse {
+            path.deinit();
             return globalThis.ERR(.MISSING_ARGS, "Expected a Blob-y thing to write", .{}).throw();
         };
 
@@ -251,7 +246,6 @@ pub const S3Client = struct {
         const path: jsc.Node.PathLike = try jsc.Node.PathLike.fromJS(globalThis, &args) orelse {
             return globalThis.ERR(.MISSING_ARGS, "Expected a path to unlink", .{}).throw();
         };
-        errdefer path.deinit();
         const options = args.nextEat();
         var blob = try S3File.constructS3FileWithS3CredentialsAndOptions(globalThis, path, options, ptr.credentials, ptr.options, ptr.acl, ptr.storage_class, ptr.request_payer);
         defer blob.detach();

src/bun.js/webcore/S3File.zig

@@ -67,11 +67,6 @@ pub fn presign(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.J
 
     // accept a path or a blob
     var path_or_blob = try PathOrBlob.fromJSNoCopy(globalThis, &args);
-    errdefer {
-        if (path_or_blob == .path) {
-            path_or_blob.path.deinit();
-        }
-    }
 
     if (path_or_blob == .blob and (path_or_blob.blob.store == null or path_or_blob.blob.store.?.data != .s3)) {
         return globalThis.throwInvalidArguments("Expected a S3 or path to presign", .{});
@@ -97,12 +92,7 @@ pub fn unlink(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.JS
     defer args.deinit();
 
     // accept a path or a blob
-    var path_or_blob = try PathOrBlob.fromJSNoCopy(globalThis, &args);
-    errdefer {
-        if (path_or_blob == .path) {
-            path_or_blob.path.deinit();
-        }
-    }
+    const path_or_blob = try PathOrBlob.fromJSNoCopy(globalThis, &args);
     if (path_or_blob == .blob and (path_or_blob.blob.store == null or path_or_blob.blob.store.?.data != .s3)) {
         return globalThis.throwInvalidArguments("Expected a S3 or path to delete", .{});
     }
@@ -130,17 +120,13 @@ pub fn write(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.JSE
 
     // accept a path or a blob
     var path_or_blob = try PathOrBlob.fromJSNoCopy(globalThis, &args);
-    errdefer {
-        if (path_or_blob == .path) {
-            path_or_blob.path.deinit();
-        }
-    }
 
     if (path_or_blob == .blob and (path_or_blob.blob.store == null or path_or_blob.blob.store.?.data != .s3)) {
         return globalThis.throwInvalidArguments("Expected a S3 or path to upload", .{});
     }
 
     const data = args.nextEat() orelse {
+        if (path_or_blob == .path) path_or_blob.path.deinit();
         return globalThis.ERR(.MISSING_ARGS, "Expected a Blob-y thing to upload", .{}).throw();
     };
 
@@ -173,11 +159,6 @@ pub fn size(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.JSEr
 
     // accept a path or a blob
     var path_or_blob = try PathOrBlob.fromJSNoCopy(globalThis, &args);
-    errdefer {
-        if (path_or_blob == .path) {
-            path_or_blob.path.deinit();
-        }
-    }
 
     if (path_or_blob == .blob and (path_or_blob.blob.store == null or path_or_blob.blob.store.?.data != .s3)) {
         return globalThis.throwInvalidArguments("Expected a S3 or path to get size", .{});
@@ -206,11 +187,6 @@ pub fn exists(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.JS
 
     // accept a path or a blob
     var path_or_blob = try PathOrBlob.fromJSNoCopy(globalThis, &args);
-    errdefer {
-        if (path_or_blob == .path) {
-            path_or_blob.path.deinit();
-        }
-    }
 
     if (path_or_blob == .blob and (path_or_blob.blob.store == null or path_or_blob.blob.store.?.data != .s3)) {
         return globalThis.throwInvalidArguments("Expected a S3 or path to check if it exists", .{});
@@ -253,7 +229,12 @@ pub fn constructS3FileWithS3CredentialsAndOptions(
     default_storage_class: ?bun.S3.StorageClass,
     default_request_payer: bool,
 ) bun.JSError!Blob {
-    var aws_options = try S3.S3Credentials.getCredentialsWithOptions(default_credentials.*, default_options, options, default_acl, default_storage_class, default_request_payer, globalObject);
+    // This function always takes ownership of `path`. The store created below consumes it;
+    // if we fail before the store is created, we must release it ourselves.
+    var aws_options = S3.S3Credentials.getCredentialsWithOptions(default_credentials.*, default_options, options, default_acl, default_storage_class, default_request_payer, globalObject) catch |err| {
+        path.deinit();
+        return err;
+    };
     defer aws_options.deinit();
 
     const store = brk: {
@@ -304,7 +285,12 @@ pub fn constructS3FileWithS3Credentials(
     options: ?jsc.JSValue,
     existing_credentials: S3.S3Credentials,
 ) bun.JSError!Blob {
-    var aws_options = try S3.S3Credentials.getCredentialsWithOptions(existing_credentials, .{}, options, null, null, false, globalObject);
+    // This function always takes ownership of `path`. The store created below consumes it;
+    // if we fail before the store is created, we must release it ourselves.
+    var aws_options = S3.S3Credentials.getCredentialsWithOptions(existing_credentials, .{}, options, null, null, false, globalObject) catch |err| {
+        path.deinit();
+        return err;
+    };
     defer aws_options.deinit();
     const store = bun.handleOom(Blob.Store.initS3(path, null, aws_options.credentials, bun.default_allocator));
     errdefer store.deinit();
@@ -557,11 +543,6 @@ pub fn stat(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.JSEr
 
     // accept a path or a blob
     var path_or_blob = try PathOrBlob.fromJSNoCopy(globalThis, &args);
-    errdefer {
-        if (path_or_blob == .path) {
-            path_or_blob.path.deinit();
-        }
-    }
 
     if (path_or_blob == .blob and (path_or_blob.blob.store == null or path_or_blob.blob.store.?.data != .s3)) {
         return globalThis.throwInvalidArguments("Expected a S3 or path to get size", .{});

test/js/bun/s3/s3-path-error-double-free.test.ts

@@ -0,0 +1,114 @@
+import { S3Client } from "bun";
+import { describe, expect, test } from "bun:test";
+
+// Regression test: S3Client methods would double-free the path string when an
+// error was thrown after the internal S3 blob store had already taken ownership
+// of the path (via toThreadSafe()). The `errdefer path.deinit()` in the caller
+// then tried to release the same underlying WTFStringImpl again, tripping a
+// refcount assertion in debug builds. The bug only reproduced when the path
+// string was not already a pre-existing atom (so isolatedCopy() produced a new
+// StringImpl and deref'd the original).
+
+describe("S3Client path ownership on error", () => {
+  const throwingData = {
+    [Symbol.toPrimitive]() {
+      throw new Error("boom");
+    },
+  };
+
+  const throwingCredOptions = {
+    get accessKeyId(): string {
+      throw new Error("cred-boom");
+    },
+  };
+
+  const throwingTypeOptions = {
+    get type(): string {
+      throw new Error("type-boom");
+    },
+  };
+
+  // Use a path string that is unlikely to already exist as an atom in the VM.
+  let counter = 0;
+  const freshPath = () => `zzz-unique-s3-path-${process.pid}-${counter++}`;
+
+  describe("instance methods", () => {
+    const client = new S3Client();
+
+    test("write() with data whose string coercion throws", () => {
+      expect(() => client.write(freshPath(), throwingData)).toThrow("boom");
+    });
+
+    test("write() with throwing options.type", () => {
+      expect(() => client.write(freshPath(), throwingData, throwingTypeOptions)).toThrow();
+    });
+
+    test("file() with throwing credentials option", () => {
+      expect(() => client.file(freshPath(), throwingCredOptions)).toThrow("cred-boom");
+    });
+
+    test("presign() with throwing credentials option", () => {
+      expect(() => client.presign(freshPath(), throwingCredOptions)).toThrow("cred-boom");
+    });
+
+    test("presign() with throwing options.type", () => {
+      expect(() => client.presign(freshPath(), throwingTypeOptions)).toThrow("type-boom");
+    });
+
+    test("exists() with throwing credentials option", () => {
+      expect(() => client.exists(freshPath(), throwingCredOptions)).toThrow("cred-boom");
+    });
+
+    test("size() with throwing credentials option", () => {
+      expect(() => client.size(freshPath(), throwingCredOptions)).toThrow("cred-boom");
+    });
+
+    test("stat() with throwing credentials option", () => {
+      expect(() => client.stat(freshPath(), throwingCredOptions)).toThrow("cred-boom");
+    });
+
+    test("unlink() with throwing credentials option", () => {
+      expect(() => client.unlink(freshPath(), throwingCredOptions)).toThrow("cred-boom");
+    });
+
+    test("write() with missing data argument", () => {
+      // @ts-expect-error
+      expect(() => client.write(freshPath())).toThrow();
+    });
+  });
+
+  describe("static methods", () => {
+    test("write() with data whose string coercion throws", () => {
+      expect(() => S3Client.write(freshPath(), throwingData)).toThrow("boom");
+    });
+
+    test("presign() with throwing options.type", () => {
+      expect(() => S3Client.presign(freshPath(), throwingTypeOptions)).toThrow("type-boom");
+    });
+
+    test("exists() with throwing credentials option", () => {
+      expect(() => S3Client.exists(freshPath(), throwingCredOptions)).toThrow("cred-boom");
+    });
+
+    test("size() with throwing credentials option", () => {
+      expect(() => S3Client.size(freshPath(), throwingCredOptions)).toThrow("cred-boom");
+    });
+
+    test("stat() with throwing credentials option", () => {
+      expect(() => S3Client.stat(freshPath(), throwingCredOptions)).toThrow("cred-boom");
+    });
+
+    test("unlink() with throwing credentials option", () => {
+      expect(() => S3Client.unlink(freshPath(), throwingCredOptions)).toThrow("cred-boom");
+    });
+
+    test("file() with throwing credentials option", () => {
+      expect(() => S3Client.file(freshPath(), throwingCredOptions)).toThrow("cred-boom");
+    });
+
+    test("write() with missing data argument", () => {
+      // @ts-expect-error
+      expect(() => S3Client.write(freshPath())).toThrow();
+    });
+  });
+});