From d4d3a3c54f5e26dec0cc79e273dc488f8a47f2b3 Mon Sep 17 00:00:00 2001 From: Nathan Whitaker <17734409+nathanwhit@users.noreply.github.com> Date: Fri, 5 Jul 2024 11:32:51 -0700 Subject: [PATCH] fix(node): Implement `fs.lchown` (and `process.getegid`) (#24418) Closes https://github.com/denoland/deno/issues/21260. Part of https://github.com/denoland/deno/issues/18218. Implements `node:fs.lchown`, and enables the node_compat test for it. The test uses `process.getegid`, which we didn't have implemented, so I went ahead and implemented that as well to get the test working. --- cli/standalone/file_system.rs | 20 ++++++ ext/fs/in_memory_fs.rs | 18 +++++ ext/fs/interface.rs | 13 ++++ ext/fs/std_fs.rs | 43 +++++++++++ ext/node/lib.rs | 4 ++ ext/node/ops/fs.rs | 41 +++++++++++ ext/node/ops/os/mod.rs | 19 +++++ ext/node/polyfills/_fs/_fs_lchown.ts | 61 ++++++++++++++++ ext/node/polyfills/fs.ts | 9 ++- ext/node/polyfills/process.ts | 10 ++- runtime/permissions/lib.rs | 4 +- tests/node_compat/config.jsonc | 1 + tests/node_compat/runner/TODO.md | 1 - .../test/parallel/test-fs-lchown.js | 71 +++++++++++++++++++ 14 files changed, 310 insertions(+), 5 deletions(-) create mode 100644 ext/node/polyfills/_fs/_fs_lchown.ts create mode 100644 tests/node_compat/test/parallel/test-fs-lchown.js diff --git a/cli/standalone/file_system.rs b/cli/standalone/file_system.rs index 13da0a729d..536b17f277 100644 --- a/cli/standalone/file_system.rs +++ b/cli/standalone/file_system.rs @@ -145,6 +145,26 @@ impl FileSystem for DenoCompileFileSystem { RealFs.chown_async(path, uid, gid).await } + fn lchown_sync( + &self, + path: &Path, + uid: Option, + gid: Option, + ) -> FsResult<()> { + self.error_if_in_vfs(path)?; + RealFs.lchown_sync(path, uid, gid) + } + + async fn lchown_async( + &self, + path: PathBuf, + uid: Option, + gid: Option, + ) -> FsResult<()> { + self.error_if_in_vfs(&path)?; + RealFs.lchown_async(path, uid, gid).await + } + fn remove_sync(&self, path: &Path, recursive: bool) -> FsResult<()> { self.error_if_in_vfs(path)?; RealFs.remove_sync(path, recursive) diff --git a/ext/fs/in_memory_fs.rs b/ext/fs/in_memory_fs.rs index e2babf40aa..027539e849 100644 --- a/ext/fs/in_memory_fs.rs +++ b/ext/fs/in_memory_fs.rs @@ -178,6 +178,24 @@ impl FileSystem for InMemoryFs { self.chown_sync(&path, uid, gid) } + fn lchown_sync( + &self, + _path: &Path, + _uid: Option, + _gid: Option, + ) -> FsResult<()> { + Err(FsError::NotSupported) + } + + async fn lchown_async( + &self, + path: PathBuf, + uid: Option, + gid: Option, + ) -> FsResult<()> { + self.lchown_sync(&path, uid, gid) + } + fn remove_sync(&self, _path: &Path, _recursive: bool) -> FsResult<()> { Err(FsError::NotSupported) } diff --git a/ext/fs/interface.rs b/ext/fs/interface.rs index cb6fc4f639..f639a700bf 100644 --- a/ext/fs/interface.rs +++ b/ext/fs/interface.rs @@ -146,6 +146,19 @@ pub trait FileSystem: std::fmt::Debug + MaybeSend + MaybeSync { gid: Option, ) -> FsResult<()>; + fn lchown_sync( + &self, + path: &Path, + uid: Option, + gid: Option, + ) -> FsResult<()>; + async fn lchown_async( + &self, + path: PathBuf, + uid: Option, + gid: Option, + ) -> FsResult<()>; + fn remove_sync(&self, path: &Path, recursive: bool) -> FsResult<()>; async fn remove_async(&self, path: PathBuf, recursive: bool) -> FsResult<()>; diff --git a/ext/fs/std_fs.rs b/ext/fs/std_fs.rs index c501b8928e..7903700c3b 100644 --- a/ext/fs/std_fs.rs +++ b/ext/fs/std_fs.rs @@ -303,6 +303,24 @@ impl FileSystem for RealFs { .await? } + fn lchown_sync( + &self, + path: &Path, + uid: Option, + gid: Option, + ) -> FsResult<()> { + lchown(path, uid, gid) + } + + async fn lchown_async( + &self, + path: PathBuf, + uid: Option, + gid: Option, + ) -> FsResult<()> { + spawn_blocking(move || lchown(&path, uid, gid)).await? + } + fn write_file_sync( &self, path: &Path, @@ -431,6 +449,31 @@ fn chown(_path: &Path, _uid: Option, _gid: Option) -> FsResult<()> { Err(FsError::NotSupported) } +#[cfg(unix)] +fn lchown(path: &Path, uid: Option, gid: Option) -> FsResult<()> { + use std::os::unix::ffi::OsStrExt; + let c_path = std::ffi::CString::new(path.as_os_str().as_bytes()).unwrap(); + // -1 = leave unchanged + let uid = uid + .map(|uid| uid as libc::uid_t) + .unwrap_or(-1i32 as libc::uid_t); + let gid = gid + .map(|gid| gid as libc::gid_t) + .unwrap_or(-1i32 as libc::gid_t); + // SAFETY: `c_path` is a valid C string and lives throughout this function call. + let result = unsafe { libc::lchown(c_path.as_ptr(), uid, gid) }; + if result != 0 { + return Err(io::Error::last_os_error().into()); + } + Ok(()) +} + +// TODO: implement lchown for Windows +#[cfg(not(unix))] +fn lchown(_path: &Path, _uid: Option, _gid: Option) -> FsResult<()> { + Err(FsError::NotSupported) +} + fn remove(path: &Path, recursive: bool) -> FsResult<()> { // TODO: this is racy. This should open fds, and then `unlink` those. let metadata = fs::symlink_metadata(path)?; diff --git a/ext/node/lib.rs b/ext/node/lib.rs index 01c464df13..e864133464 100644 --- a/ext/node/lib.rs +++ b/ext/node/lib.rs @@ -315,6 +315,8 @@ deno_core::extension!(deno_node, ops::fs::op_node_fs_exists_sync

, ops::fs::op_node_cp_sync

, ops::fs::op_node_cp

, + ops::fs::op_node_lchown_sync

, + ops::fs::op_node_lchown

, ops::fs::op_node_lutimes_sync

, ops::fs::op_node_lutimes

, ops::fs::op_node_statfs

, @@ -365,6 +367,7 @@ deno_core::extension!(deno_node, ops::os::op_node_os_set_priority

, ops::os::op_node_os_username

, ops::os::op_geteuid

, + ops::os::op_getegid

, ops::os::op_cpus

, ops::os::op_homedir

, op_node_build_os, @@ -426,6 +429,7 @@ deno_core::extension!(deno_node, "_fs/_fs_fsync.ts", "_fs/_fs_ftruncate.ts", "_fs/_fs_futimes.ts", + "_fs/_fs_lchown.ts", "_fs/_fs_link.ts", "_fs/_fs_lstat.ts", "_fs/_fs_lutimes.ts", diff --git a/ext/node/ops/fs.rs b/ext/node/ops/fs.rs index 304a6c2539..47b66ee1d5 100644 --- a/ext/node/ops/fs.rs +++ b/ext/node/ops/fs.rs @@ -273,3 +273,44 @@ where Ok(()) } + +#[op2] +pub fn op_node_lchown_sync

( + state: &mut OpState, + #[string] path: String, + uid: Option, + gid: Option, +) -> Result<(), AnyError> +where + P: NodePermissions + 'static, +{ + let path = PathBuf::from(path); + state + .borrow_mut::

() + .check_write_with_api_name(&path, Some("node:fs.lchownSync"))?; + let fs = state.borrow::(); + fs.lchown_sync(&path, uid, gid)?; + Ok(()) +} + +#[op2(async)] +pub async fn op_node_lchown

( + state: Rc>, + #[string] path: String, + uid: Option, + gid: Option, +) -> Result<(), AnyError> +where + P: NodePermissions + 'static, +{ + let path = PathBuf::from(path); + let fs = { + let mut state = state.borrow_mut(); + state + .borrow_mut::

() + .check_write_with_api_name(&path, Some("node:fs.lchown"))?; + state.borrow::().clone() + }; + fs.lchown_async(path, uid, gid).await?; + Ok(()) +} diff --git a/ext/node/ops/os/mod.rs b/ext/node/ops/os/mod.rs index 5b32113e5b..b7374dc322 100644 --- a/ext/node/ops/os/mod.rs +++ b/ext/node/ops/os/mod.rs @@ -75,6 +75,25 @@ where Ok(euid) } +#[op2(fast)] +pub fn op_getegid

(state: &mut OpState) -> Result +where + P: NodePermissions + 'static, +{ + { + let permissions = state.borrow_mut::

(); + permissions.check_sys("getegid", "node:os.getegid()")?; + } + + #[cfg(windows)] + let egid = 0; + #[cfg(unix)] + // SAFETY: Call to libc getegid. + let egid = unsafe { libc::getegid() }; + + Ok(egid) +} + #[op2] #[serde] pub fn op_cpus

(state: &mut OpState) -> Result, AnyError> diff --git a/ext/node/polyfills/_fs/_fs_lchown.ts b/ext/node/polyfills/_fs/_fs_lchown.ts new file mode 100644 index 0000000000..8611c8021d --- /dev/null +++ b/ext/node/polyfills/_fs/_fs_lchown.ts @@ -0,0 +1,61 @@ +// Copyright 2018-2024 the Deno authors. All rights reserved. MIT license. + +// TODO(petamoriken): enable prefer-primordials for node polyfills +// deno-lint-ignore-file prefer-primordials + +import { + type CallbackWithError, + makeCallback, +} from "ext:deno_node/_fs/_fs_common.ts"; +import { + getValidatedPath, + kMaxUserId, +} from "ext:deno_node/internal/fs/utils.mjs"; +import * as pathModule from "node:path"; +import { validateInteger } from "ext:deno_node/internal/validators.mjs"; +import type { Buffer } from "node:buffer"; +import { promisify } from "ext:deno_node/internal/util.mjs"; +import { op_node_lchown, op_node_lchown_sync } from "ext:core/ops"; + +/** + * Asynchronously changes the owner and group + * of a file, without following symlinks. + */ +export function lchown( + path: string | Buffer | URL, + uid: number, + gid: number, + callback: CallbackWithError, +) { + callback = makeCallback(callback); + path = getValidatedPath(path).toString(); + validateInteger(uid, "uid", -1, kMaxUserId); + validateInteger(gid, "gid", -1, kMaxUserId); + + op_node_lchown(pathModule.toNamespacedPath(path), uid, gid).then( + () => callback(null), + callback, + ); +} + +export const lchownPromise = promisify(lchown) as ( + path: string | Buffer | URL, + uid: number, + gid: number, +) => Promise; + +/** + * Synchronously changes the owner and group + * of a file, without following symlinks. + */ +export function lchownSync( + path: string | Buffer | URL, + uid: number, + gid: number, +) { + path = getValidatedPath(path).toString(); + validateInteger(uid, "uid", -1, kMaxUserId); + validateInteger(gid, "gid", -1, kMaxUserId); + + op_node_lchown_sync(pathModule.toNamespacedPath(path), uid, gid); +} diff --git a/ext/node/polyfills/fs.ts b/ext/node/polyfills/fs.ts index 6f0c53e4d2..7a3cf4e67f 100644 --- a/ext/node/polyfills/fs.ts +++ b/ext/node/polyfills/fs.ts @@ -27,6 +27,11 @@ import { fstat, fstatSync } from "ext:deno_node/_fs/_fs_fstat.ts"; import { fsync, fsyncSync } from "ext:deno_node/_fs/_fs_fsync.ts"; import { ftruncate, ftruncateSync } from "ext:deno_node/_fs/_fs_ftruncate.ts"; import { futimes, futimesSync } from "ext:deno_node/_fs/_fs_futimes.ts"; +import { + lchown, + lchownPromise, + lchownSync, +} from "ext:deno_node/_fs/_fs_lchown.ts"; import { link, linkPromise, linkSync } from "ext:deno_node/_fs/_fs_link.ts"; import { lstat, lstatPromise, lstatSync } from "ext:deno_node/_fs/_fs_lstat.ts"; import { @@ -173,7 +178,7 @@ const promises = { unlink: unlinkPromise, chmod: chmodPromise, // lchmod: promisify(lchmod), - // lchown: promisify(lchown), + lchown: lchownPromise, chown: chownPromise, utimes: utimesPromise, lutimes: lutimesPromise, @@ -218,6 +223,8 @@ export default { ftruncateSync, futimes, futimesSync, + lchown, + lchownSync, link, linkSync, lstat, diff --git a/ext/node/polyfills/process.ts b/ext/node/polyfills/process.ts index 02837f8270..de48fea3e8 100644 --- a/ext/node/polyfills/process.ts +++ b/ext/node/polyfills/process.ts @@ -7,6 +7,7 @@ import { core, internals } from "ext:core/mod.js"; import { initializeDebugEnv } from "ext:deno_node/internal/util/debuglog.ts"; import { + op_getegid, op_geteuid, op_node_process_kill, op_process_abort, @@ -309,15 +310,16 @@ export function kill(pid: number, sig: string | number = "SIGTERM") { return true; } -let getgid, getuid, geteuid; +let getgid, getuid, getegid, geteuid; if (!isWindows) { getgid = () => Deno.gid(); getuid = () => Deno.uid(); + getegid = () => op_getegid(); geteuid = () => op_geteuid(); } -export { geteuid, getgid, getuid }; +export { getegid, geteuid, getgid, getuid }; const ALLOWED_FLAGS = buildAllowedFlags(); @@ -685,6 +687,9 @@ Process.prototype.getgid = getgid; /** This method is removed on Windows */ Process.prototype.getuid = getuid; +/** This method is removed on Windows */ +Process.prototype.getegid = getegid; + /** This method is removed on Windows */ Process.prototype.geteuid = geteuid; @@ -726,6 +731,7 @@ Process.prototype.noDeprecation = false; if (isWindows) { delete Process.prototype.getgid; delete Process.prototype.getuid; + delete Process.prototype.getegid; delete Process.prototype.geteuid; } diff --git a/runtime/permissions/lib.rs b/runtime/permissions/lib.rs index 4579eba1a3..3b3f68a530 100644 --- a/runtime/permissions/lib.rs +++ b/runtime/permissions/lib.rs @@ -915,7 +915,9 @@ impl Descriptor for SysDescriptor { pub fn parse_sys_kind(kind: &str) -> Result<&str, AnyError> { match kind { "hostname" | "osRelease" | "osUptime" | "loadavg" | "networkInterfaces" - | "systemMemoryInfo" | "uid" | "gid" | "cpus" | "homedir" => Ok(kind), + | "systemMemoryInfo" | "uid" | "gid" | "cpus" | "homedir" | "getegid" => { + Ok(kind) + } _ => Err(type_error(format!("unknown system info kind \"{kind}\""))), } } diff --git a/tests/node_compat/config.jsonc b/tests/node_compat/config.jsonc index f0b667df44..0c8d380774 100644 --- a/tests/node_compat/config.jsonc +++ b/tests/node_compat/config.jsonc @@ -327,6 +327,7 @@ "test-fs-chown-type-check.js", "test-fs-copyfile.js", "test-fs-empty-readStream.js", + "test-fs-lchown.js", "test-fs-mkdir.js", "test-fs-open-flags.js", "test-fs-open-mode-mask.js", diff --git a/tests/node_compat/runner/TODO.md b/tests/node_compat/runner/TODO.md index ec2a2337e3..cab34f8645 100644 --- a/tests/node_compat/runner/TODO.md +++ b/tests/node_compat/runner/TODO.md @@ -794,7 +794,6 @@ NOTE: This file should not be manually edited. Please edit `tests/node_compat/co - [parallel/test-fs-fmap.js](https://github.com/nodejs/node/tree/v18.12.1/test/parallel/test-fs-fmap.js) - [parallel/test-fs-fsync.js](https://github.com/nodejs/node/tree/v18.12.1/test/parallel/test-fs-fsync.js) - [parallel/test-fs-lchmod.js](https://github.com/nodejs/node/tree/v18.12.1/test/parallel/test-fs-lchmod.js) -- [parallel/test-fs-lchown.js](https://github.com/nodejs/node/tree/v18.12.1/test/parallel/test-fs-lchown.js) - [parallel/test-fs-link.js](https://github.com/nodejs/node/tree/v18.12.1/test/parallel/test-fs-link.js) - [parallel/test-fs-long-path.js](https://github.com/nodejs/node/tree/v18.12.1/test/parallel/test-fs-long-path.js) - [parallel/test-fs-make-callback.js](https://github.com/nodejs/node/tree/v18.12.1/test/parallel/test-fs-make-callback.js) diff --git a/tests/node_compat/test/parallel/test-fs-lchown.js b/tests/node_compat/test/parallel/test-fs-lchown.js new file mode 100644 index 0000000000..ce3333745b --- /dev/null +++ b/tests/node_compat/test/parallel/test-fs-lchown.js @@ -0,0 +1,71 @@ +// deno-fmt-ignore-file +// deno-lint-ignore-file + +// Copyright Joyent and Node contributors. All rights reserved. MIT license. +// Taken from Node 18.12.1 +// This file is automatically generated by `tests/node_compat/runner/setup.ts`. Do not modify this file manually. + +'use strict'; + +const common = require('../common'); +const tmpdir = require('../common/tmpdir'); +const assert = require('assert'); +const fs = require('fs'); +const path = require('path'); +const { promises } = fs; + +// Validate the path argument. +[false, 1, {}, [], null, undefined].forEach((i) => { + const err = { name: 'TypeError', code: 'ERR_INVALID_ARG_TYPE' }; + + assert.throws(() => fs.lchown(i, 1, 1, common.mustNotCall()), err); + assert.throws(() => fs.lchownSync(i, 1, 1), err); + promises.lchown(false, 1, 1) + .then(common.mustNotCall()) + .catch(common.expectsError(err)); +}); + +// Validate the uid and gid arguments. +[false, 'test', {}, [], null, undefined].forEach((i) => { + const err = { name: 'TypeError', code: 'ERR_INVALID_ARG_TYPE' }; + + assert.throws( + () => fs.lchown('not_a_file_that_exists', i, 1, common.mustNotCall()), + err + ); + assert.throws( + () => fs.lchown('not_a_file_that_exists', 1, i, common.mustNotCall()), + err + ); + assert.throws(() => fs.lchownSync('not_a_file_that_exists', i, 1), err); + assert.throws(() => fs.lchownSync('not_a_file_that_exists', 1, i), err); + + promises.lchown('not_a_file_that_exists', i, 1) + .then(common.mustNotCall()) + .catch(common.expectsError(err)); + + promises.lchown('not_a_file_that_exists', 1, i) + .then(common.mustNotCall()) + .catch(common.expectsError(err)); +}); + +// Validate the callback argument. +[false, 1, 'test', {}, [], null, undefined].forEach((i) => { + assert.throws(() => fs.lchown('not_a_file_that_exists', 1, 1, i), { + name: 'TypeError', + code: 'ERR_INVALID_ARG_TYPE' + }); +}); + +if (!common.isWindows) { + const testFile = tmpdir.resolve(path.basename(__filename)); + const uid = process.geteuid(); + const gid = process.getegid(); + + tmpdir.refresh(); + fs.copyFileSync(__filename, testFile); + fs.lchownSync(testFile, uid, gid); + fs.lchown(testFile, uid, gid, common.mustSucceed(async (err) => { + await promises.lchown(testFile, uid, gid); + })); +}