From 7ff50f9e9cf5459c60f10f3919e79223c8b7446c 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 df6371056d1..07731667205 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
 
@@ -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
diff --git a/doc/api/documentation.md b/doc/api/documentation.md
index a5909e3268d..f9fa9591af9 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 9b518cbac1b..f648527435f 100644
--- a/doc/api/fs.md
+++ b/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
diff --git a/lib/fs.js b/lib/fs.js
index f80bb6ac33c..d770c6ecd2d 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 750d0d75797..0cac39534ab 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 a63e90fafb2..ed1799da586 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 00000000000..23469eb279a
--- /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 c409e6f4884..f5a56b01d5f 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 7ab480f14a2..77ec0863406 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");' +