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: allow position parameter to be a BigInt in read and readSync #36190

Closed
wants to merge 1 commit into from
+127 −10
Closed

fs: allow position parameter to be a BigInt in read and readSync #36190

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 4 additions & 4 deletions doc/api/fs.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2931,7 +2931,7 @@ changes:
* `buffer` {Buffer|TypedArray|DataView}
* `offset` {integer}
* `length` {integer}
* `position` {integer}
* `position` {integer|bigint}
* `callback` {Function}
* `err` {Error}
* `bytesRead` {integer}
Expand Down Expand Up @@ -2976,7 +2976,7 @@ changes:
* `buffer` {Buffer|TypedArray|DataView} **Default:** `Buffer.alloc(16384)`
* `offset` {integer} **Default:** `0`
* `length` {integer} **Default:** `buffer.length`
* `position` {integer} **Default:** `null`
* `position` {integer|bigint} **Default:** `null`
* `callback` {Function}
* `err` {Error}
* `bytesRead` {integer}
Expand Down Expand Up @@ -3273,7 +3273,7 @@ changes:
* `buffer` {Buffer|TypedArray|DataView}
* `offset` {integer}
* `length` {integer}
* `position` {integer}
* `position` {integer|bigint}
* Returns: {number}

Returns the number of `bytesRead`.
Expand All @@ -3300,7 +3300,7 @@ changes:
* `options` {Object}
* `offset` {integer} **Default:** `0`
* `length` {integer} **Default:** `buffer.length`
* `position` {integer} **Default:** `null`
* `position` {integer|bigint} **Default:** `null`
* Returns: {number}

Returns the number of `bytesRead`.
Expand Down
36 changes: 32 additions & 4 deletions lib/fs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ const {
BigIntPrototypeToString,
MathMax,
Number,
NumberIsSafeInteger,
ObjectCreate,
ObjectDefineProperties,
ObjectDefineProperty,
Expand Down Expand Up @@ -75,7 +74,8 @@ const {
ERR_FS_FILE_TOO_LARGE,
ERR_INVALID_ARG_VALUE,
ERR_INVALID_ARG_TYPE,
ERR_FEATURE_UNAVAILABLE_ON_PLATFORM
ERR_FEATURE_UNAVAILABLE_ON_PLATFORM,
ERR_OUT_OF_RANGE,
},
hideStackFrames,
uvErrmapGet,
Expand Down Expand Up @@ -545,9 +545,23 @@ function read(fd, buffer, offset, length, position, callback) {

validateOffsetLengthRead(offset, length, buffer.byteLength);

if (!NumberIsSafeInteger(position))
if (position == null)
position = -1;
Copy link
Member

@Trott Trott Dec 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Not necessarily something to be done in this PR and I haven't looked super-closely but I wonder if this bit of code means that the docs shouldn't say the default is null but instead should say the default is -1 (and be sure to explain what -1 means and make sure that passing -1 explicitly works). Saying the thing has to be a an integer (or bigint, as added in this PR), but then saying the default is something that is not an integer or bigint (null), seems confusing and not quite right.

Copy link
Contributor Author

@RaisinTen RaisinTen Dec 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I agree. I was also thinking about removing the null-check and using a default function parameter for position by setting it to -1 in the function prototype itself. Should I add that change?


if (typeof position === 'number') {
validateInteger(position, 'position');
} else if (typeof position === 'bigint') {
if (!(position >= -(2n ** 63n) && position <= 2n ** 63n - 1n)) {
throw new ERR_OUT_OF_RANGE('position',
`>= ${-(2n ** 63n)} && <= ${2n ** 63n - 1n}`,
position);
}
Comment on lines +554 to +558
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worth put this in a function to avoid duplicating the code twice.

Copy link
Contributor Author

@RaisinTen RaisinTen Jan 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Updated :)
I was thinking if I should also add a validateInt64 to internal/validators.js (in a different PR) because that might get rid of some repetitions in the validation.

Originally posted by @RaisinTen in #36190 (comment)

Like this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure this can be done in a future PR :)

} else {
throw new ERR_INVALID_ARG_TYPE('position',
['integer', 'bigint'],
position);
}

function wrapper(err, bytesRead) {
// Retain a reference to buffer so that it can't be GC'ed too soon.
callback(err, bytesRead || 0, buffer);
Expand Down Expand Up @@ -597,9 +611,23 @@ function readSync(fd, buffer, offset, length, position) {

validateOffsetLengthRead(offset, length, buffer.byteLength);

if (!NumberIsSafeInteger(position))
if (position == null)
position = -1;
Copy link

Choose a reason for hiding this comment

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

It could the optimized to have a else? Once that if (position == null), then position = -1, so it is a valid integer and not need be checked by the next ìf (typeof position === 'number')`, right?

if (position == null) {
    position = -1;
}
else if (typeof position === 'number') {
    // ...
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, that would work. Do you want to open a PR with this change?

Copy link

Choose a reason for hiding this comment

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

@aduh95 if possible, you can do it?

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 was actually thinking about doing what was discussed here: #36190 (comment)
That way, position does not need to be set to -1 if it has a null value.

Copy link
Contributor

Choose a reason for hiding this comment

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

@rentalhost see #37101.


if (typeof position === 'number') {
validateInteger(position, 'position');
} else if (typeof position === 'bigint') {
if (!(position >= -(2n ** 63n) && position <= 2n ** 63n - 1n)) {
throw new ERR_OUT_OF_RANGE('position',
`>= ${-(2n ** 63n)} && <= ${2n ** 63n - 1n}`,
position);
}
} else {
throw new ERR_INVALID_ARG_TYPE('position',
['integer', 'bigint'],
position);
}

const ctx = {};
const result = binding.read(fd, buffer, offset, length, position,
undefined, ctx);
Expand Down
7 changes: 5 additions & 2 deletions src/node_file.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ namespace node {
namespace fs {

using v8::Array;
using v8::BigInt;
using v8::Boolean;
using v8::Context;
using v8::EscapableHandleScope;
Expand Down Expand Up @@ -2038,8 +2039,10 @@ static void Read(const FunctionCallbackInfo<Value>& args) {
const size_t len = static_cast<size_t>(args[3].As<Int32>()->Value());
CHECK(Buffer::IsWithinBounds(off, len, buffer_length));

CHECK(IsSafeJsInt(args[4]));
const int64_t pos = args[4].As<Integer>()->Value();
CHECK(IsSafeJsInt(args[4]) || args[4]->IsBigInt());
const int64_t pos = args[4]->IsNumber() ?
args[4].As<Integer>()->Value() :
args[4].As<BigInt>()->Int64Value();

char* buf = buffer_data + off;
uv_buf_t uvbuf = uv_buf_init(buf, len);
Expand Down
86 changes: 86 additions & 0 deletions test/parallel/test-fs-read-type.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,47 @@ assert.throws(() => {
'It must be >= 0. Received -1'
});

[true, () => {}, {}, ''].forEach((value) => {
assert.throws(() => {
fs.read(fd,
Buffer.allocUnsafe(expected.length),
0,
expected.length,
value,
common.mustNotCall());
}, {
code: 'ERR_INVALID_ARG_TYPE',
name: 'TypeError'
});
});

[0.5, 2 ** 53, 2n ** 63n].forEach((value) => {
assert.throws(() => {
fs.read(fd,
Buffer.allocUnsafe(expected.length),
0,
expected.length,
value,
common.mustNotCall());
}, {
code: 'ERR_OUT_OF_RANGE',
name: 'RangeError'
});
});

fs.read(fd,
Buffer.allocUnsafe(expected.length),
0,
expected.length,
0n,
common.mustCall());

fs.read(fd,
Buffer.allocUnsafe(expected.length),
0,
expected.length,
2n ** 53n - 1n,
common.mustCall());

assert.throws(
() => fs.readSync(fd, expected.length, 0, 'utf-8'),
Expand Down Expand Up @@ -151,3 +192,48 @@ assert.throws(() => {
message: 'The value of "length" is out of range. ' +
'It must be <= 4. Received 5'
});

[true, () => {}, {}, ''].forEach((value) => {
assert.throws(() => {
fs.readSync(fd,
Buffer.allocUnsafe(expected.length),
0,
expected.length,
value);
}, {
code: 'ERR_INVALID_ARG_TYPE',
name: 'TypeError'
});
});

[0.5, 2 ** 53, 2n ** 63n].forEach((value) => {
assert.throws(() => {
fs.readSync(fd,
Buffer.allocUnsafe(expected.length),
0,
expected.length,
value);
}, {
code: 'ERR_OUT_OF_RANGE',
name: 'RangeError'
});
});

fs.readSync(fd,
Buffer.allocUnsafe(expected.length),
0,
expected.length,
0n);

try {
fs.readSync(fd,
Buffer.allocUnsafe(expected.length),
0,
expected.length,
2n ** 53n - 1n);
} catch (err) {
// On systems where max file size is below 2^53-1, we'd expect a EFBIG error.
// This is not using `assert.throws` because the above call should not raise
// any error on systems that allows file of that size.
if (err.code !== 'EFBIG') throw err;
}