From 85987450df87aaf74fd70b6106d3ace43d3d4762 Mon Sep 17 00:00:00 2001 From: Darshan Sen Date: Wed, 31 Mar 2021 18:44:52 +0530 Subject: [PATCH] fs: fix chown abort This syncs the type assertion introduced in the referenced PR in the C++ side. Since chown, lchown, and fchown can accept -1 as a value for uid and gid, we should also accept signed integers from the JS side. Fixes: https://github.com/nodejs/node/issues/37995 Refs: https://github.com/nodejs/node/pull/31694 PR-URL: https://github.com/nodejs/node/pull/38004 Reviewed-By: Anna Henningsen Reviewed-By: Rich Trott --- lib/internal/fs/promises.js | 16 +++++++++------- src/node_file.cc | 25 ++++++++++++------------- test/parallel/test-fs-promises.js | 8 ++++---- 3 files changed, 25 insertions(+), 24 deletions(-) diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js index d989f8818ca786..7b2c526b59850e 100644 --- a/lib/internal/fs/promises.js +++ b/lib/internal/fs/promises.js @@ -9,6 +9,9 @@ const kIoMaxLength = 2 ** 31 - 1; const kReadFileMaxChunkSize = 2 ** 14; const kWriteFileMaxChunkSize = 2 ** 14; +// 2 ** 32 - 1 +const kMaxUserId = 4294967295; + const { Error, MathMax, @@ -66,7 +69,6 @@ const { validateAbortSignal, validateBuffer, validateInteger, - validateUint32 } = require('internal/validators'); const pathModule = require('path'); const { promisify } = require('internal/util'); @@ -580,22 +582,22 @@ async function lchmod(path, mode) { async function lchown(path, uid, gid) { path = getValidatedPath(path); - validateUint32(uid, 'uid'); - validateUint32(gid, 'gid'); + validateInteger(uid, 'uid', -1, kMaxUserId); + validateInteger(gid, 'gid', -1, kMaxUserId); return binding.lchown(pathModule.toNamespacedPath(path), uid, gid, kUsePromises); } async function fchown(handle, uid, gid) { - validateUint32(uid, 'uid'); - validateUint32(gid, 'gid'); + validateInteger(uid, 'uid', -1, kMaxUserId); + validateInteger(gid, 'gid', -1, kMaxUserId); return binding.fchown(handle.fd, uid, gid, kUsePromises); } async function chown(path, uid, gid) { path = getValidatedPath(path); - validateUint32(uid, 'uid'); - validateUint32(gid, 'gid'); + validateInteger(uid, 'uid', -1, kMaxUserId); + validateInteger(gid, 'gid', -1, kMaxUserId); return binding.chown(pathModule.toNamespacedPath(path), uid, gid, kUsePromises); } diff --git a/src/node_file.cc b/src/node_file.cc index d5e26a8d4aaa0c..cce3540a49027f 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -69,7 +69,6 @@ using v8::ObjectTemplate; using v8::Promise; using v8::String; using v8::Symbol; -using v8::Uint32; using v8::Undefined; using v8::Value; @@ -2178,11 +2177,11 @@ static void Chown(const FunctionCallbackInfo& args) { BufferValue path(env->isolate(), args[0]); CHECK_NOT_NULL(*path); - CHECK(args[1]->IsUint32()); - const uv_uid_t uid = static_cast(args[1].As()->Value()); + CHECK(IsSafeJsInt(args[1])); + const uv_uid_t uid = static_cast(args[1].As()->Value()); - CHECK(args[2]->IsUint32()); - const uv_gid_t gid = static_cast(args[2].As()->Value()); + CHECK(IsSafeJsInt(args[2])); + const uv_gid_t gid = static_cast(args[2].As()->Value()); FSReqBase* req_wrap_async = GetReqWrap(args, 3); if (req_wrap_async != nullptr) { // chown(path, uid, gid, req) @@ -2211,11 +2210,11 @@ static void FChown(const FunctionCallbackInfo& args) { CHECK(args[0]->IsInt32()); const int fd = args[0].As()->Value(); - CHECK(args[1]->IsUint32()); - const uv_uid_t uid = static_cast(args[1].As()->Value()); + CHECK(IsSafeJsInt(args[1])); + const uv_uid_t uid = static_cast(args[1].As()->Value()); - CHECK(args[2]->IsUint32()); - const uv_gid_t gid = static_cast(args[2].As()->Value()); + CHECK(IsSafeJsInt(args[2])); + const uv_gid_t gid = static_cast(args[2].As()->Value()); FSReqBase* req_wrap_async = GetReqWrap(args, 3); if (req_wrap_async != nullptr) { // fchown(fd, uid, gid, req) @@ -2241,11 +2240,11 @@ static void LChown(const FunctionCallbackInfo& args) { BufferValue path(env->isolate(), args[0]); CHECK_NOT_NULL(*path); - CHECK(args[1]->IsUint32()); - const uv_uid_t uid = static_cast(args[1].As()->Value()); + CHECK(IsSafeJsInt(args[1])); + const uv_uid_t uid = static_cast(args[1].As()->Value()); - CHECK(args[2]->IsUint32()); - const uv_gid_t gid = static_cast(args[2].As()->Value()); + CHECK(IsSafeJsInt(args[2])); + const uv_gid_t gid = static_cast(args[2].As()->Value()); FSReqBase* req_wrap_async = GetReqWrap(args, 3); if (req_wrap_async != nullptr) { // lchown(path, uid, gid, req) diff --git a/test/parallel/test-fs-promises.js b/test/parallel/test-fs-promises.js index f23df685eac072..b6d4260a6484d0 100644 --- a/test/parallel/test-fs-promises.js +++ b/test/parallel/test-fs-promises.js @@ -183,24 +183,24 @@ async function getHandle(dest) { assert.rejects( async () => { - await chown(dest, 1, -1); + await chown(dest, 1, -2); }, { code: 'ERR_OUT_OF_RANGE', name: 'RangeError', message: 'The value of "gid" is out of range. ' + - 'It must be >= 0 && < 4294967296. Received -1' + 'It must be >= -1 && <= 4294967295. Received -2' }); assert.rejects( async () => { - await handle.chown(1, -1); + await handle.chown(1, -2); }, { code: 'ERR_OUT_OF_RANGE', name: 'RangeError', message: 'The value of "gid" is out of range. ' + - 'It must be >= 0 && < 4294967296. Received -1' + 'It must be >= -1 && <= 4294967295. Received -2' }); await handle.close();