From dc460731e82edd8c7fc293fdff5973daa8e6f9d3 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 5db2f7759c5238..1eac04665d85fc 100644 --- a/lib/internal/fs/promises.js +++ b/lib/internal/fs/promises.js @@ -8,6 +8,9 @@ const kReadFileBufferLength = 512 * 1024; const kReadFileUnknownBufferLength = 64 * 1024; const kWriteFileMaxChunkSize = 512 * 1024; +// 2 ** 32 - 1 +const kMaxUserId = 4294967295; + const { ArrayPrototypePush, Error, @@ -71,7 +74,6 @@ const { validateBoolean, validateBuffer, validateInteger, - validateUint32 } = require('internal/validators'); const pathModule = require('path'); const { promisify } = require('internal/util'); @@ -615,22 +617,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 0eb882b49a88bf..7a61ce22af1c70 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -71,7 +71,6 @@ using v8::ObjectTemplate; using v8::Promise; using v8::String; using v8::Symbol; -using v8::Uint32; using v8::Undefined; using v8::Value; @@ -2184,11 +2183,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) @@ -2217,11 +2216,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) @@ -2247,11 +2246,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 e01fc2ec6ad1e7..bb9a3257e11fae 100644 --- a/test/parallel/test-fs-promises.js +++ b/test/parallel/test-fs-promises.js @@ -191,24 +191,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();