Repository: denoland/deno
Pull request: fix(ext/node): parse fs open options correctly

Fixes #30299 

I decided to build the `OpenOptions` on the Rust side, because it's cheaper to pass integers to the op function and we can enable the fast op call. Also the tests that I added to the `config.toml` were already passing before this PR.

<!--
Before submitting a PR, please read https://docs.deno.com/runtime/manual/references/contributing

1. Give the PR a descriptive title.

  Examples of good title:
    - fix(std/http): Fix race condition in server
    - docs(console): Update docstrings
    - feat(doc): Handle nested reexports

  Examples of bad title:
    - fix #7123
    - update docs
    - fix bugs

2. Ensure there is a related issue and it is referenced in the PR text.
3. Ensure there are tests that cover the changes.
4. Ensure `cargo test` passes.
5. Ensure `./tools/format.js` passes without changing files.
6. Ensure `./tools/lint.js` passes.
7. Open as a draft PR if your work is still in progress. The CI won't run
   all steps, but you can add '[ci]' to a commit message to force it to.
8. If you would like to run the benchmarks on the CI, add the 'ci-bench' label.
-->


Changed files:
filename: ext/node/ops/fs.rs
patch: @@ -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;
+  }
+  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;
@@ -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(
@@ -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();

filename: ext/node/polyfills/_fs/_fs_common.ts
patch: @@ -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 {
@@ -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 {
@@ -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) {

filename: ext/node/polyfills/_fs/_fs_open.ts
patch: @@ -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,
@@ -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 })),
@@ -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 });
   }

filename: tests/node_compat/config.toml
patch: @@ -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" = {}
@@ -431,6 +432,7 @@
 "parallel/test-fs-internal-assertencoding.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" = {}

filename: tests/unit_node/_fs/_fs_open_test.ts
patch: @@ -2,7 +2,7 @@
 import {
   O_APPEND,
   O_CREAT,
-  O_DIRECTORY as macOsODirectory,
+  O_DIRECTORY,
   O_EXCL,
   O_RDONLY,
   O_RDWR,
@@ -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() {
@@ -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;
@@ -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);
+  },
+);