Skip to content

Commit

Permalink
fs: allow int64 offset in fs.read/readSync/fd.read
Browse files Browse the repository at this point in the history
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 #26563

PR-URL: #26572
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
  • Loading branch information
zbjornson authored and Trott committed Aug 17, 2019
1 parent 91a4cb7 commit 0bbda5e
Show file tree
Hide file tree
Showing 6 changed files with 69 additions and 8 deletions.
14 changes: 12 additions & 2 deletions lib/fs.js
Original file line number Diff line number Diff line change
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
7 changes: 6 additions & 1 deletion lib/internal/fs/promises.js
Original file line number Diff line number Diff line change
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
12 changes: 7 additions & 5 deletions src/node_file.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1851,7 +1851,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 +1869,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
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;
return false;
}

} // namespace node

#endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
Expand Down
5 changes: 5 additions & 0 deletions src/util.h
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
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

0 comments on commit 0bbda5e

Please sign in to comment.