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

fs: allow int64 offset in fs.read/readSync/write/writeSync #26572

Closed
wants to merge 4 commits into from
Closed

fs: allow int64 offset in fs.read/readSync/write/writeSync #26572

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
75b6ec9
lib: rename validateInteger to validateSafeInteger
zbjornson Mar 12, 2019
a888eca
fs: allow int64 offset in fs.read/readSync/fd.read
zbjornson Mar 18, 2019
6789e96
fs: use IsSafeJsInt instead of IsNumber for ftruncate
zbjornson Mar 27, 2019
8f545bf
fs: allow int64 offset in fs.write/writeSync/fd.write
zbjornson Mar 27, 2019
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
32 changes: 24 additions & 8 deletions lib/fs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ const {
isUint32,
parseMode,
validateBuffer,
validateInteger,
validateSafeInteger,
validateInt32,
validateUint32
} = require('internal/validators');
Expand Down Expand Up @@ -453,7 +453,12 @@ function read(fd, buffer, offset, length, position, callback) {
validateBuffer(buffer);
callback = maybeCallback(callback);

offset |= 0;
if (offset == null) {
offset = 0;
} else {
validateSafeInteger(offset, 'offset');
}

length |= 0;

if (length === 0) {
Expand Down Expand Up @@ -490,7 +495,12 @@ function readSync(fd, buffer, offset, length, position) {
validateInt32(fd, 'fd', 0);
validateBuffer(buffer);

offset |= 0;
if (offset == null) {
offset = 0;
} else {
validateSafeInteger(offset, 'offset');
}

length |= 0;

if (length === 0) {
Expand Down Expand Up @@ -531,8 +541,11 @@ function write(fd, buffer, offset, length, position, callback) {

if (isArrayBufferView(buffer)) {
callback = maybeCallback(callback || position || length || offset);
if (typeof offset !== 'number')
if (offset == null || typeof offset === 'function') {
offset = 0;
zbjornson marked this conversation as resolved.
Show resolved Hide resolved
} else {
validateSafeInteger(offset, 'offset');
}
if (typeof length !== 'number')
length = buffer.length - offset;
if (typeof position !== 'number')
Expand Down Expand Up @@ -570,8 +583,11 @@ function writeSync(fd, buffer, offset, length, position) {
if (isArrayBufferView(buffer)) {
if (position === undefined)
position = null;
if (typeof offset !== 'number')
if (offset == null) {
offset = 0;
} else {
validateSafeInteger(offset, 'offset');
}
if (typeof length !== 'number')
length = buffer.byteLength - offset;
validateOffsetLengthWrite(offset, length, buffer.byteLength);
Expand Down Expand Up @@ -621,7 +637,7 @@ function truncate(path, len, callback) {
len = 0;
}

validateInteger(len, 'len');
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these changes be a part of this PR, as we are dealing only with offsets here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

bnoordhuis suggested the change (#26572 (comment)), and I isolated them in a separate commit (intending all commits in this PR to be merged, not squashed).

validateSafeInteger(len, 'len');
callback = maybeCallback(callback);
fs.open(path, 'r+', (er, fd) => {
if (er) return callback(er);
Expand Down Expand Up @@ -662,7 +678,7 @@ function ftruncate(fd, len = 0, callback) {
len = 0;
}
validateInt32(fd, 'fd', 0);
validateInteger(len, 'len');
validateSafeInteger(len, 'len');
len = Math.max(0, len);
const req = new FSReqCallback();
req.oncomplete = makeCallback(callback);
Expand All @@ -671,7 +687,7 @@ function ftruncate(fd, len = 0, callback) {

function ftruncateSync(fd, len = 0) {
validateInt32(fd, 'fd', 0);
validateInteger(len, 'len');
validateSafeInteger(len, 'len');
len = Math.max(0, len);
const ctx = {};
binding.ftruncate(fd, len, undefined, ctx);
Expand Down
4 changes: 2 additions & 2 deletions lib/internal/crypto/scrypt.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
const { AsyncWrap, Providers } = internalBinding('async_wrap');
const { Buffer } = require('buffer');
const { scrypt: _scrypt } = internalBinding('crypto');
const { validateInteger, validateUint32 } = require('internal/validators');
const { validateSafeInteger, validateUint32 } = require('internal/validators');
const {
ERR_CRYPTO_SCRYPT_INVALID_PARAMETER,
ERR_CRYPTO_SCRYPT_NOT_SUPPORTED,
Expand Down Expand Up @@ -108,7 +108,7 @@ function check(password, salt, keylen, options) {
}
if (options.maxmem !== undefined) {
maxmem = options.maxmem;
validateInteger(maxmem, 'maxmem', 0);
validateSafeInteger(maxmem, 'maxmem', 0);
}
if (N === 0) N = defaults.N;
if (r === 0) r = defaults.r;
Expand Down
16 changes: 12 additions & 4 deletions lib/internal/fs/promises.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ const {
const {
parseMode,
validateBuffer,
validateInteger,
validateSafeInteger,
validateUint32
} = require('internal/validators');
const pathModule = require('path');
Expand Down Expand Up @@ -206,7 +206,12 @@ async function read(handle, buffer, offset, length, position) {
validateFileHandle(handle);
validateBuffer(buffer);

offset |= 0;
if (offset == null) {
offset = 0;
} else {
validateSafeInteger(offset, 'offset');
}

length |= 0;

if (length === 0)
Expand Down Expand Up @@ -235,8 +240,11 @@ async function write(handle, buffer, offset, length, position) {
return { bytesWritten: 0, buffer };

if (isUint8Array(buffer)) {
if (typeof offset !== 'number')
if (offset == null) {
offset = 0;
} else {
validateSafeInteger(offset, 'offset');
}
if (typeof length !== 'number')
length = buffer.length - offset;
if (typeof position !== 'number')
Expand Down Expand Up @@ -270,7 +278,7 @@ async function truncate(path, len = 0) {

async function ftruncate(handle, len = 0) {
validateFileHandle(handle);
validateInteger(len, 'len');
validateSafeInteger(len, 'len');
len = Math.max(0, len);
return binding.ftruncate(handle.fd, len, kUsePromises);
}
Expand Down
7 changes: 3 additions & 4 deletions lib/internal/fs/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

const { Object, Reflect } = primordials;

const { Buffer, kMaxLength } = require('buffer');
const { Buffer } = require('buffer');
const {
codes: {
ERR_FS_INVALID_SYMLINK_TYPE,
Expand Down Expand Up @@ -476,9 +476,8 @@ const validateOffsetLengthWrite = hideStackFrames(
throw new ERR_OUT_OF_RANGE('offset', `<= ${byteLength}`, offset);
}

const max = byteLength > kMaxLength ? kMaxLength : byteLength;
if (length > max - offset) {
throw new ERR_OUT_OF_RANGE('length', `<= ${max - offset}`, length);
if (length > byteLength - offset) {
throw new ERR_OUT_OF_RANGE('length', `<= ${byteLength - offset}`, length);
}
}
);
Expand Down
4 changes: 2 additions & 2 deletions lib/internal/validators.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ function parseMode(value, name, def) {
throw new ERR_INVALID_ARG_VALUE(name, value, modeDesc);
}

const validateInteger = hideStackFrames(
const validateSafeInteger = hideStackFrames(
(value, name, min = MIN_SAFE_INTEGER, max = MAX_SAFE_INTEGER) => {
if (typeof value !== 'number')
throw new ERR_INVALID_ARG_TYPE(name, 'number', value);
Expand Down Expand Up @@ -161,7 +161,7 @@ module.exports = {
parseMode,
validateBuffer,
validateEncoding,
validateInteger,
validateSafeInteger,
validateInt32,
validateUint32,
validateString,
Expand Down
24 changes: 14 additions & 10 deletions src/node_file.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ constexpr char kPathSeparator = '/';
const char* const kPathSeparator = "\\/";
#endif

#define GET_OFFSET(a) ((a)->IsNumber() ? (a).As<Integer>()->Value() : -1)
#define GET_OFFSET(a) (IsSafeJsInt(a) ? (a).As<Integer>()->Value() : -1)
#define TRACE_NAME(name) "fs.sync." #name
#define GET_TRACE_ENABLED \
(*TRACE_EVENT_API_GET_CATEGORY_GROUP_ENABLED \
Expand Down Expand Up @@ -1130,7 +1130,7 @@ static void FTruncate(const FunctionCallbackInfo<Value>& args) {
CHECK(args[0]->IsInt32());
const int fd = args[0].As<Int32>()->Value();

CHECK(args[1]->IsNumber());
CHECK(IsSafeJsInt(args[1]));
const int64_t len = args[1].As<Integer>()->Value();

FSReqBase* req_wrap_async = GetReqWrap(env, args[2]);
Expand Down Expand Up @@ -1669,9 +1669,11 @@ static void WriteBuffer(const FunctionCallbackInfo<Value>& args) {
char* buffer_data = Buffer::Data(buffer_obj);
size_t buffer_length = Buffer::Length(buffer_obj);

CHECK(args[2]->IsInt32());
const size_t off = static_cast<size_t>(args[2].As<Int32>()->Value());
CHECK_LE(off, buffer_length);
CHECK(IsSafeJsInt(args[2]));
const int64_t off_64 = args[2].As<Integer>()->Value();
CHECK_GE(off_64, 0);
CHECK_LE(static_cast<uint64_t>(off_64), buffer_length);
addaleax marked this conversation as resolved.
Show resolved Hide resolved
const size_t off = static_cast<size_t>(off_64);

CHECK(args[3]->IsInt32());
const size_t len = static_cast<size_t>(args[3].As<Int32>()->Value());
Expand Down Expand Up @@ -1851,7 +1853,7 @@ static void WriteString(const FunctionCallbackInfo<Value>& args) {
*
* 0 fd int32. file descriptor
* 1 buffer instance of Buffer
* 2 offset int32. offset to start reading into inside buffer
* 2 offset int64. offset to start reading into inside buffer
* 3 length int32. length to read
* 4 position int64. file position - -1 for current position
*/
Expand All @@ -1869,15 +1871,17 @@ static void Read(const FunctionCallbackInfo<Value>& args) {
char* buffer_data = Buffer::Data(buffer_obj);
size_t buffer_length = Buffer::Length(buffer_obj);

CHECK(args[2]->IsInt32());
const size_t off = static_cast<size_t>(args[2].As<Int32>()->Value());
CHECK_LT(off, buffer_length);
CHECK(IsSafeJsInt(args[2]));
const int64_t off_64 = args[2].As<Integer>()->Value();
CHECK_GE(off_64, 0);
CHECK_LT(static_cast<uint64_t>(off_64), buffer_length);
const size_t off = static_cast<size_t>(off_64);

CHECK(args[3]->IsInt32());
const size_t len = static_cast<size_t>(args[3].As<Int32>()->Value());
CHECK(Buffer::IsWithinBounds(off, len, buffer_length));

CHECK(args[4]->IsNumber());
CHECK(IsSafeJsInt(args[4]));
const int64_t pos = args[4].As<Integer>()->Value();

char* buf = buffer_data + off;
Expand Down
12 changes: 12 additions & 0 deletions src/util-inl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@

#if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS

#include <cmath>
#include <cstring>
#include "util.h"

Expand Down Expand Up @@ -521,6 +522,17 @@ void ArrayBufferViewContents<T, S>::Read(v8::Local<v8::ArrayBufferView> abv) {
}
}

// ECMA262 20.1.2.5
inline bool IsSafeJsInt(v8::Local<v8::Value> v) {
if (!v->IsNumber()) return false;
double v_d = v.As<v8::Number>()->Value();
if (std::isnan(v_d)) return false;
if (std::isinf(v_d)) return false;
if (std::trunc(v_d) != v_d) return false; // not int
if (std::abs(v_d) <= static_cast<double>(kMaxSafeJsInteger)) return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Simply returning the result of this condition also should work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very true. This method is a verbatim port of the referenced part of ECMA-262 (https://www.ecma-international.org/ecma-262/9.0/index.html#sec-number.issafeinteger) though; I thought that would be clearer to follow exactly.

return false;
}

} // namespace node

#endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
Expand Down
5 changes: 5 additions & 0 deletions src/util.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,11 @@ void DumpBacktrace(FILE* fp);
#define UNREACHABLE(...) \
ERROR_AND_ABORT("Unreachable code reached" __VA_OPT__(": ") __VA_ARGS__)

// ECMA262 20.1.2.6 Number.MAX_SAFE_INTEGER (2^53-1)
constexpr int64_t kMaxSafeJsInteger = 9007199254740991;

inline bool IsSafeJsInt(v8::Local<v8::Value> v);

// TAILQ-style intrusive list node.
template <typename T>
class ListNode;
Expand Down
27 changes: 27 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 @@ -50,6 +50,20 @@ assert.throws(() => {
'Received -1'
});

assert.throws(() => {
fs.read(fd,
Buffer.allocUnsafe(expected.length),
NaN,
expected.length,
0,
common.mustNotCall());
}, {
code: 'ERR_OUT_OF_RANGE',
name: 'RangeError',
message: 'The value of "offset" is out of range. It must be an integer. ' +
'Received NaN'
});

assert.throws(() => {
fs.read(fd,
Buffer.allocUnsafe(expected.length),
Expand Down Expand Up @@ -103,6 +117,19 @@ assert.throws(() => {
'It must be >= 0 && <= 4. Received -1'
});

assert.throws(() => {
fs.readSync(fd,
Buffer.allocUnsafe(expected.length),
NaN,
expected.length,
0);
}, {
code: 'ERR_OUT_OF_RANGE',
name: 'RangeError',
message: 'The value of "offset" is out of range. It must be an integer. ' +
'Received NaN'
});

assert.throws(() => {
fs.readSync(fd,
Buffer.allocUnsafe(expected.length),
Expand Down
16 changes: 0 additions & 16 deletions test/parallel/test-fs-util-validateoffsetlengthwrite.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,22 +22,6 @@ const { kMaxLength } = require('buffer');
);
}

// RangeError when byteLength > kMaxLength, and length > kMaxLength - offset .
{
const offset = kMaxLength;
const length = 100;
const byteLength = kMaxLength + 1;
common.expectsError(
() => validateOffsetLengthWrite(offset, length, byteLength),
{
code: 'ERR_OUT_OF_RANGE',
type: RangeError,
message: 'The value of "length" is out of range. ' +
`It must be <= ${kMaxLength - offset}. Received ${length}`
}
);
}
BridgeAR marked this conversation as resolved.
Show resolved Hide resolved

// RangeError when byteLength < kMaxLength, and length > byteLength - offset .
{
const offset = kMaxLength - 150;
Expand Down
24 changes: 24 additions & 0 deletions test/parallel/test-fs-write-buffer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,3 +148,27 @@ tmpdir.refresh();
fs.write(fd, Uint8Array.from(expected), cb);
}));
}

// fs.write with invalid offset type
{
const filename = path.join(tmpdir.path, 'write7.txt');
fs.open(filename, 'w', 0o644, common.mustCall((err, fd) => {
assert.ifError(err);

assert.throws(() => {
fs.write(fd,
Buffer.from('abcd'),
NaN,
expected.length,
0,
common.mustNotCall());
}, {
code: 'ERR_OUT_OF_RANGE',
name: 'RangeError',
message: 'The value of "offset" is out of range. ' +
'It must be an integer. Received NaN'
});

fs.closeSync(fd);
}));
}