aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorGravatar Jarred Sumner <jarred@jarredsumner.com> 2023-08-03 20:09:05 -0700
committerGravatar GitHub <noreply@github.com> 2023-08-03 20:09:05 -0700
commit9beccc3305b1931311605d512ca3e85295c23f55 (patch)
tree93dd66e29b48ae491896c6ab4d02629b3d2b3654
parent717f0a2f423fce1245b59f33e1dab1f5e6c86734 (diff)
downloadbun-9beccc3305b1931311605d512ca3e85295c23f55.tar.gz
bun-9beccc3305b1931311605d512ca3e85295c23f55.tar.zst
bun-9beccc3305b1931311605d512ca3e85295c23f55.zip
Fix thread safety issue in async fs functions file paths (#3964)
Co-authored-by: Jarred Sumner <709451+Jarred-Sumner@users.noreply.github.com>
-rw-r--r--src/ArenaAllocator.zig11
-rw-r--r--src/bun.js/bindings/BunString.cpp11
-rw-r--r--src/bun.js/node/node_fs.zig9
-rw-r--r--src/bun.js/node/types.zig12
-rw-r--r--src/string.zig24
-rw-r--r--test/js/node/fs/fs.test.ts33
6 files changed, 91 insertions, 9 deletions
diff --git a/src/ArenaAllocator.zig b/src/ArenaAllocator.zig
index 2ccb08d19..c2d8718fd 100644
--- a/src/ArenaAllocator.zig
+++ b/src/ArenaAllocator.zig
@@ -1,4 +1,3 @@
-/// TODO: delete this once we've upgraded Zig and https://github.com/ziglang/zig/pull/15985 is merged.
const std = @import("std");
const assert = std.debug.assert;
const mem = std.mem;
@@ -152,7 +151,7 @@ pub const ArenaAllocator = struct {
return false;
};
self.child_allocator.rawFree(first_alloc_buf, align_bits, @returnAddress());
- const node = @as(*BufNode, @ptrCast(@alignCast(new_ptr)));
+ const node: *BufNode = @ptrCast(@alignCast(new_ptr));
node.* = .{ .data = total_size };
self.state.buffer_list.first = node;
}
@@ -167,7 +166,7 @@ pub const ArenaAllocator = struct {
const log2_align = comptime std.math.log2_int(usize, @alignOf(BufNode));
const ptr = self.child_allocator.rawAlloc(len, log2_align, @returnAddress()) orelse
return null;
- const buf_node = @as(*BufNode, @ptrCast(@alignCast(ptr)));
+ const buf_node: *BufNode = @ptrCast(@alignCast(ptr));
buf_node.* = .{ .data = len };
self.state.buffer_list.prepend(buf_node);
self.state.end_index = 0;
@@ -175,7 +174,7 @@ pub const ArenaAllocator = struct {
}
fn alloc(ctx: *anyopaque, n: usize, log2_ptr_align: u8, ra: usize) ?[*]u8 {
- const self = @as(*ArenaAllocator, @ptrCast(@alignCast(ctx)));
+ const self: *ArenaAllocator = @ptrCast(@alignCast(ctx));
_ = ra;
const ptr_align = @as(usize, 1) << @as(Allocator.Log2Align, @intCast(log2_ptr_align));
@@ -209,7 +208,7 @@ pub const ArenaAllocator = struct {
}
fn resize(ctx: *anyopaque, buf: []u8, log2_buf_align: u8, new_len: usize, ret_addr: usize) bool {
- const self = @as(*ArenaAllocator, @ptrCast(@alignCast(ctx)));
+ const self: *ArenaAllocator = @ptrCast(@alignCast(ctx));
_ = log2_buf_align;
_ = ret_addr;
@@ -236,7 +235,7 @@ pub const ArenaAllocator = struct {
_ = log2_buf_align;
_ = ret_addr;
- const self = @as(*ArenaAllocator, @ptrCast(@alignCast(ctx)));
+ const self: *ArenaAllocator = @ptrCast(@alignCast(ctx));
const cur_node = self.state.buffer_list.first orelse return;
const cur_buf = @as([*]u8, @ptrCast(cur_node))[@sizeOf(BufNode)..cur_node.data];
diff --git a/src/bun.js/bindings/BunString.cpp b/src/bun.js/bindings/BunString.cpp
index 630d9be76..09b545cba 100644
--- a/src/bun.js/bindings/BunString.cpp
+++ b/src/bun.js/bindings/BunString.cpp
@@ -96,6 +96,17 @@ BunString fromJS(JSC::JSGlobalObject* globalObject, JSValue value)
return { BunStringTag::WTFStringImpl, { .wtf = wtfString.impl() } };
}
+extern "C" void BunString__toThreadSafe(BunString* str)
+{
+ if (str->tag == BunStringTag::WTFStringImpl) {
+ auto impl = str->impl.wtf->isolatedCopy();
+ if (impl.ptr() != str->impl.wtf) {
+ impl->ref();
+ str->impl.wtf = &impl.leakRef();
+ }
+ }
+}
+
BunString toString(JSC::JSGlobalObject* globalObject, JSValue value)
{
return fromJS(globalObject, value);
diff --git a/src/bun.js/node/node_fs.zig b/src/bun.js/node/node_fs.zig
index 2068cca3f..826fde635 100644
--- a/src/bun.js/node/node_fs.zig
+++ b/src/bun.js/node/node_fs.zig
@@ -74,6 +74,7 @@ pub const AsyncReaddirTask = struct {
.arena = arena,
};
task.ref.ref(vm);
+ task.args.path.toThreadSafe();
JSC.WorkPool.schedule(&task.task);
@@ -157,6 +158,7 @@ pub const AsyncStatTask = struct {
.arena = arena,
};
task.ref.ref(vm);
+ task.args.path.toThreadSafe();
JSC.WorkPool.schedule(&task.task);
@@ -240,6 +242,7 @@ pub const AsyncReadFileTask = struct {
.arena = arena,
};
task.ref.ref(vm);
+ task.args.path.toThreadSafe();
JSC.WorkPool.schedule(&task.task);
@@ -904,7 +907,7 @@ pub const Arguments = struct {
}
pub fn fromJS(ctx: JSC.C.JSContextRef, arguments: *ArgumentsSlice, exception: JSC.C.ExceptionRef) ?Stat {
- const path = PathLike.fromJS(ctx, arguments, exception) orelse {
+ const path = PathLike.fromJSWithAllocator(ctx, arguments, bun.default_allocator, exception) orelse {
if (exception.* == null) {
JSC.throwInvalidArguments(
"path must be a string or TypedArray",
@@ -1405,7 +1408,7 @@ pub const Arguments = struct {
}
pub fn fromJS(ctx: JSC.C.JSContextRef, arguments: *ArgumentsSlice, exception: JSC.C.ExceptionRef) ?Readdir {
- const path = PathLike.fromJS(ctx, arguments, exception) orelse {
+ const path = PathLike.fromJSWithAllocator(ctx, arguments, bun.default_allocator, exception) orelse {
if (exception.* == null) {
JSC.throwInvalidArguments(
"path must be a string or TypedArray",
@@ -1972,7 +1975,7 @@ pub const Arguments = struct {
}
pub fn fromJS(ctx: JSC.C.JSContextRef, arguments: *ArgumentsSlice, exception: JSC.C.ExceptionRef) ?ReadFile {
- const path = PathOrFileDescriptor.fromJS(ctx, arguments, arguments.arena.allocator(), exception) orelse {
+ const path = PathOrFileDescriptor.fromJS(ctx, arguments, bun.default_allocator, exception) orelse {
if (exception.* == null) {
JSC.throwInvalidArguments(
"path must be a string or a file descriptor",
diff --git a/src/bun.js/node/types.zig b/src/bun.js/node/types.zig
index 81073463a..3ba3db5b9 100644
--- a/src/bun.js/node/types.zig
+++ b/src/bun.js/node/types.zig
@@ -629,6 +629,12 @@ pub const PathLike = union(Tag) {
}
}
+ pub fn toThreadSafe(this: *PathLike) void {
+ if (this.* == .slice_with_underlying_string) {
+ this.slice_with_underlying_string.toThreadSafe();
+ }
+ }
+
pub fn deinitAndUnprotect(this: *const PathLike) void {
switch (this.*) {
.slice_with_underlying_string => |val| {
@@ -1050,6 +1056,12 @@ pub const PathOrFileDescriptor = union(Tag) {
}
}
+ pub fn toThreadSafe(this: *PathOrFileDescriptor) void {
+ if (this.* == .path) {
+ this.path.toThreadSafe();
+ }
+ }
+
pub fn deinitAndUnprotect(this: PathOrFileDescriptor) void {
if (this == .path) {
this.path.deinitAndUnprotect();
diff --git a/src/string.zig b/src/string.zig
index 4fb6e001a..9002234cf 100644
--- a/src/string.zig
+++ b/src/string.zig
@@ -829,6 +829,13 @@ pub const String = extern struct {
return bun.strings.eqlLong(this.byteSlice(), value, true);
}
+ extern fn BunString__toThreadSafe(this: *String) void;
+ pub fn toThreadSafe(this: *String) void {
+ if (this.tag == .WTFStringImpl) {
+ BunString__toThreadSafe(this);
+ }
+ }
+
pub fn eql(this: String, other: String) bool {
return this.toZigString().eql(other.toZigString());
}
@@ -838,6 +845,23 @@ pub const SliceWithUnderlyingString = struct {
utf8: ZigString.Slice,
underlying: String,
+ pub fn toThreadSafe(this: *SliceWithUnderlyingString) void {
+ std.debug.assert(this.underlying.tag == .WTFStringImpl);
+
+ var orig = this.underlying.value.WTFStringImpl;
+ this.underlying.toThreadSafe();
+ if (this.underlying.value.WTFStringImpl != orig) {
+ orig.deref();
+
+ if (this.utf8.allocator.get()) |allocator| {
+ if (String.isWTFAllocator(allocator)) {
+ this.utf8.deinit();
+ this.utf8 = this.underlying.toUTF8(bun.default_allocator);
+ }
+ }
+ }
+ }
+
pub fn deinit(this: SliceWithUnderlyingString) void {
this.utf8.deinit();
this.underlying.deref();
diff --git a/test/js/node/fs/fs.test.ts b/test/js/node/fs/fs.test.ts
index 17c010fb6..86e64b713 100644
--- a/test/js/node/fs/fs.test.ts
+++ b/test/js/node/fs/fs.test.ts
@@ -229,6 +229,39 @@ it("promises.readFile", async () => {
}
});
+it("promises.readFile - UTF16 file path", async () => {
+ const dest = `/tmp/superduperduperdupduperdupersuperduperduperduperduperduperdupersuperduperduperduperduperduperdupersuperduperduperdupe-Bun-👍-${Date.now()}-${
+ (Math.random() * 1024000) | 0
+ }.txt`;
+ await fs.promises.copyFile(import.meta.path, dest);
+ const expected = readFileSync(import.meta.path, "utf-8");
+ Bun.gc(true);
+ for (let i = 0; i < 100; i++) {
+ expect(await fs.promises.readFile(dest, "utf-8")).toEqual(expected);
+ }
+ Bun.gc(true);
+});
+
+it("promises.readFile - atomized file path", async () => {
+ const destInput = `/tmp/superduperduperdupduperdupersuperduperduperduperduperduperdupersuperduperduperduperduperduperdupersuperduperduperdupe-Bun-👍-${Date.now()}-${
+ (Math.random() * 1024000) | 0
+ }.txt`;
+ // Force it to become an atomized string by making it a property access
+ const dest: string = (
+ {
+ [destInput]: destInput,
+ boop: 123,
+ } as const
+ )[destInput] as string;
+ await fs.promises.copyFile(import.meta.path, dest);
+ const expected = readFileSync(import.meta.path, "utf-8");
+ Bun.gc(true);
+ for (let i = 0; i < 100; i++) {
+ expect(await fs.promises.readFile(dest, "utf-8")).toEqual(expected);
+ }
+ Bun.gc(true);
+});
+
it("promises.readFile with buffer as file path", async () => {
for (let i = 0; i < 10; i++)
expect(await fs.promises.readFile(Buffer.from(import.meta.path), "utf-8")).toEqual(