From 75b6ec9f37f07d110e46f1a6bc3c7efdfca68a46 Mon Sep 17 00:00:00 2001 From: Zach Bjornson Date: Tue, 12 Mar 2019 00:14:19 -0700 Subject: [PATCH 1/4] lib: rename validateInteger to validateSafeInteger --- lib/fs.js | 8 ++++---- lib/internal/crypto/scrypt.js | 4 ++-- lib/internal/fs/promises.js | 4 ++-- lib/internal/validators.js | 4 ++-- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index baba33a6a2fa8b..fbc8f8023582ce 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -85,7 +85,7 @@ const { isUint32, parseMode, validateBuffer, - validateInteger, + validateSafeInteger, validateInt32, validateUint32 } = require('internal/validators'); @@ -621,7 +621,7 @@ function truncate(path, len, callback) { len = 0; } - validateInteger(len, 'len'); + validateSafeInteger(len, 'len'); callback = maybeCallback(callback); fs.open(path, 'r+', (er, fd) => { if (er) return callback(er); @@ -662,7 +662,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); @@ -671,7 +671,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); diff --git a/lib/internal/crypto/scrypt.js b/lib/internal/crypto/scrypt.js index e2751f8fa58261..19a742c85c7b91 100644 --- a/lib/internal/crypto/scrypt.js +++ b/lib/internal/crypto/scrypt.js @@ -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, @@ -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; diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js index d807cb71f7127f..c8203055b57cfe 100644 --- a/lib/internal/fs/promises.js +++ b/lib/internal/fs/promises.js @@ -36,7 +36,7 @@ const { const { parseMode, validateBuffer, - validateInteger, + validateSafeInteger, validateUint32 } = require('internal/validators'); const pathModule = require('path'); @@ -270,7 +270,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); } diff --git a/lib/internal/validators.js b/lib/internal/validators.js index f74338ebcb8cef..88d1d0a42cf444 100644 --- a/lib/internal/validators.js +++ b/lib/internal/validators.js @@ -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); @@ -161,7 +161,7 @@ module.exports = { parseMode, validateBuffer, validateEncoding, - validateInteger, + validateSafeInteger, validateInt32, validateUint32, validateString, From a888eca2cd97e25a50dc78f05b52f5be87113078 Mon Sep 17 00:00:00 2001 From: Zach Bjornson Date: Mon, 18 Mar 2019 11:27:13 -0700 Subject: [PATCH 2/4] fs: allow int64 offset in fs.read/readSync/fd.read Since v10.10.0, 'buf' can be any DataView, meaning the largest byteLength can be Float64Array.BYTES_PER_ELEMENT * kMaxLength = 17,179,869,176. 'offset' can now be up to 2**53 - 1. This makes it possible to tile reads into a large buffer. Breaking: now throws if read offset is not a safe int, is null or is undefined. Fixes https://github.com/nodejs/node/issues/26563 --- lib/fs.js | 14 ++++++++++++-- lib/internal/fs/promises.js | 7 ++++++- src/node_file.cc | 12 +++++++----- src/util-inl.h | 12 ++++++++++++ src/util.h | 5 +++++ test/parallel/test-fs-read-type.js | 27 +++++++++++++++++++++++++++ 6 files changed, 69 insertions(+), 8 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index fbc8f8023582ce..7318d89a8f8616 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -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) { @@ -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) { diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js index c8203055b57cfe..67237c954babc1 100644 --- a/lib/internal/fs/promises.js +++ b/lib/internal/fs/promises.js @@ -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) diff --git a/src/node_file.cc b/src/node_file.cc index 138848c49da45e..5bd0237e7b0556 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -1851,7 +1851,7 @@ static void WriteString(const FunctionCallbackInfo& 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 */ @@ -1869,15 +1869,17 @@ static void Read(const FunctionCallbackInfo& 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(args[2].As()->Value()); - CHECK_LT(off, buffer_length); + CHECK(IsSafeJsInt(args[2])); + const int64_t off_64 = args[2].As()->Value(); + CHECK_GE(off_64, 0); + CHECK_LT(static_cast(off_64), buffer_length); + const size_t off = static_cast(off_64); CHECK(args[3]->IsInt32()); const size_t len = static_cast(args[3].As()->Value()); CHECK(Buffer::IsWithinBounds(off, len, buffer_length)); - CHECK(args[4]->IsNumber()); + CHECK(IsSafeJsInt(args[4])); const int64_t pos = args[4].As()->Value(); char* buf = buffer_data + off; diff --git a/src/util-inl.h b/src/util-inl.h index 5f8e7acb2a8e52..c06a0ae84c88f5 100644 --- a/src/util-inl.h +++ b/src/util-inl.h @@ -24,6 +24,7 @@ #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS +#include #include #include "util.h" @@ -521,6 +522,17 @@ void ArrayBufferViewContents::Read(v8::Local abv) { } } +// ECMA262 20.1.2.5 +inline bool IsSafeJsInt(v8::Local v) { + if (!v->IsNumber()) return false; + double v_d = v.As()->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(kMaxSafeJsInteger)) return true; + return false; +} + } // namespace node #endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS diff --git a/src/util.h b/src/util.h index a94e88f232fd31..c8bb44721f6250 100644 --- a/src/util.h +++ b/src/util.h @@ -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 v); + // TAILQ-style intrusive list node. template class ListNode; diff --git a/test/parallel/test-fs-read-type.js b/test/parallel/test-fs-read-type.js index b51df515898231..f5ac78a23026fd 100644 --- a/test/parallel/test-fs-read-type.js +++ b/test/parallel/test-fs-read-type.js @@ -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), @@ -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), From 6789e96209c0449f718eee47f4c5d7f8bb03decd Mon Sep 17 00:00:00 2001 From: Zach Bjornson Date: Wed, 27 Mar 2019 13:34:02 -0700 Subject: [PATCH 3/4] fs: use IsSafeJsInt instead of IsNumber for ftruncate --- src/node_file.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/node_file.cc b/src/node_file.cc index 5bd0237e7b0556..4301cdc7b4e410 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -1130,7 +1130,7 @@ static void FTruncate(const FunctionCallbackInfo& args) { CHECK(args[0]->IsInt32()); const int fd = args[0].As()->Value(); - CHECK(args[1]->IsNumber()); + CHECK(IsSafeJsInt(args[1])); const int64_t len = args[1].As()->Value(); FSReqBase* req_wrap_async = GetReqWrap(env, args[2]); From 8f545bf910c59d65a4a8e57c26223b820d229aa7 Mon Sep 17 00:00:00 2001 From: Zach Bjornson Date: Wed, 27 Mar 2019 15:23:59 -0700 Subject: [PATCH 4/4] fs: allow int64 offset in fs.write/writeSync/fd.write Ref https://github.com/nodejs/node/issues/26563 --- lib/fs.js | 10 ++++++-- lib/internal/fs/promises.js | 5 +++- lib/internal/fs/utils.js | 7 +++--- src/node_file.cc | 10 ++++---- .../test-fs-util-validateoffsetlengthwrite.js | 16 ------------- test/parallel/test-fs-write-buffer.js | 24 +++++++++++++++++++ 6 files changed, 45 insertions(+), 27 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 7318d89a8f8616..61ac16dfd2445c 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -541,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; + } else { + validateSafeInteger(offset, 'offset'); + } if (typeof length !== 'number') length = buffer.length - offset; if (typeof position !== 'number') @@ -580,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); diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js index 67237c954babc1..6364fb6e9db0e4 100644 --- a/lib/internal/fs/promises.js +++ b/lib/internal/fs/promises.js @@ -240,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') diff --git a/lib/internal/fs/utils.js b/lib/internal/fs/utils.js index e91918a00772cf..2924f7582e7663 100644 --- a/lib/internal/fs/utils.js +++ b/lib/internal/fs/utils.js @@ -2,7 +2,7 @@ const { Object, Reflect } = primordials; -const { Buffer, kMaxLength } = require('buffer'); +const { Buffer } = require('buffer'); const { codes: { ERR_FS_INVALID_SYMLINK_TYPE, @@ -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); } } ); diff --git a/src/node_file.cc b/src/node_file.cc index 4301cdc7b4e410..c4e3b83392c5cc 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -88,7 +88,7 @@ constexpr char kPathSeparator = '/'; const char* const kPathSeparator = "\\/"; #endif -#define GET_OFFSET(a) ((a)->IsNumber() ? (a).As()->Value() : -1) +#define GET_OFFSET(a) (IsSafeJsInt(a) ? (a).As()->Value() : -1) #define TRACE_NAME(name) "fs.sync." #name #define GET_TRACE_ENABLED \ (*TRACE_EVENT_API_GET_CATEGORY_GROUP_ENABLED \ @@ -1669,9 +1669,11 @@ static void WriteBuffer(const FunctionCallbackInfo& 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(args[2].As()->Value()); - CHECK_LE(off, buffer_length); + CHECK(IsSafeJsInt(args[2])); + const int64_t off_64 = args[2].As()->Value(); + CHECK_GE(off_64, 0); + CHECK_LE(static_cast(off_64), buffer_length); + const size_t off = static_cast(off_64); CHECK(args[3]->IsInt32()); const size_t len = static_cast(args[3].As()->Value()); diff --git a/test/parallel/test-fs-util-validateoffsetlengthwrite.js b/test/parallel/test-fs-util-validateoffsetlengthwrite.js index 9aca956bd505bf..f2f80ebdaa577a 100644 --- a/test/parallel/test-fs-util-validateoffsetlengthwrite.js +++ b/test/parallel/test-fs-util-validateoffsetlengthwrite.js @@ -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}` - } - ); -} - // RangeError when byteLength < kMaxLength, and length > byteLength - offset . { const offset = kMaxLength - 150; diff --git a/test/parallel/test-fs-write-buffer.js b/test/parallel/test-fs-write-buffer.js index 362571f2479c84..b2cd299bb186a7 100644 --- a/test/parallel/test-fs-write-buffer.js +++ b/test/parallel/test-fs-write-buffer.js @@ -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); + })); +}