fs: allow int64 offset in fs.read/readSync/fd.read

zbjornson · Trott · commit 0bbda5e5aede · 2019-08-16T22:09:27.000-07:00
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>
diff --git 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
@@ -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
@@ -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
  */
@@ -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;
diff --git a/src/util-inl.h b/src/util-inl.h
@@ -24,6 +24,7 @@
 
 #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
 
+#include <cmath>
 #include <cstring>
 #include "util.h"
 
@@ -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
diff --git 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<v8::Value> v);
+
 // TAILQ-style intrusive list node.
 template <typename T>
 class ListNode;
diff --git 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),
