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: #21498Fixes: #19868
Reviewed-By: Wyatt Preul <wpreul@gmail.com>

Author: cjihrig

doc/api/deprecations.md

@@ -351,20 +351,6 @@ Type: Documentation-only
 
 The [`fs.lchmodSync(path, mode)`][] API is deprecated.
 
-<aid="DEP0037"></a>
-### DEP0037: fs.lchown(path, uid, gid, callback)
-
-Type: Documentation-only
-
-The [`fs.lchown(path, uid, gid, callback)`][] API is deprecated.
-
-<aid="DEP0038"></a>
-### DEP0038: fs.lchownSync(path, uid, gid)
-
-Type: Documentation-only
-
-The [`fs.lchownSync(path, uid, gid)`][] API is deprecated.
-
 <aid="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

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

@@ -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

lib/fs.js

@@ -997,31 +997,24 @@ function chmodSync(path, mode) {
 }
 
 functionlchown(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');
+constreq=newFSReqWrap();
+req.oncomplete=callback;
+binding.lchown(pathModule.toNamespacedPath(path),uid,gid,req);
 }
 
 functionlchownSync(path,uid,gid){
-constfd=fs.openSync(path,O_WRONLY|O_SYMLINK);
-letret;
-try{
-ret=fs.fchownSync(fd,uid,gid);
-}finally{
-fs.closeSync(fd);
-}
-returnret;
+path=getPathFromURL(path);
+validatePath(path);
+validateUint32(uid,'uid');
+validateUint32(gid,'gid');
+constctx={ path };
+binding.lchown(pathModule.toNamespacedPath(path),uid,gid,undefined,ctx);
+handleErrorFromBinding(ctx);
 }
 
 functionfchown(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,

lib/internal/fs/promises.js

@@ -377,11 +377,12 @@ async function lchmod(path, mode) {
 }
 
 asyncfunctionlchown(path,uid,gid){
-if(O_SYMLINK===undefined)
-thrownewERR_METHOD_NOT_IMPLEMENTED();
-
-constfd=awaitopen(path,O_WRONLY|O_SYMLINK);
-returnfchown(fd,uid,gid).finally(fd.close.bind(fd));
+path=getPathFromURL(path);
+validatePath(path);
+validateUint32(uid,'uid');
+validateUint32(gid,'gid');
+returnbinding.lchown(pathModule.toNamespacedPath(path),
+uid,gid,kUsePromises);
 }
 
 asyncfunctionfchown(handle,uid,gid){

src/node_file.cc

@@ -1771,6 +1771,36 @@ static void FChown(const FunctionCallbackInfo<Value>& args) {
 }
 
 
+staticvoidLChown(const FunctionCallbackInfo<Value>& args) {
+ Environment* env = Environment::GetCurrent(args);
+
+constint argc = args.Length();
+CHECK_GE(argc, 3);
+
+ BufferValue path(env->isolate(), args[0]);
+CHECK_NOT_NULL(*path);
+
+CHECK(args[1]->IsUint32());
+constuv_uid_t uid = static_cast<uv_uid_t>(args[1].As<Uint32>()->Value());
+
+CHECK(args[2]->IsUint32());
+constuv_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);
+ }
+}
+
+
 staticvoidUTimes(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';
+
+constcommon=require('../common');
+consttmpdir=require('../common/tmpdir');
+constassert=require('assert');
+constfs=require('fs');
+constpath=require('path');
+const{ promises }=fs;
+
+common.crashOnUnhandledRejection();
+
+// Validate the path argument.
+[false,1,{},[],null,undefined].forEach((i)=>{
+consterr={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)=>{
+consterr={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){
+consttestFile=path.join(tmpdir.path,path.basename(__filename));
+constuid=process.geteuid();
+constgid=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);
+awaitpromises.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");'+