Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fs: fix chown abort #38004

Closed
wants to merge 5 commits into from
Closed

fs: fix chown abort #38004

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5402b26
fs: fix chown abort
RaisinTen Mar 31, 2021
0c5fea2
fixup! fs: fix chown abort
RaisinTen Mar 31, 2021
d9c96b6
update validation in fs promises
RaisinTen Mar 31, 2021
97e3d0c
fixup! update validation in fs promises
RaisinTen Mar 31, 2021
9749f64
fixup! update validation in fs promises
RaisinTen Apr 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 9 additions & 7 deletions lib/internal/fs/promises.js
View file
Open in desktop
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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this value? Is this documented somewhere? Could you add a comment explaining where does it come from please?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually pasted it from:

node/lib/fs.js

Lines 139 to 140 in 82bc5c3

// 2 ** 32 - 1
const kMaxUserId = 4294967295;

since I could not require it because of a circular dependency.
Should we move this (and possibly other k* constants) into lib/internal/fs/utils.js and require them from there?


const {
ArrayPrototypePush,
Error,
Expand Down Expand Up @@ -72,7 +75,6 @@ const {
validateBoolean,
validateBuffer,
validateInteger,
validateUint32
} = require('internal/validators');
const pathModule = require('path');
const { promisify } = require('internal/util');
Expand Down Expand Up @@ -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);
}
Expand Down
25 changes: 12 additions & 13 deletions src/node_file.cc
View file
Open in desktop
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
Open in desktop
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