-
Notifications
You must be signed in to change notification settings - Fork 5k
Fix use-after-free in S3 Store.initS3 PathLike refcounting #28417
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
e3d0dd9
ca64e3d
023b526
b6a0a26
b7fb02f
d2bacc7
9ef58f0
4ab6e75
ee3ff22
3008e5b
be8105e
9b1ffda
bef6631
737b8ef
97ad865
ca543de
c5505f1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -83,6 +83,8 @@ pub fn presign(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.J | |
| return globalThis.throwInvalidArguments("Expected a S3 or path to presign", .{}); | ||
| } | ||
| const options = args.nextEat(); | ||
| // constructS3FileInternalStore takes ownership of path. | ||
| path_or_blob = .{ .blob = .initEmpty(globalThis) }; | ||
| var blob = try constructS3FileInternalStore(globalThis, path.path, options); | ||
| defer blob.deinit(); | ||
| return try getPresignUrlFrom(&blob, globalThis, options); | ||
|
|
@@ -113,6 +115,8 @@ pub fn unlink(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.JS | |
| return globalThis.throwInvalidArguments("Expected a S3 or path to delete", .{}); | ||
| } | ||
| const options = args.nextEat(); | ||
| // constructS3FileInternalStore takes ownership of path. | ||
| path_or_blob = .{ .blob = .initEmpty(globalThis) }; | ||
| var blob = try constructS3FileInternalStore(globalThis, path.path, options); | ||
| defer blob.deinit(); | ||
| return try blob.store.?.data.s3.unlink(blob.store.?, globalThis, options); | ||
|
|
@@ -150,6 +154,8 @@ pub fn write(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.JSE | |
| if (path == .fd) { | ||
| return globalThis.throwInvalidArguments("Expected a S3 or path to upload", .{}); | ||
| } | ||
| // constructS3FileInternalStore takes ownership of path. | ||
| path_or_blob = .{ .blob = .initEmpty(globalThis) }; | ||
| var blob = try constructS3FileInternalStore(globalThis, path.path, options); | ||
| defer blob.deinit(); | ||
|
|
||
|
|
@@ -189,6 +195,8 @@ pub fn size(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.JSEr | |
| if (path == .fd) { | ||
| return globalThis.throwInvalidArguments("Expected a S3 or path to get size", .{}); | ||
| } | ||
| // constructS3FileInternalStore takes ownership of path. | ||
| path_or_blob = .{ .blob = .initEmpty(globalThis) }; | ||
| var blob = try constructS3FileInternalStore(globalThis, path.path, options); | ||
| defer blob.deinit(); | ||
|
|
||
|
|
@@ -222,6 +230,8 @@ pub fn exists(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.JS | |
| if (path == .fd) { | ||
| return globalThis.throwInvalidArguments("Expected a S3 or path to check if it exists", .{}); | ||
| } | ||
| // constructS3FileInternalStore takes ownership of path. | ||
| path_or_blob = .{ .blob = .initEmpty(globalThis) }; | ||
| var blob = try constructS3FileInternalStore(globalThis, path.path, options); | ||
| defer blob.deinit(); | ||
|
|
||
|
|
@@ -253,6 +263,12 @@ pub fn constructS3FileWithS3CredentialsAndOptions( | |
| default_storage_class: ?bun.S3.StorageClass, | ||
| default_request_payer: bool, | ||
| ) bun.JSError!Blob { | ||
| // This function takes ownership of `path`. If we fail before passing | ||
| // it to initS3/initS3WithReferencedCredentials (which consume it via | ||
| // toThreadSafe), we must clean it up ourselves. | ||
| var path_to_clean = path; | ||
| errdefer path_to_clean.deinit(); | ||
|
|
||
| var aws_options = try S3.S3Credentials.getCredentialsWithOptions(default_credentials.*, default_options, options, default_acl, default_storage_class, default_request_payer, globalObject); | ||
| defer aws_options.deinit(); | ||
|
|
||
|
|
@@ -263,6 +279,9 @@ pub fn constructS3FileWithS3CredentialsAndOptions( | |
| break :brk bun.handleOom(Blob.Store.initS3WithReferencedCredentials(path, null, default_credentials, bun.default_allocator)); | ||
| } | ||
| }; | ||
| // Path has been consumed by initS3/initS3WithReferencedCredentials | ||
| // via toThreadSafe — neutralize errdefer to prevent double-free. | ||
| path_to_clean = .{ .string = bun.PathString.empty }; | ||
| errdefer store.deinit(); | ||
| store.data.s3.options = aws_options.options; | ||
| store.data.s3.acl = aws_options.acl; | ||
|
|
@@ -304,9 +323,15 @@ pub fn constructS3FileWithS3Credentials( | |
| options: ?jsc.JSValue, | ||
| existing_credentials: S3.S3Credentials, | ||
| ) bun.JSError!Blob { | ||
| // This function takes ownership of `path`. | ||
| var path_to_clean = path; | ||
| errdefer path_to_clean.deinit(); | ||
|
|
||
| var aws_options = try S3.S3Credentials.getCredentialsWithOptions(existing_credentials, .{}, options, null, null, false, globalObject); | ||
| defer aws_options.deinit(); | ||
| const store = bun.handleOom(Blob.Store.initS3(path, null, aws_options.credentials, bun.default_allocator)); | ||
| // Path consumed by initS3 via toThreadSafe. | ||
| path_to_clean = .{ .string = bun.PathString.empty }; | ||
| errdefer store.deinit(); | ||
| store.data.s3.options = aws_options.options; | ||
| store.data.s3.acl = aws_options.acl; | ||
|
|
@@ -573,6 +598,8 @@ pub fn stat(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.JSEr | |
| if (path == .fd) { | ||
| return globalThis.throwInvalidArguments("Expected a S3 or path to get size", .{}); | ||
| } | ||
| // constructS3FileInternalStore takes ownership of path. | ||
| path_or_blob = .{ .blob = .initEmpty(globalThis) }; | ||
| var blob = try constructS3FileInternalStore(globalThis, path.path, options); | ||
| defer blob.deinit(); | ||
|
|
||
|
Comment on lines
598
to
605
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟣 The stat() function in S3File.zig contains two error messages copied from size() that say "Expected a S3 or path to get size" instead of referencing stat -- callers who pass a non-S3 blob or a file descriptor to S3.stat() will see a misleading error message. This is a pre-existing copy-paste error, but the PR directly modifies this function body (adding the ownership-transfer lines at lines 601-602), making it an adjacent opportunity to fix the messages. Extended reasoning...What the bug isThe stat() function in src/bun.js/webcore/S3File.zig contains two error messages copied verbatim from the size() function and never updated:
Both should say something like "Expected a S3 or path to stat" to correctly describe what stat() does. The specific code path that triggers itAny call to S3.stat() or Bun.s3.stat() that fails one of these guards produces the misleading message:
In both cases the user sees an error about "get size" even though they called stat, not size. Why existing code does not prevent itThis is purely a copy-paste oversight. The size() function at lines 180-210 uses identical guard messages. When stat() was created from size() as a template, these strings were never updated. There is no automated check that validates error message text against the function name. ImpactThe impact is confusion at the JavaScript API surface: a user calling Bun.s3.stat(blob) with a non-S3 blob or a file descriptor receives an error message that tells them they cannot "get size" of that path, which is irrelevant to what they were trying to do. While not a correctness bug, this is a DX issue that makes debugging harder. How to fix itChange both error strings in stat() from "Expected a S3 or path to get size" to "Expected a S3 or path to stat" (or similar stat-specific wording). The size() function's identical messages are correct for that function and should remain unchanged. Step-by-step proof
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| import { expect, test } from "bun:test"; | ||
| import { bunEnv, bunExe } from "harness"; | ||
|
|
||
| test("s3 presign with missing credentials throws instead of crashing", async () => { | ||
|
claude[bot] marked this conversation as resolved.
|
||
| // Scrub AWS credential/config env vars so the test always hits the | ||
| // missing-credentials path regardless of ambient host configuration. | ||
| const env: Record<string, string> = {}; | ||
| for (const [key, value] of Object.entries(bunEnv)) { | ||
| if (!key.startsWith("AWS_") && !key.startsWith("S3_") && !key.startsWith("BUN_S3_")) { | ||
| env[key] = value as string; | ||
| } | ||
| } | ||
|
|
||
| // Test instance method (constructS3FileWithS3CredentialsAndOptions): | ||
| // - initS3WithReferencedCredentials (no credential overrides) | ||
| // - initS3 (with per-request credentials that still lack endpoint/bucket) | ||
| // Test static method (constructS3FileWithS3Credentials): | ||
| // - Bun.S3Client.presign (static path) | ||
| const code = [ | ||
| `try { Bun.s3.presign("mykey"); } catch(e) { console.log(e.code); }`, | ||
| `try { Bun.s3.presign("mykey", { accessKeyId: "x", secretAccessKey: "y" }); } catch(e) { console.log(e.code); }`, | ||
| `try { Bun.S3Client.presign("mykey"); } catch(e) { console.log(e.code); }`, | ||
| ].join("\n"); | ||
|
|
||
| await using proc = Bun.spawn({ | ||
| cmd: [bunExe(), "-e", code], | ||
| env, | ||
| stdout: "pipe", | ||
| stderr: "inherit", | ||
| }); | ||
|
|
||
| const [stdout, exitCode] = await Promise.all([proc.stdout.text(), proc.exited]); | ||
|
|
||
| expect(stdout.trim()).toBe("ERR_S3_MISSING_CREDENTIALS\nERR_S3_INVALID_PATH\nERR_S3_MISSING_CREDENTIALS"); | ||
| expect(exitCode).toBe(0); | ||
| }); | ||
Uh oh!
There was an error while loading. Please reload this page.