fs: special input -1 on chown, lchown and fchown

PR-URL: https://github.com/nodejs/node/pull/58836
Fixes: https://github.com/nodejs/node/issues/58826
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

src/node_file.cc

@@ -2853,10 +2853,10 @@ static void Chown(const FunctionCallbackInfo<Value>& args) {
   ToNamespacedPath(env, &path);
 
   CHECK(IsSafeJsInt(args[1]));
-  const uv_uid_t uid = FromV8Value<uv_uid_t>(args[1]);
+  const auto uid = FromV8Value<uv_uid_t, true>(args[1]);
 
   CHECK(IsSafeJsInt(args[2]));
-  const uv_gid_t gid = FromV8Value<uv_gid_t>(args[2]);
+  const auto gid = FromV8Value<uv_gid_t, true>(args[2]);
 
   if (argc > 3) {  // chown(path, uid, gid, req)
     FSReqBase* req_wrap_async = GetReqWrap(args, 3);
@@ -2898,10 +2898,10 @@ static void FChown(const FunctionCallbackInfo<Value>& args) {
   }
 
   CHECK(IsSafeJsInt(args[1]));
-  const uv_uid_t uid = static_cast<uv_uid_t>(args[1].As<Integer>()->Value());
+  const auto uid = FromV8Value<uv_uid_t, true>(args[1]);
 
   CHECK(IsSafeJsInt(args[2]));
-  const uv_gid_t gid = static_cast<uv_gid_t>(args[2].As<Integer>()->Value());
+  const auto gid = FromV8Value<uv_gid_t, true>(args[2]);
 
   if (argc > 3) {  // fchown(fd, uid, gid, req)
     FSReqBase* req_wrap_async = GetReqWrap(args, 3);
@@ -2928,10 +2928,10 @@ static void LChown(const FunctionCallbackInfo<Value>& args) {
   ToNamespacedPath(env, &path);
 
   CHECK(IsSafeJsInt(args[1]));
-  const uv_uid_t uid = FromV8Value<uv_uid_t>(args[1]);
+  const auto uid = FromV8Value<uv_uid_t, true>(args[1]);
 
   CHECK(IsSafeJsInt(args[2]));
-  const uv_gid_t gid = FromV8Value<uv_gid_t>(args[2]);
+  const auto gid = FromV8Value<uv_gid_t, true>(args[2]);
 
   if (argc > 3) {  // lchown(path, uid, gid, req)
     FSReqBase* req_wrap_async = GetReqWrap(args, 3);

src/util-inl.h

@@ -627,19 +627,24 @@ constexpr std::string_view FastStringKey::as_string_view() const {
 
 // Converts a V8 numeric value to a corresponding C++ primitive or enum type.
 template <typename T,
+          bool loose = false,
           typename = std::enable_if_t<std::numeric_limits<T>::is_specialized ||
                                       std::is_enum_v<T>>>
 T FromV8Value(v8::Local<v8::Value> value) {
   if constexpr (std::is_enum_v<T>) {
     using Underlying = std::underlying_type_t<T>;
-    return static_cast<T>(FromV8Value<Underlying>(value));
+    return static_cast<T>(FromV8Value<Underlying, loose>(value));
   } else if constexpr (std::is_integral_v<T> && std::is_unsigned_v<T>) {
     static_assert(
         std::numeric_limits<T>::max() <= std::numeric_limits<uint32_t>::max() &&
             std::numeric_limits<T>::min() >=
                 std::numeric_limits<uint32_t>::min(),
         "Type is out of unsigned integer range");
-    CHECK(value->IsUint32());
+    if constexpr (!loose) {
+      CHECK(value->IsUint32());
+    } else {
+      CHECK(value->IsNumber());
+    }
     return static_cast<T>(value.As<v8::Uint32>()->Value());
   } else if constexpr (std::is_integral_v<T> && std::is_signed_v<T>) {
     static_assert(
@@ -647,7 +652,11 @@ T FromV8Value(v8::Local<v8::Value> value) {
             std::numeric_limits<T>::min() >=
                 std::numeric_limits<int32_t>::min(),
         "Type is out of signed integer range");
-    CHECK(value->IsInt32());
+    if constexpr (!loose) {
+      CHECK(value->IsInt32());
+    } else {
+      CHECK(value->IsNumber());
+    }
     return static_cast<T>(value.As<v8::Int32>()->Value());
   } else {
     static_assert(std::is_floating_point_v<T>,

test/parallel/test-fs-chown-negative-one.js

@@ -0,0 +1,32 @@
+'use strict';
+
+const common = require('../common');
+const assert = require('assert');
+const fs = require('fs');
+const path = require('path');
+const tmpdir = require('../common/tmpdir');
+
+tmpdir.refresh();
+
+const testFile = path.join(tmpdir.path, 'chown-test-file.txt');
+
+fs.writeFileSync(testFile, 'test content for chown');
+
+const stats = fs.statSync(testFile);
+const uid = stats.uid;
+const gid = stats.gid;
+
+// -1 for uid and gid means "don't change the value"
+{
+  fs.chown(testFile, -1, -1, common.mustSucceed(() => {
+    const stats = fs.statSync(testFile);
+    assert.strictEqual(stats.uid, uid);
+    assert.strictEqual(stats.gid, gid);
+  }));
+}
+{
+  fs.chownSync(testFile, -1, -1);
+  const stats = fs.statSync(testFile);
+  assert.strictEqual(stats.uid, uid);
+  assert.strictEqual(stats.gid, gid);
+}

test/parallel/test-fs-fchown-negative-one.js

@@ -0,0 +1,42 @@
+'use strict';
+
+const common = require('../common');
+const assert = require('assert');
+const fs = require('fs');
+const path = require('path');
+const tmpdir = require('../common/tmpdir');
+
+tmpdir.refresh();
+
+const testFilePath = path.join(tmpdir.path, 'fchown-test-file.txt');
+
+fs.writeFileSync(testFilePath, 'test content for fchown');
+
+{
+  const fd = fs.openSync(testFilePath, 'r+');
+  const stats = fs.fstatSync(fd);
+  const uid = stats.uid;
+  const gid = stats.gid;
+
+  fs.fchown(fd, -1, -1, common.mustSucceed(() => {
+    const stats = fs.fstatSync(fd);
+    assert.strictEqual(stats.uid, uid);
+    assert.strictEqual(stats.gid, gid);
+    fs.closeSync(fd);
+  }));
+}
+
+// Test sync fchown with -1 values
+{
+  const fd = fs.openSync(testFilePath, 'r+');
+  const stats = fs.fstatSync(fd);
+  const uid = stats.uid;
+  const gid = stats.gid;
+
+  fs.fchownSync(fd, -1, -1);
+  const statsAfter = fs.fstatSync(fd);
+  assert.strictEqual(statsAfter.uid, uid);
+  assert.strictEqual(statsAfter.gid, gid);
+
+  fs.closeSync(fd);
+}

test/parallel/test-fs-lchown-negative-one.js

@@ -0,0 +1,34 @@
+'use strict';
+
+const common = require('../common');
+const assert = require('assert');
+const fs = require('fs');
+const path = require('path');
+const tmpdir = require('../common/tmpdir');
+
+tmpdir.refresh();
+
+const testFile = path.join(tmpdir.path, 'lchown-test-file.txt');
+const testLink = path.join(tmpdir.path, 'lchown-test-link');
+
+fs.writeFileSync(testFile, 'test content for lchown');
+fs.symlinkSync(testFile, testLink);
+
+const stats = fs.lstatSync(testLink);
+const uid = stats.uid;
+const gid = stats.gid;
+
+// -1 for uid and gid means "don't change the value"
+{
+  fs.lchown(testLink, -1, -1, common.mustSucceed(() => {
+    const stats = fs.lstatSync(testLink);
+    assert.strictEqual(stats.uid, uid);
+    assert.strictEqual(stats.gid, gid);
+  }));
+}
+{
+  fs.lchownSync(testLink, -1, -1);
+  const stats = fs.lstatSync(testLink);
+  assert.strictEqual(stats.uid, uid);
+  assert.strictEqual(stats.gid, gid);
+}