Skip to content
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

fs.mkdir empty string bugfix #14510

Merged
merged 18 commits into from
Oct 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 11 additions & 16 deletions src/bun.js/node/node_fs.zig
Original file line number Diff line number Diff line change
Expand Up @@ -4857,21 +4857,11 @@ pub const NodeFS = struct {
pub fn mkdirRecursiveImpl(this: *NodeFS, args: Arguments.Mkdir, comptime flavor: Flavor, comptime Ctx: type, ctx: Ctx) Maybe(Return.Mkdir) {
_ = flavor;
var buf: bun.OSPathBuffer = undefined;
const path: bun.OSPathSliceZ = if (!Environment.isWindows)
args.path.osPath(&buf)
else brk: {
// TODO(@paperdave): clean this up a lot.
var joined_buf: bun.PathBuffer = undefined;
if (std.fs.path.isAbsolute(args.path.slice())) {
const utf8 = PosixToWinNormalizer.resolveCWDWithExternalBufZ(&joined_buf, args.path.slice()) catch
return .{ .err = .{ .errno = @intFromEnum(C.SystemErrno.ENOMEM), .syscall = .getcwd } };
break :brk strings.toWPath(&buf, utf8);
} else {
var cwd_buf: bun.PathBuffer = undefined;
const cwd = std.posix.getcwd(&cwd_buf) catch return .{ .err = .{ .errno = @intFromEnum(C.SystemErrno.ENOMEM), .syscall = .getcwd } };
break :brk strings.toWPath(&buf, bun.path.joinAbsStringBuf(cwd, &joined_buf, &.{args.path.slice()}, .windows));
}
};
const path: bun.OSPathSliceZ = if (Environment.isWindows)
strings.toNTPath(&buf, args.path.slice())
else
args.path.osPath(&buf);

// TODO: remove and make it always a comptime argument
return switch (args.always_return_none) {
inline else => |always_return_none| this.mkdirRecursiveOSPathImpl(Ctx, ctx, path, args.mode, !always_return_none),
Expand Down Expand Up @@ -4921,7 +4911,12 @@ pub const NodeFS = struct {
return .{ .result = .{ .none = {} } };
},
// continue
.NOENT => {},
.NOENT => {
if (len == 0) {
// no path to copy
return .{ .err = err };
}
},
}
},
.result => {
Expand Down
1 change: 0 additions & 1 deletion src/sys.zig
Original file line number Diff line number Diff line change
Expand Up @@ -729,7 +729,6 @@ pub fn mkdirOSPath(file_path: bun.OSPathSliceZ, flags: bun.Mode) Maybe(void) {
return switch (Environment.os) {
else => mkdir(file_path, flags),
.windows => {
assertIsValidWindowsPath(bun.OSPathChar, file_path);
const rc = kernel32.CreateDirectoryW(file_path, null);

if (Maybe(void).errnoSys(
Expand Down
8 changes: 2 additions & 6 deletions test/cli/install/bun-install.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8210,12 +8210,8 @@ it("should ensure read permissions of all extracted files", async () => {

await runBunInstall(env, package_dir);

expect((await stat(join(package_dir, "node_modules", "pkg-only-owner", "package.json"))).mode & 0o666).toBe(
isWindows ? 0o666 : 0o644,
);
expect((await stat(join(package_dir, "node_modules", "pkg-only-owner", "src", "index.js"))).mode & 0o666).toBe(
isWindows ? 0o666 : 0o644,
);
expect((await stat(join(package_dir, "node_modules", "pkg-only-owner", "package.json"))).mode & 0o444).toBe(0o444);
expect((await stat(join(package_dir, "node_modules", "pkg-only-owner", "src", "index.js"))).mode & 0o444).toBe(0o444);
});

it("should handle @scoped name that contains tilde, issue#7045", async () => {
Expand Down
3 changes: 2 additions & 1 deletion test/cli/run/log-test.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { spawnSync } from "bun";
import { expect, it } from "bun:test";
import * as fs from "fs";
import { bunEnv, bunExe } from "harness";
import { dirname, join } from "path";
import { dirname, join, resolve } from "path";

it("should not log .env when quiet", async () => {
writeDirectoryTree("/tmp/log-test-silent", {
Expand Down Expand Up @@ -36,6 +36,7 @@ it("should log .env by default", async () => {
});

function writeDirectoryTree(base: string, paths: Record<string, any>) {
base = resolve(base);
for (const path of Object.keys(paths)) {
const content = paths[path];
const joined = join(base, path);
Expand Down
71 changes: 41 additions & 30 deletions test/js/node/fs/fs.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,19 @@ function mkdirForce(path: string) {
if (!existsSync(path)) mkdirSync(path, { recursive: true });
}

function tmpdirTestMkdir(): string {
const now = Date.now().toString();
const tempdir = `${tmpdir()}/fs.test.ts/${now}/1234/hi`;
expect(existsSync(tempdir)).toBe(false);
const res = mkdirSync(tempdir, { recursive: true });
if (!res?.includes(now)) {
expect(res).toInclude("fs.test.ts");
}
expect(res).not.toInclude("1234");
expect(existsSync(tempdir)).toBe(true);
return tempdir;
}

it("fs.writeFile(1, data) should work when its inherited", async () => {
expect([join(import.meta.dir, "fs-writeFile-1-fixture.js"), "1"]).toRun();
});
Expand Down Expand Up @@ -315,9 +328,7 @@ it("writeFileSync NOT in append SHOULD truncate the file", () => {

describe("copyFileSync", () => {
it("should work for files < 128 KB", () => {
const tempdir = `${tmpdir()}/fs.test.js/${Date.now()}/1234/hi`;
expect(existsSync(tempdir)).toBe(false);
expect(tempdir.includes(mkdirSync(tempdir, { recursive: true })!)).toBe(true);
const tempdir = tmpdirTestMkdir();

// that don't exist
copyFileSync(import.meta.path, tempdir + "/copyFileSync.js");
Expand All @@ -333,9 +344,7 @@ describe("copyFileSync", () => {
});

it("should work for files > 128 KB ", () => {
const tempdir = `${tmpdir()}/fs.test.js/${Date.now()}-1/1234/hi`;
expect(existsSync(tempdir)).toBe(false);
expect(tempdir.includes(mkdirSync(tempdir, { recursive: true })!)).toBe(true);
const tempdir = tmpdirTestMkdir();
var buffer = new Int32Array(128 * 1024);
for (let i = 0; i < buffer.length; i++) {
buffer[i] = i % 256;
Expand All @@ -362,9 +371,7 @@ describe("copyFileSync", () => {
});

it("FICLONE option does not error ever", () => {
const tempdir = `${tmpdir()}/fs.test.js/${Date.now()}.FICLONE/1234/hi`;
expect(existsSync(tempdir)).toBe(false);
expect(tempdir.includes(mkdirSync(tempdir, { recursive: true })!)).toBe(true);
const tempdir = tmpdirTestMkdir();

// that don't exist
copyFileSync(import.meta.path, tempdir + "/copyFileSync.js", fs.constants.COPYFILE_FICLONE);
Expand All @@ -373,9 +380,7 @@ describe("copyFileSync", () => {
});

it("COPYFILE_EXCL works", () => {
const tempdir = `${tmpdir()}/fs.test.js/${Date.now()}.COPYFILE_EXCL/1234/hi`;
expect(existsSync(tempdir)).toBe(false);
expect(tempdir.includes(mkdirSync(tempdir, { recursive: true })!)).toBe(true);
const tempdir = tmpdirTestMkdir();

// that don't exist
copyFileSync(import.meta.path, tempdir + "/copyFileSync.js", fs.constants.COPYFILE_EXCL);
Expand All @@ -387,9 +392,7 @@ describe("copyFileSync", () => {
if (process.platform === "linux") {
describe("should work when copyFileRange is not available", () => {
it("on large files", () => {
const tempdir = `${tmpdir()}/fs.test.js/${Date.now()}-1/1234/large`;
expect(existsSync(tempdir)).toBe(false);
expect(tempdir.includes(mkdirSync(tempdir, { recursive: true })!)).toBe(true);
const tempdir = tmpdirTestMkdir();
var buffer = new Int32Array(128 * 1024);
for (let i = 0; i < buffer.length; i++) {
buffer[i] = i % 256;
Expand Down Expand Up @@ -421,9 +424,7 @@ describe("copyFileSync", () => {
});

it("on small files", () => {
const tempdir = `${tmpdir()}/fs.test.js/${Date.now()}-1/1234/small`;
expect(existsSync(tempdir)).toBe(false);
expect(tempdir.includes(mkdirSync(tempdir, { recursive: true })!)).toBe(true);
const tempdir = tmpdirTestMkdir();
var buffer = new Int32Array(1 * 1024);
for (let i = 0; i < buffer.length; i++) {
buffer[i] = i % 256;
Expand Down Expand Up @@ -460,12 +461,22 @@ describe("copyFileSync", () => {

describe("mkdirSync", () => {
it("should create a directory", () => {
const tempdir = `${tmpdir()}/fs.test.js/${Date.now()}.mkdirSync/1234/hi`;
const now = Date.now().toString();
const base = join(now, ".mkdirSync", "1234", "hi");
const tempdir = `${tmpdir()}/${base}`;
expect(existsSync(tempdir)).toBe(false);
expect(tempdir.includes(mkdirSync(tempdir, { recursive: true })!)).toBe(true);

const res = mkdirSync(tempdir, { recursive: true });
expect(res).toInclude(now);
expect(res).not.toInclude(".mkdirSync");
expect(existsSync(tempdir)).toBe(true);
});

it("should throw ENOENT for empty string", () => {
expect(() => mkdirSync("", { recursive: true })).toThrow("No such file or directory");
expect(() => mkdirSync("")).toThrow("No such file or directory");
});

it("throws for invalid options", () => {
const path = `${tmpdir()}/${Date.now()}.rm.dir2/foo/bar`;

Expand Down Expand Up @@ -1091,10 +1102,10 @@ describe("readSync", () => {
closeSync(fd);
});

it("works with invalid fd but zero length",()=>{
it("works with invalid fd but zero length", () => {
expect(readSync(2147483640, Buffer.alloc(0))).toBe(0);
expect(readSync(2147483640, Buffer.alloc(10), 0, 0, 0)).toBe(0);
})
});
});

it("writevSync", () => {
Expand Down Expand Up @@ -2074,7 +2085,7 @@ describe("fs.ReadStream", () => {

describe("createWriteStream", () => {
it("simple write stream finishes", async () => {
const path = `${tmpdir()}/fs.test.js/${Date.now()}.createWriteStream.txt`;
const path = `${tmpdir()}/fs.test.ts/${Date.now()}.createWriteStream.txt`;
const stream = createWriteStream(path);
stream.write("Test file written successfully");
stream.end();
Expand All @@ -2092,7 +2103,7 @@ describe("createWriteStream", () => {
});

it("writing null throws ERR_STREAM_NULL_VALUES", async () => {
const path = `${tmpdir()}/fs.test.js/${Date.now()}.createWriteStreamNulls.txt`;
const path = `${tmpdir()}/fs.test.ts/${Date.now()}.createWriteStreamNulls.txt`;
const stream = createWriteStream(path);
try {
stream.write(null);
Expand All @@ -2103,7 +2114,7 @@ describe("createWriteStream", () => {
});

it("writing null throws ERR_STREAM_NULL_VALUES (objectMode: true)", async () => {
const path = `${tmpdir()}/fs.test.js/${Date.now()}.createWriteStreamNulls.txt`;
const path = `${tmpdir()}/fs.test.ts/${Date.now()}.createWriteStreamNulls.txt`;
const stream = createWriteStream(path, {
// @ts-ignore-next-line
objectMode: true,
Expand All @@ -2117,7 +2128,7 @@ describe("createWriteStream", () => {
});

it("writing false throws ERR_INVALID_ARG_TYPE", async () => {
const path = `${tmpdir()}/fs.test.js/${Date.now()}.createWriteStreamFalse.txt`;
const path = `${tmpdir()}/fs.test.ts/${Date.now()}.createWriteStreamFalse.txt`;
const stream = createWriteStream(path);
try {
stream.write(false);
Expand All @@ -2128,7 +2139,7 @@ describe("createWriteStream", () => {
});

it("writing false throws ERR_INVALID_ARG_TYPE (objectMode: true)", async () => {
const path = `${tmpdir()}/fs.test.js/${Date.now()}.createWriteStreamFalse.txt`;
const path = `${tmpdir()}/fs.test.ts/${Date.now()}.createWriteStreamFalse.txt`;
const stream = createWriteStream(path, {
// @ts-ignore-next-line
objectMode: true,
Expand All @@ -2142,7 +2153,7 @@ describe("createWriteStream", () => {
});

it("writing in append mode should not truncate the file", async () => {
const path = `${tmpdir()}/fs.test.js/${Date.now()}.createWriteStreamAppend.txt`;
const path = `${tmpdir()}/fs.test.ts/${Date.now()}.createWriteStreamAppend.txt`;
const stream = createWriteStream(path, {
// @ts-ignore-next-line
flags: "a",
Expand Down Expand Up @@ -2233,7 +2244,7 @@ describe("fs/promises", () => {
});

it("writeFile", async () => {
const path = `${tmpdir()}/fs.test.js/${Date.now()}.writeFile.txt`;
const path = `${tmpdir()}/fs.test.ts/${Date.now()}.writeFile.txt`;
await writeFile(path, "File written successfully");
expect(readFileSync(path, "utf8")).toBe("File written successfully");
});
Expand Down Expand Up @@ -2595,7 +2606,7 @@ it("fstat on a large file", () => {
var dest: string = "",
fd;
try {
dest = `${tmpdir()}/fs.test.js/${Math.trunc(Math.random() * 10000000000).toString(32)}.stat.txt`;
dest = `${tmpdir()}/fs.test.ts/${Math.trunc(Math.random() * 10000000000).toString(32)}.stat.txt`;
mkdirSync(dirname(dest), { recursive: true });
const bigBuffer = new Uint8Array(1024 * 1024 * 1024);
fd = openSync(dest, "w");
Expand Down