From 22d6227a3a5790ffaf148e732a7a6262d294948b Mon Sep 17 00:00:00 2001 From: Georgijs <48869301+gvilums@users.noreply.github.com> Date: Fri, 12 Apr 2024 13:02:24 -0700 Subject: [PATCH 1/9] fix wrong truncation on `fs.writeFileSync` with fd argument (#10225) * fix wrong truncate * close fd in test * [autofix.ci] apply automated fixes --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> --- src/bun.js/node/node_fs.zig | 4 +++- test/js/node/fs/fs.test.ts | 10 ++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/src/bun.js/node/node_fs.zig b/src/bun.js/node/node_fs.zig index a41af318cfd487..a23d79a1ed8e93 100644 --- a/src/bun.js/node/node_fs.zig +++ b/src/bun.js/node/node_fs.zig @@ -5474,7 +5474,9 @@ pub const NodeFS = struct { } } else { // https://github.com/oven-sh/bun/issues/2931 - if ((@intFromEnum(args.flag) & std.os.O.APPEND) == 0) { + // https://github.com/oven-sh/bun/issues/10222 + // only truncate if we're not appending and writing to a path + if ((@intFromEnum(args.flag) & std.os.O.APPEND) == 0 and args.file != .fd) { _ = ftruncateSync(.{ .fd = fd, .len = @as(JSC.WebCore.Blob.SizeType, @truncate(written)) }); } } diff --git a/test/js/node/fs/fs.test.ts b/test/js/node/fs/fs.test.ts index 7b838229f7d276..5094633a114bb7 100644 --- a/test/js/node/fs/fs.test.ts +++ b/test/js/node/fs/fs.test.ts @@ -2971,3 +2971,13 @@ describe.if(isWindows)("windows path handling", () => { }); } }); + +it("using writeFile on an fd does not truncate it", () => { + const temp = tmpdir(); + const fd = fs.openSync(join(temp, "file.txt"), "w+"); + fs.writeFileSync(fd, "x"); + fs.writeFileSync(fd, "x"); + fs.closeSync(fd); + const content = fs.readFileSync(join(temp, "file.txt"), "utf8"); + expect(content).toBe("xx"); +}); From d785d30eaf2cec259ec5f352815a04d8c509064b Mon Sep 17 00:00:00 2001 From: Georgijs Vilums Date: Fri, 12 Apr 2024 15:52:06 -0700 Subject: [PATCH 2/9] return correct error code on overlong paths --- src/bun.js/node/types.zig | 29 +++++++++-------------------- 1 file changed, 9 insertions(+), 20 deletions(-) diff --git a/src/bun.js/node/types.zig b/src/bun.js/node/types.zig index 8752a92a966c3e..008a9a8c09afeb 100644 --- a/src/bun.js/node/types.zig +++ b/src/bun.js/node/types.zig @@ -926,12 +926,9 @@ pub const Valid = struct { 0...bun.MAX_PATH_BYTES => return true, else => { // TODO: should this be an EINVAL? - JSC.throwInvalidArguments( - comptime std.fmt.comptimePrint("Invalid path string: path is too long (max: {d})", .{bun.MAX_PATH_BYTES}), - .{}, - ctx, - exception, - ); + var system_error = bun.sys.Error.fromCode(.NAMETOOLONG, .open).withPath(zig_str.slice()).toSystemError(); + system_error.syscall = bun.String.dead; + exception.* = system_error.toErrorInstance(ctx).asObjectRef(); return false; }, } @@ -944,12 +941,9 @@ pub const Valid = struct { 0...bun.MAX_PATH_BYTES => return true, else => { // TODO: should this be an EINVAL? - JSC.throwInvalidArguments( - comptime std.fmt.comptimePrint("Invalid path string: path is too long (max: {d})", .{bun.MAX_PATH_BYTES}), - .{}, - ctx, - exception, - ); + var system_error = bun.sys.Error.fromCode(.NAMETOOLONG, .open).toSystemError(); + system_error.syscall = bun.String.dead; + exception.* = system_error.toErrorInstance(ctx).asObjectRef(); return false; }, } @@ -970,14 +964,9 @@ pub const Valid = struct { }, else => { - - // TODO: should this be an EINVAL? - JSC.throwInvalidArguments( - comptime std.fmt.comptimePrint("Invalid path buffer: path is too long (max: {d})", .{bun.MAX_PATH_BYTES}), - .{}, - ctx, - exception, - ); + var system_error = bun.sys.Error.fromCode(.NAMETOOLONG, .open).toSystemError(); + system_error.syscall = bun.String.dead; + exception.* = system_error.toErrorInstance(ctx).asObjectRef(); return false; }, 1...bun.MAX_PATH_BYTES => return true, From 472bd6c7dedafdb52f68c5da86c36ee77757c64d Mon Sep 17 00:00:00 2001 From: Georgijs <48869301+gvilums@users.noreply.github.com> Date: Fri, 12 Apr 2024 17:11:58 -0700 Subject: [PATCH 3/9] Allow fs.close with no callback (#10229) * allow fs.close to only take one argument * add test * fix tests on windows --- src/js/node/fs.js | 12 ++++++++++-- test/js/node/fs/fs.test.ts | 12 +++++++++--- 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/src/js/node/fs.js b/src/js/node/fs.js index c1fa424fd52052..7f24fd8b9cedf7 100644 --- a/src/js/node/fs.js +++ b/src/js/node/fs.js @@ -138,8 +138,16 @@ var access = function access(...args) { appendFile = function appendFile(...args) { callbackify(fs.appendFile, args); }, - close = function close(...args) { - callbackify(fs.close, args); + close = function close(fd, callback) { + if ($isCallable(callback)) { + fs.close(fd).then(() => callback(), callback); + } else if (callback == undefined) { + fs.close(fd).then(() => {}); + } else { + const err = new TypeError("Callback must be a function"); + err.code = "ERR_INVALID_ARG_TYPE"; + throw err; + } }, rm = function rm(...args) { callbackify(fs.rm, args); diff --git a/test/js/node/fs/fs.test.ts b/test/js/node/fs/fs.test.ts index 5094633a114bb7..c2b11cecd4e788 100644 --- a/test/js/node/fs/fs.test.ts +++ b/test/js/node/fs/fs.test.ts @@ -2973,11 +2973,17 @@ describe.if(isWindows)("windows path handling", () => { }); it("using writeFile on an fd does not truncate it", () => { - const temp = tmpdir(); - const fd = fs.openSync(join(temp, "file.txt"), "w+"); + const filepath = join(tmpdir(), `file-${Math.random().toString(32).slice(2)}.txt`); + const fd = fs.openSync(filepath, "w+"); fs.writeFileSync(fd, "x"); fs.writeFileSync(fd, "x"); fs.closeSync(fd); - const content = fs.readFileSync(join(temp, "file.txt"), "utf8"); + const content = fs.readFileSync(filepath, "utf8"); expect(content).toBe("xx"); }); + +it("fs.close with one arg works", () => { + const filepath = join(tmpdir(), `file-${Math.random().toString(32).slice(2)}.txt`); + const fd = fs.openSync(filepath, "w+"); + fs.close(fd); +}); From 1820d08d25aea732c8f2afc6b78b26f2a307c7ea Mon Sep 17 00:00:00 2001 From: Georgijs <48869301+gvilums@users.noreply.github.com> Date: Fri, 12 Apr 2024 17:12:44 -0700 Subject: [PATCH 4/9] fix create with github URL on windows (#10231) * correctly ignore error on windows to match posix behavior * replace zig accessat with bun.sys.existsAt * fix posix build --- src/cli/create_command.zig | 12 +++++++++--- src/sys.zig | 2 +- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/cli/create_command.zig b/src/cli/create_command.zig index 9eed58bf420dda..bd3fa3be3f5033 100644 --- a/src/cli/create_command.zig +++ b/src/cli/create_command.zig @@ -1611,7 +1611,9 @@ pub const CreateCommand = struct { const outdir_path = filesystem.absBuf(&parts, &home_dir_buf); home_dir_buf[outdir_path.len] = 0; const outdir_path_ = home_dir_buf[0..outdir_path.len :0]; - std.fs.accessAbsoluteZ(outdir_path_, .{}) catch break :outer; + if (!bun.sys.existsAt(bun.invalid_fd, outdir_path_)) { + break :outer; + } example_tag = Example.Tag.local_folder; break :brk outdir_path; } @@ -1622,7 +1624,9 @@ pub const CreateCommand = struct { const outdir_path = filesystem.absBuf(&parts, &home_dir_buf); home_dir_buf[outdir_path.len] = 0; const outdir_path_ = home_dir_buf[0..outdir_path.len :0]; - std.fs.accessAbsoluteZ(outdir_path_, .{}) catch break :outer; + if (!bun.sys.existsAt(bun.invalid_fd, outdir_path_)) { + break :outer; + } example_tag = Example.Tag.local_folder; break :brk outdir_path; } @@ -1633,7 +1637,9 @@ pub const CreateCommand = struct { const outdir_path = filesystem.absBuf(&parts, &home_dir_buf); home_dir_buf[outdir_path.len] = 0; const outdir_path_ = home_dir_buf[0..outdir_path.len :0]; - std.fs.accessAbsoluteZ(outdir_path_, .{}) catch break :outer; + if (!bun.sys.existsAt(bun.invalid_fd, outdir_path_)) { + break :outer; + } example_tag = Example.Tag.local_folder; break :brk outdir_path; } diff --git a/src/sys.zig b/src/sys.zig index 33c34857552a73..ba7c690f72288e 100644 --- a/src/sys.zig +++ b/src/sys.zig @@ -2120,7 +2120,7 @@ pub fn exists(path: []const u8) bool { pub fn existsAt(fd: bun.FileDescriptor, subpath: []const u8) bool { if (comptime Environment.isPosix) { - return system.faccessat(bun.toFD(fd), &(std.os.toPosixPath(subpath) catch return false), 0, 0) == 0; + return system.faccessat(fd.cast(), &(std.os.toPosixPath(subpath) catch return false), 0, 0) == 0; } if (comptime Environment.isWindows) { From 176af5cf583b196fa32ad1f66c3bf6394e8971a4 Mon Sep 17 00:00:00 2001 From: Dylan Conway <35280289+dylan-conway@users.noreply.github.com> Date: Fri, 12 Apr 2024 19:02:45 -0700 Subject: [PATCH 5/9] reachable errors (#10190) Co-authored-by: Jarred Sumner --- src/cli/create_command.zig | 2 +- src/deps/zig | 2 +- src/libarchive/libarchive.zig | 4 ++-- test/cli/install/bun-create.test.ts | 22 +++++++++++++++++++++- 4 files changed, 25 insertions(+), 5 deletions(-) diff --git a/src/cli/create_command.zig b/src/cli/create_command.zig index bd3fa3be3f5033..22bc494ae7352f 100644 --- a/src/cli/create_command.zig +++ b/src/cli/create_command.zig @@ -378,7 +378,7 @@ pub const CreateCommand = struct { progress.refresh(); var pluckers: [1]Archive.Plucker = if (!create_options.skip_package_json) - [1]Archive.Plucker{try Archive.Plucker.init("package.json", 2048, ctx.allocator)} + [1]Archive.Plucker{try Archive.Plucker.init(comptime strings.literal(bun.OSPathChar, "package.json"), 2048, ctx.allocator)} else [1]Archive.Plucker{undefined}; diff --git a/src/deps/zig b/src/deps/zig index 7fe33d94eaeb1a..593a407f121a28 160000 --- a/src/deps/zig +++ b/src/deps/zig @@ -1 +1 @@ -Subproject commit 7fe33d94eaeb1af7705e9c5f43a3b243aa895436 +Subproject commit 593a407f121a2870e9c645da33c11db5e4331920 diff --git a/src/libarchive/libarchive.zig b/src/libarchive/libarchive.zig index c6ddb0c738bce2..a1c565c44c9043 100644 --- a/src/libarchive/libarchive.zig +++ b/src/libarchive/libarchive.zig @@ -376,10 +376,10 @@ pub const Archive = struct { filename_hash: u64 = 0, found: bool = false, fd: FileDescriptorType = .zero, - pub fn init(filepath: string, estimated_size: usize, allocator: std.mem.Allocator) !Plucker { + pub fn init(filepath: bun.OSPathSlice, estimated_size: usize, allocator: std.mem.Allocator) !Plucker { return Plucker{ .contents = try MutableString.init(allocator, estimated_size), - .filename_hash = bun.hash(filepath), + .filename_hash = bun.hash(std.mem.sliceAsBytes(filepath)), .fd = .zero, .found = false, }; diff --git a/test/cli/install/bun-create.test.ts b/test/cli/install/bun-create.test.ts index 49162855f8c909..a8515c1d5d744c 100644 --- a/test/cli/install/bun-create.test.ts +++ b/test/cli/install/bun-create.test.ts @@ -1,7 +1,7 @@ import { spawn, spawnSync } from "bun"; import { afterEach, beforeEach, expect, it, describe } from "bun:test"; import { bunExe, bunEnv as env } from "harness"; -import { mkdtemp, realpath, rm, mkdir, stat } from "fs/promises"; +import { mkdtemp, realpath, rm, mkdir, stat, exists } from "fs/promises"; import { tmpdir } from "os"; import { join } from "path"; @@ -100,3 +100,23 @@ it("should create template from local folder", async () => { const dirStat = await stat(`${x_dir}/${testTemplate}`); expect(dirStat.isDirectory()).toBe(true); }); + +for (const repo of ["https://github.com/dylan-conway/create-test", "github.com/dylan-conway/create-test"]) { + it(`should create and install github template from ${repo}`, async () => { + const { stderr, stdout, exited } = spawn({ + cmd: [bunExe(), "create", repo], + cwd: x_dir, + stdout: "pipe", + stderr: "pipe", + env, + }); + + const err = await Bun.readableStreamToText(stderr); + expect(err).not.toContain("error:"); + const out = await Bun.readableStreamToText(stdout); + expect(out).toContain("Success! dylan-conway/create-test loaded into create-test"); + expect(await exists(join(x_dir, "create-test", "node_modules", "jquery"))).toBe(true); + + expect(await exited).toBe(0); + }); +} From 4627af5893ded1fbd7bdfeed0dab9077be731ead Mon Sep 17 00:00:00 2001 From: Ciro Spaciari Date: Fri, 12 Apr 2024 23:58:13 -0300 Subject: [PATCH 6/9] fix(stream) fix http body-stream sending duplicate data (#10221) * some fixes * cleanup * more complete test * fix test + use same server * opsie * incremental steps --- src/bun.js/webcore/streams.zig | 21 ++++------ test/js/node/http/node-http.test.ts | 59 +++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 14 deletions(-) diff --git a/src/bun.js/webcore/streams.zig b/src/bun.js/webcore/streams.zig index 11cfadf62cbdb5..0ef0e1c315b3b6 100644 --- a/src/bun.js/webcore/streams.zig +++ b/src/bun.js/webcore/streams.zig @@ -1964,6 +1964,7 @@ pub fn HTTPServerWritable(comptime ssl: bool) type { } fn handleWrote(this: *@This(), amount1: usize) void { + defer log("handleWrote: {d} offset: {d}, {d}", .{ amount1, this.offset, this.buffer.len }); const amount = @as(Blob.SizeType, @truncate(amount1)); this.offset += amount; this.wrote += amount; @@ -1996,8 +1997,11 @@ pub fn HTTPServerWritable(comptime ssl: bool) type { if (this.requested_end and !this.res.state().isHttpWriteCalled()) { this.handleFirstWriteIfNecessary(); const success = this.res.tryEnd(buf, this.end_len, false); - this.has_backpressure = !success; - if (this.has_backpressure) { + if (success) { + this.has_backpressure = false; + this.handleWrote(this.end_len); + } else { + this.has_backpressure = true; this.res.onWritable(*@This(), onWritable, this); } return success; @@ -2018,7 +2022,7 @@ pub fn HTTPServerWritable(comptime ssl: bool) type { } else { this.has_backpressure = !this.res.write(buf); } - + this.handleWrote(buf.len); return true; } @@ -2064,7 +2068,6 @@ pub fn HTTPServerWritable(comptime ssl: bool) type { // if we were unable to send it, retry return false; } - this.handleWrote(@as(Blob.SizeType, @truncate(chunk.len))); total_written = chunk.len; if (this.requested_end) { @@ -2150,7 +2153,6 @@ pub fn HTTPServerWritable(comptime ssl: bool) type { const success = this.send(slice); if (success) { - this.handleWrote(@as(Blob.SizeType, @truncate(slice.len))); return .{ .result = JSValue.jsNumber(slice.len) }; } @@ -2178,7 +2180,6 @@ pub fn HTTPServerWritable(comptime ssl: bool) type { assert(slice.len > 0); const success = this.send(slice); if (success) { - this.handleWrote(@as(Blob.SizeType, @truncate(slice.len))); return .{ .result = JSC.JSPromise.resolvedPromiseValue(globalThis, JSValue.jsNumber(slice.len)) }; } } @@ -2221,7 +2222,6 @@ pub fn HTTPServerWritable(comptime ssl: bool) type { // - large-ish chunk // - no backpressure if (this.send(bytes)) { - this.handleWrote(len); return .{ .owned = len }; } @@ -2236,7 +2236,6 @@ pub fn HTTPServerWritable(comptime ssl: bool) type { }; const slice = this.readableSlice(); if (this.send(slice)) { - this.handleWrote(slice.len); return .{ .owned = len }; } } else { @@ -2274,7 +2273,6 @@ pub fn HTTPServerWritable(comptime ssl: bool) type { // - large-ish chunk // - no backpressure if (this.send(bytes)) { - this.handleWrote(bytes.len); return .{ .owned = len }; } do_send = false; @@ -2286,7 +2284,6 @@ pub fn HTTPServerWritable(comptime ssl: bool) type { if (do_send) { if (this.send(this.readableSlice())) { - this.handleWrote(bytes.len); return .{ .owned = len }; } } @@ -2299,7 +2296,6 @@ pub fn HTTPServerWritable(comptime ssl: bool) type { }; const readable = this.readableSlice(); if (this.send(readable)) { - this.handleWrote(readable.len); return .{ .owned = len }; } } else { @@ -2336,7 +2332,6 @@ pub fn HTTPServerWritable(comptime ssl: bool) type { const readable = this.readableSlice(); if (readable.len >= this.highWaterMark or this.hasBackpressure()) { if (this.send(readable)) { - this.handleWrote(readable.len); return .{ .owned = @as(Blob.SizeType, @intCast(written)) }; } } @@ -2464,8 +2459,6 @@ pub fn HTTPServerWritable(comptime ssl: bool) type { this.auto_flusher.registered = true; return true; } - - this.handleWrote(readable.len); this.auto_flusher.registered = false; return false; } diff --git a/test/js/node/http/node-http.test.ts b/test/js/node/http/node-http.test.ts index f4199cd206cf78..17f7d9d546ed74 100644 --- a/test/js/node/http/node-http.test.ts +++ b/test/js/node/http/node-http.test.ts @@ -1746,3 +1746,62 @@ if (process.platform !== "win32") { expect([joinPath(import.meta.dir, "node-http-ref-fixture.js")]).toRun(); }); } + +it("#10177 response.write with non-ascii latin1 should not cause duplicated character or segfault", done => { + // x = ascii + // รก = latin1 supplementary character + // ๐Ÿ“™ = emoji + // ๐Ÿ‘๐Ÿฝ = its a grapheme of ๐Ÿ‘ ๐ŸŸค + // "\u{1F600}" = utf16 + const chars = ["x", "รก", "๐Ÿ“™", "๐Ÿ‘๐Ÿฝ", "\u{1F600}"]; + + // 128 = small than waterMark, 256 = waterMark, 1024 = large than waterMark + // 8Kb = small than cork buffer + // 16Kb = cork buffer + // 32Kb = large than cork buffer + const start_size = 128; + const increment_step = 1024; + const end_size = 32 * 1024; + let expected = ""; + + function finish(err) { + server.closeAllConnections(); + Bun.gc(true); + done(err); + } + const server = require("http") + .createServer((_, response) => { + response.write(expected); + response.write(""); + response.end(); + }) + .listen(0, "localhost", async (err, hostname, port) => { + expect(err).toBeFalsy(); + expect(port).toBeGreaterThan(0); + + for (const char of chars) { + for (let size = start_size; size <= end_size; size += increment_step) { + expected = char + "-".repeat(size) + "x"; + + try { + const url = `http://${hostname}:${port}`; + const count = 20; + const all = []; + const batchSize = 20; + while (all.length < count) { + const batch = Array.from({ length: batchSize }, () => fetch(url).then(a => a.text())); + + all.push(...(await Promise.all(batch))); + } + + for (const result of all) { + expect(result).toBe(expected); + } + } catch (err) { + return finish(err); + } + } + } + finish(); + }); +}); From 65d8288d81f357098727c6e833cd164642d78bbd Mon Sep 17 00:00:00 2001 From: Jarred Sumner Date: Fri, 12 Apr 2024 20:55:51 -0700 Subject: [PATCH 7/9] Revert "fix create with github URL on windows (#10231)" (#10236) This reverts commit 1820d08d25aea732c8f2afc6b78b26f2a307c7ea. --- src/cli/create_command.zig | 12 +++--------- src/sys.zig | 2 +- 2 files changed, 4 insertions(+), 10 deletions(-) diff --git a/src/cli/create_command.zig b/src/cli/create_command.zig index 22bc494ae7352f..0e5fffff14971b 100644 --- a/src/cli/create_command.zig +++ b/src/cli/create_command.zig @@ -1611,9 +1611,7 @@ pub const CreateCommand = struct { const outdir_path = filesystem.absBuf(&parts, &home_dir_buf); home_dir_buf[outdir_path.len] = 0; const outdir_path_ = home_dir_buf[0..outdir_path.len :0]; - if (!bun.sys.existsAt(bun.invalid_fd, outdir_path_)) { - break :outer; - } + std.fs.accessAbsoluteZ(outdir_path_, .{}) catch break :outer; example_tag = Example.Tag.local_folder; break :brk outdir_path; } @@ -1624,9 +1622,7 @@ pub const CreateCommand = struct { const outdir_path = filesystem.absBuf(&parts, &home_dir_buf); home_dir_buf[outdir_path.len] = 0; const outdir_path_ = home_dir_buf[0..outdir_path.len :0]; - if (!bun.sys.existsAt(bun.invalid_fd, outdir_path_)) { - break :outer; - } + std.fs.accessAbsoluteZ(outdir_path_, .{}) catch break :outer; example_tag = Example.Tag.local_folder; break :brk outdir_path; } @@ -1637,9 +1633,7 @@ pub const CreateCommand = struct { const outdir_path = filesystem.absBuf(&parts, &home_dir_buf); home_dir_buf[outdir_path.len] = 0; const outdir_path_ = home_dir_buf[0..outdir_path.len :0]; - if (!bun.sys.existsAt(bun.invalid_fd, outdir_path_)) { - break :outer; - } + std.fs.accessAbsoluteZ(outdir_path_, .{}) catch break :outer; example_tag = Example.Tag.local_folder; break :brk outdir_path; } diff --git a/src/sys.zig b/src/sys.zig index ba7c690f72288e..33c34857552a73 100644 --- a/src/sys.zig +++ b/src/sys.zig @@ -2120,7 +2120,7 @@ pub fn exists(path: []const u8) bool { pub fn existsAt(fd: bun.FileDescriptor, subpath: []const u8) bool { if (comptime Environment.isPosix) { - return system.faccessat(fd.cast(), &(std.os.toPosixPath(subpath) catch return false), 0, 0) == 0; + return system.faccessat(bun.toFD(fd), &(std.os.toPosixPath(subpath) catch return false), 0, 0) == 0; } if (comptime Environment.isWindows) { From f6b9c0c909702f44e3b16f2cec1bc30c5195448f Mon Sep 17 00:00:00 2001 From: Ciro Spaciari Date: Sat, 13 Apr 2024 00:58:45 -0300 Subject: [PATCH 8/9] fix(socket) fix error in case of failure/returning a error in the open handler (#10154) * fix socket * one more test * always clean callback on deinit * Update src/bun.js/api/bun/socket.zig Co-authored-by: Jarred Sumner * make context close private * keep old logic * move clean step to SocketContext.close * add comment * wait for close on stop * cleanup --------- Co-authored-by: Jarred Sumner --- src/bun.js/api/bun/socket.zig | 25 ++++++---- src/deps/uws.zig | 43 +++++++++++++++++ test/js/bun/net/socket.test.ts | 87 +++++++++++++++++++++++++++++++++- 3 files changed, 144 insertions(+), 11 deletions(-) diff --git a/src/bun.js/api/bun/socket.zig b/src/bun.js/api/bun/socket.zig index 835052d680b19f..b8c570acda5657 100644 --- a/src/bun.js/api/bun/socket.zig +++ b/src/bun.js/api/bun/socket.zig @@ -218,8 +218,9 @@ const Handlers = struct { this.unprotect(); // will deinit when is not wrapped or when is the TCP wrapped connection if (wrapped != .tls) { - if (ctx) |ctx_| + if (ctx) |ctx_| { ctx_.deinit(ssl); + } } bun.default_allocator.destroy(this); } @@ -825,23 +826,27 @@ pub const Listener = struct { const arguments = callframe.arguments(1); log("close", .{}); - if (arguments.len > 0 and arguments.ptr[0].isBoolean() and arguments.ptr[0].toBoolean() and this.socket_context != null) { - this.socket_context.?.close(this.ssl); - this.listener = null; - } else { - var listener = this.listener orelse return JSValue.jsUndefined(); - this.listener = null; - listener.close(this.ssl); - } + var listener = this.listener orelse return JSValue.jsUndefined(); + this.listener = null; this.poll_ref.unref(this.handlers.vm); + // if we already have no active connections, we can deinit the context now if (this.handlers.active_connections == 0) { this.handlers.unprotect(); - this.socket_context.?.close(this.ssl); + // deiniting the context will also close the listener this.socket_context.?.deinit(this.ssl); this.socket_context = null; this.strong_self.clear(); this.strong_data.clear(); + } else { + const forceClose = arguments.len > 0 and arguments.ptr[0].isBoolean() and arguments.ptr[0].toBoolean() and this.socket_context != null; + if (forceClose) { + // close all connections in this context and wait for them to close + this.socket_context.?.close(this.ssl); + } else { + // only close the listener and wait for the connections to close by it self + listener.close(this.ssl); + } } return JSValue.jsUndefined(); diff --git a/src/deps/uws.zig b/src/deps/uws.zig index 386934c50d2dd8..e1fa199c5b411c 100644 --- a/src/deps/uws.zig +++ b/src/deps/uws.zig @@ -893,6 +893,47 @@ pub const SocketContext = opaque { us_socket_context_free(@as(i32, 0), this); } + pub fn cleanCallbacks(ctx: *SocketContext, is_ssl: bool) void { + const ssl_int: i32 = @intFromBool(is_ssl); + // replace callbacks with dummy ones + const DummyCallbacks = struct { + fn open(socket: *Socket, _: i32, _: [*c]u8, _: i32) callconv(.C) ?*Socket { + return socket; + } + fn close(socket: *Socket, _: i32, _: ?*anyopaque) callconv(.C) ?*Socket { + return socket; + } + fn data(socket: *Socket, _: [*c]u8, _: i32) callconv(.C) ?*Socket { + return socket; + } + fn writable(socket: *Socket) callconv(.C) ?*Socket { + return socket; + } + fn timeout(socket: *Socket) callconv(.C) ?*Socket { + return socket; + } + fn connect_error(socket: *Socket, _: i32) callconv(.C) ?*Socket { + return socket; + } + fn end(socket: *Socket) callconv(.C) ?*Socket { + return socket; + } + fn handshake(_: *Socket, _: i32, _: us_bun_verify_error_t, _: ?*anyopaque) callconv(.C) void {} + fn long_timeout(socket: *Socket) callconv(.C) ?*Socket { + return socket; + } + }; + us_socket_context_on_open(ssl_int, ctx, DummyCallbacks.open); + us_socket_context_on_close(ssl_int, ctx, DummyCallbacks.close); + us_socket_context_on_data(ssl_int, ctx, DummyCallbacks.data); + us_socket_context_on_writable(ssl_int, ctx, DummyCallbacks.writable); + us_socket_context_on_timeout(ssl_int, ctx, DummyCallbacks.timeout); + us_socket_context_on_connect_error(ssl_int, ctx, DummyCallbacks.connect_error); + us_socket_context_on_end(ssl_int, ctx, DummyCallbacks.end); + us_socket_context_on_handshake(ssl_int, ctx, DummyCallbacks.handshake, null); + us_socket_context_on_long_timeout(ssl_int, ctx, DummyCallbacks.long_timeout); + } + fn getLoop(this: *SocketContext, ssl: bool) ?*Loop { if (ssl) { return us_socket_context_loop(@as(i32, 1), this); @@ -902,6 +943,8 @@ pub const SocketContext = opaque { /// closes and deinit the SocketContexts pub fn deinit(this: *SocketContext, ssl: bool) void { + // we clean the callbacks to avoid UAF because we are deiniting + this.cleanCallbacks(ssl); this.close(ssl); //always deinit in next iteration if (ssl) { diff --git a/test/js/bun/net/socket.test.ts b/test/js/bun/net/socket.test.ts index af131410fa6800..c46a46b43950e3 100644 --- a/test/js/bun/net/socket.test.ts +++ b/test/js/bun/net/socket.test.ts @@ -1,7 +1,7 @@ import { expect, it } from "bun:test"; import { bunEnv, bunExe, expectMaxObjectTypeCount, isWindows } from "harness"; import { connect, fileURLToPath, SocketHandler, spawn } from "bun"; - +import type { Socket } from "bun"; it("should coerce '0' to 0", async () => { const listener = Bun.listen({ // @ts-expect-error @@ -271,3 +271,88 @@ it("socket.timeout works", async () => { it("should allow large amounts of data to be sent and received", async () => { expect([fileURLToPath(new URL("./socket-huge-fixture.js", import.meta.url))]).toRun(); }, 60_000); + +it("it should not crash when getting a ReferenceError on client socket open", async () => { + const server = Bun.serve({ + port: 8080, + hostname: "localhost", + fetch() { + return new Response("Hello World"); + }, + }); + try { + const { resolve, reject, promise } = Promise.withResolvers(); + let client: Socket | null = null; + const timeout = setTimeout(() => { + client?.end(); + reject(new Error("Timeout")); + }, 1000); + client = await Bun.connect({ + port: server.port, + hostname: server.hostname, + socket: { + open(socket) { + // ReferenceError: Can't find variable: bytes + // @ts-expect-error + socket.write(bytes); + }, + error(socket, error) { + clearTimeout(timeout); + resolve(error); + }, + close(socket) { + // we need the close handler + resolve({ message: "Closed" }); + }, + data(socket, data) {}, + }, + }); + + const result: any = await promise; + expect(result?.message).toBe("Can't find variable: bytes"); + } finally { + server.stop(true); + } +}); + +it("it should not crash when returning a Error on client socket open", async () => { + const server = Bun.serve({ + port: 8080, + hostname: "localhost", + fetch() { + return new Response("Hello World"); + }, + }); + try { + const { resolve, reject, promise } = Promise.withResolvers(); + let client: Socket | null = null; + const timeout = setTimeout(() => { + client?.end(); + reject(new Error("Timeout")); + }, 1000); + client = await Bun.connect({ + port: server.port, + hostname: server.hostname, + socket: { + //@ts-ignore + open(socket) { + return new Error("CustomError"); + }, + error(socket, error) { + clearTimeout(timeout); + resolve(error); + }, + close(socket) { + // we need the close handler + resolve({ message: "Closed" }); + }, + data(socket, data) {}, + }, + }); + + const result: any = await promise; + expect(result?.message).toBe("CustomError"); + } finally { + server.stop(true); + } +}); From 8d49a3ee37f2803ea78f13ed8418705d10befd6f Mon Sep 17 00:00:00 2001 From: Jarred Sumner Date: Fri, 12 Apr 2024 22:19:29 -0700 Subject: [PATCH 9/9] Better way to check if a directory exists (#10235) * Better way to check if a directory exists * Update sys.zig * Fix windows build * Add missing file --- src/bun.js/node/types.zig | 8 +++-- src/install/install.zig | 5 +-- src/sys.zig | 67 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 74 insertions(+), 6 deletions(-) diff --git a/src/bun.js/node/types.zig b/src/bun.js/node/types.zig index 008a9a8c09afeb..894e99a5ff4674 100644 --- a/src/bun.js/node/types.zig +++ b/src/bun.js/node/types.zig @@ -248,7 +248,9 @@ pub fn Maybe(comptime ReturnTypeT: type, comptime ErrorTypeT: type) type { pub inline fn errnoSysFd(rc: anytype, syscall: Syscall.Tag, fd: bun.FileDescriptor) ?@This() { if (comptime Environment.isWindows) { - if (rc != 0) return null; + if (comptime @TypeOf(rc) == std.os.windows.NTSTATUS) {} else { + if (rc != 0) return null; + } } return switch (Syscall.getErrno(rc)) { .SUCCESS => null, @@ -268,7 +270,9 @@ pub fn Maybe(comptime ReturnTypeT: type, comptime ErrorTypeT: type) type { @compileError("Do not pass WString path to errnoSysP, it needs the path encoded as utf8"); } if (comptime Environment.isWindows) { - if (rc != 0) return null; + if (comptime @TypeOf(rc) == std.os.windows.NTSTATUS) {} else { + if (rc != 0) return null; + } } return switch (Syscall.getErrno(rc)) { .SUCCESS => null, diff --git a/src/install/install.zig b/src/install/install.zig index c1443a31703410..aec380fac5d1eb 100644 --- a/src/install/install.zig +++ b/src/install/install.zig @@ -3338,10 +3338,7 @@ pub const PackageManager = struct { } pub fn isFolderInCache(this: *PackageManager, folder_path: stringZ) bool { - // TODO: is this slow? - var dir = this.getCacheDirectory().openDirZ(folder_path, .{}) catch return false; - dir.close(); - return true; + return bun.sys.directoryExistsAt(this.getCacheDirectory(), folder_path).unwrap() catch false; } pub fn pathForCachedNPMPath( diff --git a/src/sys.zig b/src/sys.zig index 33c34857552a73..270369fb97cc24 100644 --- a/src/sys.zig +++ b/src/sys.zig @@ -2118,6 +2118,73 @@ pub fn exists(path: []const u8) bool { @compileError("TODO: existsOSPath"); } +pub fn directoryExistsAt(dir_: anytype, subpath: anytype) JSC.Maybe(bool) { + const has_sentinel = std.meta.sentinel(@TypeOf(subpath)) != null; + const dir_fd = bun.toFD(dir_); + if (comptime Environment.isWindows) { + var wbuf: bun.WPathBuffer = undefined; + const path = bun.strings.toNTPath(&wbuf, subpath); + const path_len_bytes: u16 = @truncate(path.len * 2); + var nt_name = w.UNICODE_STRING{ + .Length = path_len_bytes, + .MaximumLength = path_len_bytes, + .Buffer = @constCast(path.ptr), + }; + var attr = w.OBJECT_ATTRIBUTES{ + .Length = @sizeOf(w.OBJECT_ATTRIBUTES), + .RootDirectory = if (std.fs.path.isAbsoluteWindowsWTF16(path)) + null + else if (dir_fd == bun.invalid_fd) + std.fs.cwd().fd + else + dir_fd.cast(), + .Attributes = 0, // Note we do not use OBJ_CASE_INSENSITIVE here. + .ObjectName = &nt_name, + .SecurityDescriptor = null, + .SecurityQualityOfService = null, + }; + var basic_info: w.FILE_BASIC_INFORMATION = undefined; + const rc = kernel32.NtQueryAttributesFile(&attr, &basic_info); + if (JSC.Maybe(bool).errnoSysP(rc, .access, subpath)) |err| { + syslog("NtQueryAttributesFile({}, {}, O_DIRECTORY | O_RDONLY, 0) = {}", .{ dir_fd, bun.fmt.fmtOSPath(path, .{}), err }); + return err; + } + + const is_dir = basic_info.FileAttributes != kernel32.INVALID_FILE_ATTRIBUTES and + basic_info.FileAttributes & kernel32.FILE_ATTRIBUTE_DIRECTORY != 0; + syslog("NtQueryAttributesFile({}, {}, O_DIRECTORY | O_RDONLY, 0) = {d}", .{ dir_fd, bun.fmt.fmtOSPath(path, .{}), @intFromBool(is_dir) }); + + return .{ + .result = is_dir, + }; + } + + if (comptime !has_sentinel) { + const path = std.os.toPosixPath(subpath) catch return JSC.Maybe(bool){ .err = Error.oom }; + return directoryExistsAt(dir_fd, path); + } + + if (comptime Environment.isLinux) { + // avoid loading the libc symbol for this to reduce chances of GLIBC minimum version requirements + const rc = linux.faccessat(dir_fd.cast(), subpath, linux.O.DIRECTORY | linux.O.RDONLY, 0); + syslog("faccessat({}, {}, O_DIRECTORY | O_RDONLY, 0) = {d}", .{ dir_fd, bun.fmt.fmtOSPath(subpath, .{}), rc }); + if (rc == 0) { + return JSC.Maybe(bool){ .result = true }; + } + + return JSC.Maybe(bool){ .result = false }; + } + + // on toher platforms use faccessat from libc + const rc = std.c.faccessat(dir_fd.cast(), subpath, std.os.O.DIRECTORY | std.os.O.RDONLY, 0); + syslog("faccessat({}, {}, O_DIRECTORY | O_RDONLY, 0) = {d}", .{ dir_fd, bun.fmt.fmtOSPath(subpath, .{}), rc }); + if (rc == 0) { + return JSC.Maybe(bool){ .result = true }; + } + + return JSC.Maybe(bool){ .result = false }; +} + pub fn existsAt(fd: bun.FileDescriptor, subpath: []const u8) bool { if (comptime Environment.isPosix) { return system.faccessat(bun.toFD(fd), &(std.os.toPosixPath(subpath) catch return false), 0, 0) == 0;