From 5402b26dc699515e46f876464a02bc108084d5ea Mon Sep 17 00:00:00 2001 From: Darshan Sen Date: Wed, 31 Mar 2021 18:44:52 +0530 Subject: [PATCH 1/5] 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 --- src/node_file.cc | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/node_file.cc b/src/node_file.cc index 502f2968b6c08a..b08deaa93c04db 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -2184,11 +2184,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 +2217,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 +2247,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) From 0c5fea2dc193ad0f9313cb4d6e6ca4497a02fdb3 Mon Sep 17 00:00:00 2001 From: Darshan Sen Date: Wed, 31 Mar 2021 19:11:33 +0530 Subject: [PATCH 2/5] fixup! fs: fix chown abort --- src/node_file.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/src/node_file.cc b/src/node_file.cc index b08deaa93c04db..b8902f6b348eca 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; From d9c96b62218e0bfa0e48b767513811db3547b558 Mon Sep 17 00:00:00 2001 From: Darshan Sen Date: Wed, 31 Mar 2021 19:10:42 +0530 Subject: [PATCH 3/5] update validation in fs promises --- lib/internal/fs/promises.js | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js index 87704a60074f18..896b0e1a008240 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, @@ -72,7 +75,6 @@ const { validateBoolean, validateBuffer, validateInteger, - validateUint32 } = require('internal/validators'); const pathModule = require('path'); const { promisify } = require('internal/util'); @@ -620,22 +622,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); } From 97e3d0c25d8c0138acd52959485c457f571481f1 Mon Sep 17 00:00:00 2001 From: Darshan Sen Date: Wed, 31 Mar 2021 20:41:51 +0530 Subject: [PATCH 4/5] fixup! update validation in fs promises --- test/parallel/test-fs-promises.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/parallel/test-fs-promises.js b/test/parallel/test-fs-promises.js index e01fc2ec6ad1e7..944ed9b316c7f1 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 && < 4294967296. 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 && < 4294967296. Received -2' }); await handle.close(); From 9749f6487d772c9fb8988a4af7bd40d054206fc2 Mon Sep 17 00:00:00 2001 From: Darshan Sen Date: Thu, 1 Apr 2021 18:17:37 +0530 Subject: [PATCH 5/5] fixup! update validation in fs promises --- test/parallel/test-fs-promises.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/parallel/test-fs-promises.js b/test/parallel/test-fs-promises.js index 944ed9b316c7f1..bb9a3257e11fae 100644 --- a/test/parallel/test-fs-promises.js +++ b/test/parallel/test-fs-promises.js @@ -197,7 +197,7 @@ async function getHandle(dest) { code: 'ERR_OUT_OF_RANGE', name: 'RangeError', message: 'The value of "gid" is out of range. ' + - 'It must be >= -1 && < 4294967296. Received -2' + 'It must be >= -1 && <= 4294967295. Received -2' }); assert.rejects( @@ -208,7 +208,7 @@ async function getHandle(dest) { code: 'ERR_OUT_OF_RANGE', name: 'RangeError', message: 'The value of "gid" is out of range. ' + - 'It must be >= -1 && < 4294967296. Received -2' + 'It must be >= -1 && <= 4294967295. Received -2' }); await handle.close();