Skip to content

Commit

Permalink
fs: fix chown abort
Browse files Browse the repository at this point in the history 
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: #37995
Refs: #31694

PR-URL: #38004
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
  • Loading branch information
@RaisinTen @MylesBorins
RaisinTen authored and MylesBorins committed Apr 4, 2021
1 parent fac2fc1 commit dc46073
Show file tree
Hide file tree
Showing 3 changed files with 25 additions and 24 deletions.
16 changes: 9 additions & 7 deletions lib/internal/fs/promises.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -71,7 +74,6 @@ const {
validateBoolean,
validateBuffer,
validateInteger,
validateUint32
} = require('internal/validators');
const pathModule = require('path');
const { promisify } = require('internal/util');
Expand Down Expand Up @@ -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);
}
Expand Down
25 changes: 12 additions & 13 deletions src/node_file.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -2184,11 +2183,11 @@ static void Chown(const FunctionCallbackInfo<Value>& args) {
BufferValue path(env->isolate(), args[0]);
CHECK_NOT_NULL(*path);

CHECK(args[1]->IsUint32());
const uv_uid_t uid = static_cast<uv_uid_t>(args[1].As<Uint32>()->Value());
CHECK(IsSafeJsInt(args[1]));
const uv_uid_t uid = static_cast<uv_uid_t>(args[1].As<Integer>()->Value());

CHECK(args[2]->IsUint32());
const uv_gid_t gid = static_cast<uv_gid_t>(args[2].As<Uint32>()->Value());
CHECK(IsSafeJsInt(args[2]));
const uv_gid_t gid = static_cast<uv_gid_t>(args[2].As<Integer>()->Value());

FSReqBase* req_wrap_async = GetReqWrap(args, 3);
if (req_wrap_async != nullptr) { // chown(path, uid, gid, req)
Expand Down Expand Up @@ -2217,11 +2216,11 @@ static void FChown(const FunctionCallbackInfo<Value>& args) {
CHECK(args[0]->IsInt32());
const int fd = args[0].As<Int32>()->Value();

CHECK(args[1]->IsUint32());
const uv_uid_t uid = static_cast<uv_uid_t>(args[1].As<Uint32>()->Value());
CHECK(IsSafeJsInt(args[1]));
const uv_uid_t uid = static_cast<uv_uid_t>(args[1].As<Integer>()->Value());

CHECK(args[2]->IsUint32());
const uv_gid_t gid = static_cast<uv_gid_t>(args[2].As<Uint32>()->Value());
CHECK(IsSafeJsInt(args[2]));
const uv_gid_t gid = static_cast<uv_gid_t>(args[2].As<Integer>()->Value());

FSReqBase* req_wrap_async = GetReqWrap(args, 3);
if (req_wrap_async != nullptr) { // fchown(fd, uid, gid, req)
Expand All @@ -2247,11 +2246,11 @@ static void LChown(const FunctionCallbackInfo<Value>& args) {
BufferValue path(env->isolate(), args[0]);
CHECK_NOT_NULL(*path);

CHECK(args[1]->IsUint32());
const uv_uid_t uid = static_cast<uv_uid_t>(args[1].As<Uint32>()->Value());
CHECK(IsSafeJsInt(args[1]));
const uv_uid_t uid = static_cast<uv_uid_t>(args[1].As<Integer>()->Value());

CHECK(args[2]->IsUint32());
const uv_gid_t gid = static_cast<uv_gid_t>(args[2].As<Uint32>()->Value());
CHECK(IsSafeJsInt(args[2]));
const uv_gid_t gid = static_cast<uv_gid_t>(args[2].As<Integer>()->Value());

FSReqBase* req_wrap_async = GetReqWrap(args, 3);
if (req_wrap_async != nullptr) { // lchown(path, uid, gid, req)
Expand Down
8 changes: 4 additions & 4 deletions test/parallel/test-fs-promises.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down

0 comments on commit dc46073

Please sign in to comment.