From 2e07d4596c0923bb27d4f4aa13356220bcc3c060 Mon Sep 17 00:00:00 2001
From: cjihrig <cjihrig@gmail.com>
Date: Sat, 23 Jun 2018 16:26:29 -0400
Subject: [PATCH] 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                    | 16 ------
 doc/api/documentation.md                   |  5 --
 doc/api/fs.md                              | 17 ++++--
 lib/fs.js                                  | 41 ++++++-------
 lib/internal/fs/promises.js                | 11 ++--
 src/node_file.cc                           | 32 ++++++++++-
 test/parallel/test-fs-lchown.js            | 67 ++++++++++++++++++++++
 test/parallel/test-fs-null-bytes.js        |  3 +
 test/parallel/test-trace-events-fs-sync.js |  3 +
 9 files changed, 140 insertions(+), 55 deletions(-)
 create mode 100644 test/parallel/test-fs-lchown.js

diff --git a/doc/api/deprecations.md b/doc/api/deprecations.md
index 40b48e8bef2dbc..6882f41625f6fc 100644
--- a/doc/api/deprecations.md
+++ b/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
 
@@ -1037,8 +1023,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
diff --git a/doc/api/documentation.md b/doc/api/documentation.md
index a5909e3268d947..f9fa9591af9805 100644
--- a/doc/api/documentation.md
+++ b/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
diff --git a/doc/api/fs.md b/doc/api/fs.md
index e244f01097cef3..111c6b4deaebbf 100644
--- a/doc/api/fs.md
+++ b/doc/api/fs.md
@@ -1903,8 +1903,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
@@ -1926,7 +1928,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}
@@ -3899,7 +3904,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}
@@ -3908,7 +3917,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
diff --git a/lib/fs.js b/lib/fs.js
index 4a7eabc5117174..9683748ba91efe 100644
--- a/lib/fs.js
+++ b/lib/fs.js
@@ -997,31 +997,24 @@ function chmodSync(path, mode) {
 }
 
 function lchown(path, uid, gid, callback) {
-  callback = maybeCallback(callback);
-  fs.open(path, O_WRONLY | O_SYMLINK, function(err, fd) {
-    if (err) {
-      callback(err);
-      return;
-    }
-    // Prefer to return the chown error, if one occurs,
-    // but still try to close, and report closing errors if they occur.
-    fs.fchown(fd, uid, gid, function(err) {
-      fs.close(fd, function(err2) {
-        callback(err || err2);
-      });
-    });
-  });
+  callback = makeCallback(callback);
+  path = getPathFromURL(path);
+  validatePath(path);
+  validateUint32(uid, 'uid');
+  validateUint32(gid, 'gid');
+  const req = new FSReqWrap();
+  req.oncomplete = callback;
+  binding.lchown(pathModule.toNamespacedPath(path), uid, gid, req);
 }
 
 function lchownSync(path, uid, gid) {
-  const fd = fs.openSync(path, O_WRONLY | O_SYMLINK);
-  let ret;
-  try {
-    ret = fs.fchownSync(fd, uid, gid);
-  } finally {
-    fs.closeSync(fd);
-  }
-  return ret;
+  path = getPathFromURL(path);
+  validatePath(path);
+  validateUint32(uid, 'uid');
+  validateUint32(gid, 'gid');
+  const ctx = { path };
+  binding.lchown(pathModule.toNamespacedPath(path), uid, gid, undefined, ctx);
+  handleErrorFromBinding(ctx);
 }
 
 function fchown(fd, uid, gid, callback) {
@@ -1753,8 +1746,8 @@ module.exports = fs = {
   ftruncateSync,
   futimes,
   futimesSync,
-  lchown: constants.O_SYMLINK !== undefined ? lchown : undefined,
-  lchownSync: constants.O_SYMLINK !== undefined ? lchownSync : undefined,
+  lchown,
+  lchownSync,
   lchmod: constants.O_SYMLINK !== undefined ? lchmod : undefined,
   lchmodSync: constants.O_SYMLINK !== undefined ? lchmodSync : undefined,
   link,
diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js
index c966e0a897900a..befcccb2769589 100644
--- a/lib/internal/fs/promises.js
+++ b/lib/internal/fs/promises.js
@@ -377,11 +377,12 @@ async function lchmod(path, mode) {
 }
 
 async function lchown(path, uid, gid) {
-  if (O_SYMLINK === undefined)
-    throw new ERR_METHOD_NOT_IMPLEMENTED();
-
-  const fd = await open(path, O_WRONLY | O_SYMLINK);
-  return fchown(fd, uid, gid).finally(fd.close.bind(fd));
+  path = getPathFromURL(path);
+  validatePath(path);
+  validateUint32(uid, 'uid');
+  validateUint32(gid, 'gid');
+  return binding.lchown(pathModule.toNamespacedPath(path),
+                        uid, gid, kUsePromises);
 }
 
 async function fchown(handle, uid, gid) {
diff --git a/src/node_file.cc b/src/node_file.cc
index a63e90fafb2926..ed1799da586e24 100644
--- a/src/node_file.cc
+++ b/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);
diff --git a/test/parallel/test-fs-lchown.js b/test/parallel/test-fs-lchown.js
new file mode 100644
index 00000000000000..23469eb279aede
--- /dev/null
+++ b/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);
+  }));
+}
diff --git a/test/parallel/test-fs-null-bytes.js b/test/parallel/test-fs-null-bytes.js
index c409e6f4884353..f5a56b01d5f883 100644
--- a/test/parallel/test-fs-null-bytes.js
+++ b/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);
diff --git a/test/parallel/test-trace-events-fs-sync.js b/test/parallel/test-trace-events-fs-sync.js
index 7ab480f14a2e9e..77ec0863406379 100644
--- a/test/parallel/test-trace-events-fs-sync.js
+++ b/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");' +