fs: undeprecate lchown()

uv_fs_lchown() exists, as of libuv 1.21.0. fs.lchown() can now
be undeprecated. This commit also adds tests, as there were
none.

PR-URL: https://github.com/nodejs/node/pull/21498
Fixes: https://github.com/nodejs/node/issues/19868
Reviewed-By: Wyatt Preul <wpreul@gmail.com>

doc/api/deprecations.md

@@ -351,20 +351,6 @@ Type: Documentation-only
 
 The [`fs.lchmodSync(path, mode)`][] API is deprecated.
 
-<a id="DEP0037"></a>
-### DEP0037: fs.lchown(path, uid, gid, callback)
-
-Type: Documentation-only
-
-The [`fs.lchown(path, uid, gid, callback)`][] API is deprecated.
-
-<a id="DEP0038"></a>
-### DEP0038: fs.lchownSync(path, uid, gid)
-
-Type: Documentation-only
-
-The [`fs.lchownSync(path, uid, gid)`][] API is deprecated.
-
 <a id="DEP0039"></a>
 ### DEP0039: require.extensions
 
@@ -1049,8 +1035,6 @@ The option `produceCachedData` has been deprecated. Use
 [`fs.exists(path, callback)`]: fs.html#fs_fs_exists_path_callback
 [`fs.lchmod(path, mode, callback)`]: fs.html#fs_fs_lchmod_path_mode_callback
 [`fs.lchmodSync(path, mode)`]: fs.html#fs_fs_lchmodsync_path_mode
-[`fs.lchown(path, uid, gid, callback)`]: fs.html#fs_fs_lchown_path_uid_gid_callback
-[`fs.lchownSync(path, uid, gid)`]: fs.html#fs_fs_lchownsync_path_uid_gid
 [`fs.read()`]: fs.html#fs_fs_read_fd_buffer_offset_length_position_callback
 [`fs.readSync()`]: fs.html#fs_fs_readsync_fd_buffer_offset_length_position
 [`fs.stat()`]: fs.html#fs_fs_stat_path_callback

doc/api/documentation.md

@@ -83,10 +83,6 @@ which simply wrap a syscall,
 like [`fs.open()`][], will document that. The docs link to the corresponding man
 pages (short for manual pages) which describe how the syscalls work.
 
-Some syscalls, like lchown(2), are BSD-specific. That means, for
-example, that [`fs.lchown()`][] only works on macOS and other BSD-derived
-systems, and is not available on Linux.
-
 Most Unix syscalls have Windows equivalents, but behavior may differ on Windows
 relative to Linux and macOS. For an example of the subtle ways in which it's
 sometimes impossible to replace Unix syscall semantics on Windows, see [Node
@@ -95,6 +91,5 @@ issue 4760](https://github.com/nodejs/node/issues/4760).
 [`'warning'`]: process.html#process_event_warning
 [`stderr`]: process.html#process_process_stderr
 [`fs.open()`]: fs.html#fs_fs_open_path_flags_mode_callback
-[`fs.lchown()`]: fs.html#fs_fs_lchown_path_uid_gid_callback
 [submit an issue]: https://github.com/nodejs/node/issues/new
 [the contributing guide]: https://github.com/nodejs/node/blob/master/CONTRIBUTING.md

doc/api/fs.md

@@ -1908,8 +1908,10 @@ Synchronous lchmod(2). Returns `undefined`.
 
 ## fs.lchown(path, uid, gid, callback)
 <!-- YAML
-deprecated: v0.4.7
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/21498
+    description: This API is no longer deprecated.
   - version: v10.0.0
     pr-url: https://github.com/nodejs/node/pull/12562
     description: The `callback` parameter is no longer optional. Not passing
@@ -1931,7 +1933,10 @@ to the completion callback.
 
 ## fs.lchownSync(path, uid, gid)
 <!-- YAML
-deprecated: v0.4.7
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/21498
+    description: This API is no longer deprecated.
 -->
 
 * `path` {string|Buffer|URL}
@@ -3904,7 +3909,11 @@ no arguments upon success. This method is only implemented on macOS.
 
 ### fsPromises.lchown(path, uid, gid)
 <!-- YAML
-deprecated: v10.0.0
+added: v10.0.0
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/21498
+    description: This API is no longer deprecated.
 -->
 
 * `path` {string|Buffer|URL}
@@ -3913,7 +3922,7 @@ deprecated: v10.0.0
 * Returns: {Promise}
 
 Changes the ownership on a symbolic link then resolves the `Promise` with
-no arguments upon success. This method is only implemented on macOS.
+no arguments upon success.
 
 ### fsPromises.link(existingPath, newPath)
 <!-- YAML

lib/fs.js

@@ -997,31 +997,24 @@ function chmodSync(path, mode) {
 }
 
 function lchown(path, uid, gid, callback) {
-  callback = maybeCallback(callback);
+  callback = makeCallback(callback);
-  fs.open(path, O_WRONLY | O_SYMLINK, function(err, fd) {
+  path = getPathFromURL(path);
-    if (err) {
+  validatePath(path);
-      callback(err);
+  validateUint32(uid, 'uid');
-      return;
+  validateUint32(gid, 'gid');
-    }
+  const req = new FSReqWrap();
-    // Prefer to return the chown error, if one occurs,
+  req.oncomplete = callback;
-    // but still try to close, and report closing errors if they occur.
+  binding.lchown(pathModule.toNamespacedPath(path), uid, gid, req);
-    fs.fchown(fd, uid, gid, function(err) {
-      fs.close(fd, function(err2) {
-        callback(err || err2);
-      });
-    });
-  });
 }
 
 function lchownSync(path, uid, gid) {
-  const fd = fs.openSync(path, O_WRONLY | O_SYMLINK);
+  path = getPathFromURL(path);
-  let ret;
+  validatePath(path);
-  try {
+  validateUint32(uid, 'uid');
-    ret = fs.fchownSync(fd, uid, gid);
+  validateUint32(gid, 'gid');
-  } finally {
+  const ctx = { path };
-    fs.closeSync(fd);
+  binding.lchown(pathModule.toNamespacedPath(path), uid, gid, undefined, ctx);
-  }
+  handleErrorFromBinding(ctx);
-  return ret;
 }
 
 function fchown(fd, uid, gid, callback) {
@@ -1753,8 +1746,8 @@ module.exports = fs = {
   ftruncateSync,
   futimes,
   futimesSync,
-  lchown: constants.O_SYMLINK !== undefined ? lchown : undefined,
+  lchown,
-  lchownSync: constants.O_SYMLINK !== undefined ? lchownSync : undefined,
+  lchownSync,
   lchmod: constants.O_SYMLINK !== undefined ? lchmod : undefined,
   lchmodSync: constants.O_SYMLINK !== undefined ? lchmodSync : undefined,
   link,

lib/internal/fs/promises.js

@@ -377,11 +377,12 @@ async function lchmod(path, mode) {
 }
 
 async function lchown(path, uid, gid) {
-  if (O_SYMLINK === undefined)
+  path = getPathFromURL(path);
-    throw new ERR_METHOD_NOT_IMPLEMENTED();
+  validatePath(path);
-
+  validateUint32(uid, 'uid');
-  const fd = await open(path, O_WRONLY | O_SYMLINK);
+  validateUint32(gid, 'gid');
-  return fchown(fd, uid, gid).finally(fd.close.bind(fd));
+  return binding.lchown(pathModule.toNamespacedPath(path),
+                        uid, gid, kUsePromises);
 }
 
 async function fchown(handle, uid, gid) {

src/node_file.cc

@@ -1771,6 +1771,36 @@ static void FChown(const FunctionCallbackInfo<Value>& args) {
 }
 
 
+static void LChown(const FunctionCallbackInfo<Value>& args) {
+  Environment* env = Environment::GetCurrent(args);
+
+  const int argc = args.Length();
+  CHECK_GE(argc, 3);
+
+  BufferValue path(env->isolate(), args[0]);
+  CHECK_NOT_NULL(*path);
+
+  CHECK(args[1]->IsUint32());
+  const uv_uid_t uid = static_cast<uv_uid_t>(args[1].As<Uint32>()->Value());
+
+  CHECK(args[2]->IsUint32());
+  const uv_gid_t gid = static_cast<uv_gid_t>(args[2].As<Uint32>()->Value());
+
+  FSReqBase* req_wrap_async = GetReqWrap(env, args[3]);
+  if (req_wrap_async != nullptr) {  // lchown(path, uid, gid, req)
+    AsyncCall(env, req_wrap_async, args, "lchown", UTF8, AfterNoArgs,
+              uv_fs_lchown, *path, uid, gid);
+  } else {  // lchown(path, uid, gid, undefined, ctx)
+    CHECK_EQ(argc, 5);
+    FSReqWrapSync req_wrap_sync;
+    FS_SYNC_TRACE_BEGIN(lchown);
+    SyncCall(env, args[4], &req_wrap_sync, "lchown",
+             uv_fs_lchown, *path, uid, gid);
+    FS_SYNC_TRACE_END(lchown);
+  }
+}
+
+
 static void UTimes(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
 
@@ -1904,7 +1934,7 @@ void Initialize(Local<Object> target,
 
   env->SetMethod(target, "chown", Chown);
   env->SetMethod(target, "fchown", FChown);
-  // env->SetMethod(target, "lchown", LChown);
+  env->SetMethod(target, "lchown", LChown);
 
   env->SetMethod(target, "utimes", UTimes);
   env->SetMethod(target, "futimes", FUTimes);

test/parallel/test-fs-lchown.js

@@ -0,0 +1,67 @@
+'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;
+
+common.crashOnUnhandledRejection();
+
+// Validate the path argument.
+[false, 1, {}, [], null, undefined].forEach((i) => {
+  const err = { type: TypeError, code: 'ERR_INVALID_ARG_TYPE' };
+
+  common.expectsError(() => fs.lchown(i, 1, 1, common.mustNotCall()), err);
+  common.expectsError(() => 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 = { type: TypeError, code: 'ERR_INVALID_ARG_TYPE' };
+
+  common.expectsError(
+    () => fs.lchown('not_a_file_that_exists', i, 1, common.mustNotCall()),
+    err
+  );
+  common.expectsError(
+    () => fs.lchown('not_a_file_that_exists', 1, i, common.mustNotCall()),
+    err
+  );
+  common.expectsError(() => fs.lchownSync('not_a_file_that_exists', i, 1), err);
+  common.expectsError(() => 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) => {
+  common.expectsError(() => fs.lchown('not_a_file_that_exists', 1, 1, i), {
+    type: TypeError,
+    code: 'ERR_INVALID_CALLBACK'
+  });
+});
+
+if (!common.isWindows) {
+  const testFile = path.join(tmpdir.path, 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.mustCall(async (err) => {
+    assert.ifError(err);
+    await promises.lchown(testFile, uid, gid);
+  }));
+}

test/parallel/test-fs-null-bytes.js

@@ -59,6 +59,7 @@ check(fs.chmod, fs.chmodSync, 'foo\u0000bar', '0644');
 check(fs.chown, fs.chownSync, 'foo\u0000bar', 12, 34);
 check(fs.copyFile, fs.copyFileSync, 'foo\u0000bar', 'abc');
 check(fs.copyFile, fs.copyFileSync, 'abc', 'foo\u0000bar');
+check(fs.lchown, fs.lchownSync, 'foo\u0000bar', 12, 34);
 check(fs.link, fs.linkSync, 'foo\u0000bar', 'foobar');
 check(fs.link, fs.linkSync, 'foobar', 'foo\u0000bar');
 check(fs.lstat, fs.lstatSync, 'foo\u0000bar');
@@ -92,6 +93,7 @@ check(fs.chmod, fs.chmodSync, fileUrl, '0644');
 check(fs.chown, fs.chownSync, fileUrl, 12, 34);
 check(fs.copyFile, fs.copyFileSync, fileUrl, 'abc');
 check(fs.copyFile, fs.copyFileSync, 'abc', fileUrl);
+check(fs.lchown, fs.lchownSync, fileUrl, 12, 34);
 check(fs.link, fs.linkSync, fileUrl, 'foobar');
 check(fs.link, fs.linkSync, 'foobar', fileUrl);
 check(fs.lstat, fs.lstatSync, fileUrl);
@@ -122,6 +124,7 @@ check(fs.chmod, fs.chmodSync, fileUrl2, '0644');
 check(fs.chown, fs.chownSync, fileUrl2, 12, 34);
 check(fs.copyFile, fs.copyFileSync, fileUrl2, 'abc');
 check(fs.copyFile, fs.copyFileSync, 'abc', fileUrl2);
+check(fs.lchown, fs.lchownSync, fileUrl2, 12, 34);
 check(fs.link, fs.linkSync, fileUrl2, 'foobar');
 check(fs.link, fs.linkSync, 'foobar', fileUrl2);
 check(fs.lstat, fs.lstatSync, fileUrl2);

test/parallel/test-trace-events-fs-sync.js

@@ -60,6 +60,9 @@ tests['fs.sync.futimes'] = 'fs.writeFileSync("fs.txt", "123", "utf8");' +
                            'const fd = fs.openSync("fs.txt", "r+");' +
                            'fs.futimesSync(fd,1,1);' +
                            'fs.unlinkSync("fs.txt")';
+tests['fs.sync.lchown'] = 'fs.writeFileSync("fs.txt", "123", "utf8");' +
+                          'fs.lchownSync("fs.txt",' + uid + ',' + gid + ');' +
+                          'fs.unlinkSync("fs.txt")';
 tests['fs.sync.link'] = 'fs.writeFileSync("fs.txt", "123", "utf8");' +
                         'fs.linkSync("fs.txt", "linkx");' +
                         'fs.unlinkSync("linkx");' +