Skip to content

fix(ext/node): parse fs open options correctly #30300

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Aug 5, 2025
Merged

fix(ext/node): parse fs open options correctly #30300

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1278252
chore: omit todo
Tango992 Aug 1, 2025
5eb79d2
fix: set the open options correctly based on the flags
Tango992 Aug 3, 2025
34713f2
test: assert changes node fs open ops
Tango992 Aug 3, 2025
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
59 changes: 56 additions & 3 deletions ext/node/ops/fs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,55 @@ where
Ok(())
}

fn get_open_options(mut flags: i32, mode: u32) -> OpenOptions {
let mut options = OpenOptions {
mode: Some(mode),
..Default::default()
};

if (flags & libc::O_APPEND) == libc::O_APPEND {
options.append = true;
flags &= !libc::O_APPEND;
}
if (flags & libc::O_CREAT) == libc::O_CREAT {
options.create = true;
flags &= !libc::O_CREAT;
}
if (flags & libc::O_EXCL) == libc::O_EXCL {
options.create_new = true;
options.write = true;
flags &= !libc::O_EXCL;
}
Comment on lines +155 to +159
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's the current logic from Deno 2.4.3:

deno/ext/node/polyfills/_fs/_fs_common.ts

Lines 222 to 226 in 5612d2e

if ((flag & O_EXCL) === O_EXCL) {
openOptions.createNew = true;
openOptions.read = true;
openOptions.write = true;
}

I intentionally omitted the read permission as the Rust stdlib only requires write permission when chained with create_new:
https://doc.rust-lang.org/src/std/sys/fs/unix.rs.html#1118-1152

if (flags & libc::O_RDWR) == libc::O_RDWR {
options.read = true;
options.write = true;
flags &= !libc::O_RDWR;
}
if (flags & libc::O_TRUNC) == libc::O_TRUNC {
options.truncate = true;
flags &= !libc::O_TRUNC;
}
if (flags & libc::O_WRONLY) == libc::O_WRONLY {
options.write = true;
flags &= !libc::O_WRONLY;
}

if flags != 0 {
options.custom_flags = Some(flags);
}

if !options.append
&& !options.create
&& !options.create_new
&& !options.read
&& !options.truncate
&& !options.write
{
options.read = true;
}
options
}

fn open_options_to_access_kind(open_options: &OpenOptions) -> OpenAccessKind {
let read = open_options.read;
let write = open_options.write || open_options.append;
Expand All @@ -148,17 +197,19 @@ fn open_options_to_access_kind(open_options: &OpenOptions) -> OpenAccessKind {
}
}

#[op2(stack_trace)]
#[op2(fast, stack_trace)]
#[smi]
pub fn op_node_open_sync<P>(
state: &mut OpState,
#[string] path: &str,
#[serde] options: OpenOptions,
#[smi] flags: i32,
#[smi] mode: u32,
) -> Result<ResourceId, FsError>
where
P: NodePermissions + 'static,
{
let path = Path::new(path);
let options = get_open_options(flags, mode);

let fs = state.borrow::<FileSystemRc>().clone();
let path = state.borrow_mut::<P>().check_open(
Expand All @@ -178,12 +229,14 @@ where
pub async fn op_node_open<P>(
state: Rc<RefCell<OpState>>,
#[string] path: String,
#[serde] options: OpenOptions,
#[smi] flags: i32,
#[smi] mode: u32,
) -> Result<ResourceId, FsError>
where
P: NodePermissions + 'static,
{
let path = PathBuf::from(path);
let options = get_open_options(flags, mode);

let (fs, path) = {
let mut state = state.borrow_mut();
Expand Down
62 changes: 0 additions & 62 deletions ext/node/polyfills/_fs/_fs_common.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,6 @@ const {
ReflectApply,
Error,
} = primordials;
import {
O_APPEND,
O_CREAT,
O_EXCL,
O_RDONLY,
O_RDWR,
O_TRUNC,
O_WRONLY,
} from "ext:deno_node/_fs/_fs_constants.ts";
import { validateFunction } from "ext:deno_node/internal/validators.mjs";
import type { ErrnoException } from "ext:deno_node/_global.d.ts";
import {
Expand Down Expand Up @@ -53,11 +44,6 @@ export interface WriteFileOptions extends FileOptions {
mode?: number;
}

export type OpOpenOptions = Deno.OpenOptions & {
customFlags?: number;
mode?: number;
};

export function isFileOptions(
fileOptions: string | WriteFileOptions | undefined,
): fileOptions is FileOptions {
Expand Down Expand Up @@ -129,54 +115,6 @@ export function checkEncoding(encoding: Encodings | null): Encodings | null {
throw new Error(`The value "${encoding}" is invalid for option "encoding"`);
}

export function getOpenOptions(
flag: number | undefined,
mode: number | undefined,
): OpOpenOptions {
if (typeof flag !== "number") {
return { create: true, append: true, mode };
}

const openOptions: OpOpenOptions = { mode };
let customFlags = flag;

if ((flag & O_APPEND) === O_APPEND) {
openOptions.append = true;
customFlags &= ~O_APPEND;
}
if ((flag & O_CREAT) === O_CREAT) {
openOptions.create = true;
openOptions.write = true;
customFlags &= ~O_CREAT;
}
if ((flag & O_EXCL) === O_EXCL) {
openOptions.createNew = true;
openOptions.read = true;
openOptions.write = true;
customFlags &= ~O_EXCL;
}
if ((flag & O_TRUNC) === O_TRUNC) {
openOptions.truncate = true;
customFlags &= ~O_TRUNC;
}
if ((flag & O_RDONLY) === O_RDONLY) {
openOptions.read = true;
customFlags &= ~O_RDONLY;
}
if ((flag & O_WRONLY) === O_WRONLY) {
openOptions.write = true;
customFlags &= ~O_WRONLY;
}
if ((flag & O_RDWR) === O_RDWR) {
openOptions.read = true;
openOptions.write = true;
customFlags &= ~O_RDWR;
}
openOptions.customFlags = customFlags;

return openOptions;
}

export { isUint32 as isFd } from "ext:deno_node/internal/validators.mjs";

export function maybeCallback(cb: unknown) {
Expand Down
12 changes: 3 additions & 9 deletions ext/node/polyfills/_fs/_fs_open.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// Copyright 2018-2025 the Deno authors. MIT license.

import { primordials } from "ext:core/mod.js";
import { getOpenOptions, makeCallback } from "ext:deno_node/_fs/_fs_common.ts";
import { makeCallback } from "ext:deno_node/_fs/_fs_common.ts";
import { parseFileMode } from "ext:deno_node/internal/validators.mjs";
import {
getValidatedPathToString,
Expand Down Expand Up @@ -69,10 +69,7 @@ export function open(
callback = makeCallback(callback);

PromisePrototypeThen(
op_node_open(
pathModule.toNamespacedPath(path),
getOpenOptions(flags, mode),
),
op_node_open(pathModule.toNamespacedPath(path), flags, mode),
(rid: number) => callback(null, rid),
(err: Error) =>
callback(denoErrorToNodeError(err, { syscall: "open", path })),
Expand Down Expand Up @@ -113,10 +110,7 @@ export function openSync(
const mode = parseFileMode(maybeMode, "mode", 0o666);

try {
return op_node_open_sync(
pathModule.toNamespacedPath(path),
getOpenOptions(flags, mode),
);
return op_node_open_sync(pathModule.toNamespacedPath(path), flags, mode);
} catch (err) {
throw denoErrorToNodeError(err as Error, { syscall: "open", path });
}
Expand Down
2 changes: 2 additions & 0 deletions tests/node_compat/config.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -417,6 +417,7 @@
"parallel/test-exception-handler2.js" = {}
"parallel/test-fetch.mjs" = {}
"parallel/test-file-read-noexist.js" = {}
"parallel/test-file-validate-mode-flag.js" = {}
"parallel/test-file-write-stream5.js" = {}
"parallel/test-finalization-registry-shutdown.js" = {}
"parallel/test-fs-buffertype-writesync.js" = {}
Expand All @@ -430,6 +431,7 @@
"parallel/test-fs-fsync.js" = {}
"parallel/test-fs-link.js" = { flaky = true }
"parallel/test-fs-long-path.js" = {}
"parallel/test-fs-open-flags.js" = {}
"parallel/test-fs-open-no-close.js" = {}
"parallel/test-fs-open-numeric-flags.js" = {}
"parallel/test-fs-open.js" = {}
Expand Down
113 changes: 95 additions & 18 deletions tests/unit_node/_fs/_fs_open_test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
import {
O_APPEND,
O_CREAT,
O_DIRECTORY as macOsODirectory,
O_DIRECTORY,
O_EXCL,
O_RDONLY,
O_RDWR,
Expand All @@ -12,27 +12,20 @@ import {
} from "node:constants";
import { assertEquals, assertRejects, assertThrows, fail } from "@std/assert";
import { assertCallbackErrorUncaught } from "../_test_utils.ts";
import { open, openSync } from "node:fs";
import {
closeSync,
existsSync,
open,
openSync,
readSync,
writeFileSync,
writeSync,
} from "node:fs";
import { open as openPromise } from "node:fs/promises";
import { join, parse } from "node:path";
import { closeSync, existsSync } from "node:fs";

const tempDir = parse(Deno.makeTempFileSync()).dir;

// TODO(Tango992): Delete this once #30113 has landed
let O_DIRECTORY: number | undefined;
if (Deno.build.os === "linux" || Deno.build.os === "android") {
if (Deno.build.arch === "x86_64") {
// https://docs.rs/libc/latest/libc/constant.O_DIRECTORY.html
O_DIRECTORY = 0x10000;
} else {
// https://docs.rs/libc/latest/aarch64-unknown-linux-gnu/libc/constant.O_DIRECTORY.html
O_DIRECTORY = 0x4000;
}
} else if (Deno.build.os === "darwin") {
O_DIRECTORY = macOsODirectory;
}

Deno.test({
name: "ASYNC: open file",
async fn() {
Expand Down Expand Up @@ -449,7 +442,7 @@ Deno.test("[std/node/fs] open with custom flag", {
let invalidFlag: number | undefined;
// On linux it refers to the `O_TMPFILE` constant in libc.
// It should throw EINVAL when it's not followed by `O_RDWR` or `O_WRONLY`.
// https://docs.rs/libc/latest/libc/constant.O_DIRECTORY.html
// https://docs.rs/libc/latest/libc/constant.O_TMPFILE.html
if (Deno.build.os === "linux" || Deno.build.os === "android") {
if (Deno.build.arch === "x86_64") {
invalidFlag = 4_259_840;
Expand Down Expand Up @@ -489,3 +482,87 @@ Deno.test("[std/node/fs] open throws on invalid flags", {
);
await Deno.remove(file);
});

Deno.test(
"[std/node/fs] openSync: only enable read permission when a custom flag is not followed by file access flags",
{
ignore: Deno.build.os === "windows",
},
async () => {
const path = await Deno.makeTempFile();
writeFileSync(path, "Hello, world!");

const fd = openSync(path, O_SYNC);
readSync(fd, new Uint8Array(1), 0, 1, 0);
assertThrows(
() => {
writeSync(fd, "This should fail");
},
Error,
);

closeSync(fd);
await Deno.remove(path);
},
);

Deno.test(
"[std/node/fs] open: only enable read permission when a custom flag is not followed by file access flags",
{
ignore: Deno.build.os === "windows",
},
async () => {
const path = await Deno.makeTempFile();
writeFileSync(path, "Hello, world!");

const fileHandle = await openPromise(path, O_SYNC);
readSync(fileHandle.fd, new Uint8Array(1), 0, 1, 0);
assertThrows(
() => {
writeSync(fileHandle.fd, "This should fail");
},
Error,
);

await fileHandle.close();
await Deno.remove(path);
},
);

Deno.test(
"[std/node/fs] open: only enable write permission",
async () => {
const path = await Deno.makeTempFile();
const fileHandle = await openPromise(path, O_WRONLY);

writeSync(fileHandle.fd, "Hello, world!");
assertThrows(
() => {
readSync(fileHandle.fd, new Uint8Array(1), 0, 1, 0);
},
Error,
);

await fileHandle.close();
await Deno.remove(path);
},
);

Deno.test(
"[std/node/fs] openSync: only enable write permission",
async () => {
const path = await Deno.makeTempFile();
const fd = openSync(path, O_WRONLY);

writeSync(fd, "Hello, world!");
assertThrows(
() => {
readSync(fd, new Uint8Array(1), 0, 1, 0);
},
Error,
);

closeSync(fd);
await Deno.remove(path);
},
);
Loading