From 19374fd25bac95c80e1eb877668f6b16affc36a2 Mon Sep 17 00:00:00 2001
From: Ujjwal Sharma <usharma1998@gmail.com>
Date: Mon, 9 Apr 2018 23:02:08 +0530
Subject: [PATCH] fs: improve argument handling for ReadStream

Improve handling of erratic arguments in fs.ReadStream

Refs: https://github.com/nodejs/node/pull/19732
PR-URL: https://github.com/nodejs/node/pull/19898
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ron Korving <ron@ronkorving.nl>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
---
 doc/api/fs.md                                 |  5 ++
 lib/fs.js                                     | 55 +++++++++++-----
 test/parallel/test-fs-read-stream-inherit.js  |  4 +-
 .../test-fs-read-stream-throw-type-error.js   | 66 ++++++++++++++-----
 test/parallel/test-fs-read-stream.js          |  4 +-
 5 files changed, 96 insertions(+), 38 deletions(-)

diff --git a/doc/api/fs.md b/doc/api/fs.md
index 286ffb3a63fb3a..b0c1d5fa63d4d8 100644
--- a/doc/api/fs.md
+++ b/doc/api/fs.md
@@ -1299,6 +1299,11 @@ fs.copyFileSync('source.txt', 'destination.txt', COPYFILE_EXCL);
 <!-- YAML
 added: v0.1.31
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/19898
+    description: Impose new restrictions on `start` and `end`, throwing
+                 more appropriate errors in cases when we cannot reasonably
+                 handle the input values.
   - version: v7.6.0
     pr-url: https://github.com/nodejs/node/pull/10739
     description: The `path` parameter can be a WHATWG `URL` object using
diff --git a/lib/fs.js b/lib/fs.js
index 8a5ee87fbaa3f8..31e78e95ac35e6 100644
--- a/lib/fs.js
+++ b/lib/fs.js
@@ -2031,30 +2031,51 @@ function ReadStream(path, options) {
   this.closed = false;
 
   if (this.start !== undefined) {
-    if (typeof this.start !== 'number' || Number.isNaN(this.start)) {
-      throw new ERR_INVALID_ARG_TYPE('start', 'number', this.start);
-    }
-    if (this.end === undefined) {
-      this.end = Infinity;
-    } else if (typeof this.end !== 'number' || Number.isNaN(this.end)) {
-      throw new ERR_INVALID_ARG_TYPE('end', 'number', this.end);
+    if (!Number.isSafeInteger(this.start)) {
+      if (typeof this.start !== 'number')
+        throw new ERR_INVALID_ARG_TYPE('start', 'number', this.start);
+      if (!Number.isInteger(this.start))
+        throw new ERR_OUT_OF_RANGE('start', 'an integer', this.start);
+      throw new ERR_OUT_OF_RANGE(
+        'start',
+        '>= 0 and <= 2 ** 53 - 1',
+        this.start
+      );
     }
-
-    if (this.start > this.end) {
-      const errVal = `{start: ${this.start}, end: ${this.end}}`;
-      throw new ERR_OUT_OF_RANGE('start', '<= "end"', errVal);
+    if (this.start < 0) {
+      throw new ERR_OUT_OF_RANGE(
+        'start',
+        '>= 0 and <= 2 ** 53 - 1',
+        this.start
+      );
     }
 
     this.pos = this.start;
   }
 
-  // Backwards compatibility: Make sure `end` is a number regardless of `start`.
-  // TODO(addaleax): Make the above typecheck not depend on `start` instead.
-  // (That is a semver-major change).
-  if (typeof this.end !== 'number')
+  if (this.end === undefined) {
     this.end = Infinity;
-  else if (Number.isNaN(this.end))
-    throw new ERR_INVALID_ARG_TYPE('end', 'number', this.end);
+  } else if (this.end !== Infinity) {
+    if (!Number.isSafeInteger(this.end)) {
+      if (typeof this.end !== 'number')
+        throw new ERR_INVALID_ARG_TYPE('end', 'number', this.end);
+      if (!Number.isInteger(this.end))
+        throw new ERR_OUT_OF_RANGE('end', 'an integer', this.end);
+      throw new ERR_OUT_OF_RANGE('end', '>= 0 and <= 2 ** 53 - 1', this.end);
+    }
+
+    if (this.end < 0) {
+      throw new ERR_OUT_OF_RANGE('end', '>= 0 and <= 2 ** 53 - 1', this.end);
+    }
+
+    if (this.start !== undefined && this.start > this.end) {
+      throw new ERR_OUT_OF_RANGE(
+        'start',
+        `<= "end" (here: ${this.end})`,
+        this.start
+      );
+    }
+  }
 
   if (typeof this.fd !== 'number')
     this.open();
diff --git a/test/parallel/test-fs-read-stream-inherit.js b/test/parallel/test-fs-read-stream-inherit.js
index f43f81ddee395d..a081302cc5a0f1 100644
--- a/test/parallel/test-fs-read-stream-inherit.js
+++ b/test/parallel/test-fs-read-stream-inherit.js
@@ -110,8 +110,8 @@ const rangeFile = fixtures.path('x.txt');
 
 {
   const message =
-    'The value of "start" is out of range. It must be <= "end". ' +
-    'Received {start: 10, end: 2}';
+    'The value of "start" is out of range. It must be <= "end" (here: 2).' +
+    ' Received 10';
 
   common.expectsError(
     () => {
diff --git a/test/parallel/test-fs-read-stream-throw-type-error.js b/test/parallel/test-fs-read-stream-throw-type-error.js
index 5cba76fa394636..9886b3c9a46d97 100644
--- a/test/parallel/test-fs-read-stream-throw-type-error.js
+++ b/test/parallel/test-fs-read-stream-throw-type-error.js
@@ -13,23 +13,55 @@ fs.createReadStream(example, null);
 fs.createReadStream(example, 'utf8');
 fs.createReadStream(example, { encoding: 'utf8' });
 
-const createReadStreamErr = (path, opt) => {
-  common.expectsError(
-    () => {
-      fs.createReadStream(path, opt);
-    },
-    {
-      code: 'ERR_INVALID_ARG_TYPE',
-      type: TypeError
-    });
+const createReadStreamErr = (path, opt, error) => {
+  common.expectsError(() => {
+    fs.createReadStream(path, opt);
+  }, error);
 };
 
-createReadStreamErr(example, 123);
-createReadStreamErr(example, 0);
-createReadStreamErr(example, true);
-createReadStreamErr(example, false);
+const typeError = {
+  code: 'ERR_INVALID_ARG_TYPE',
+  type: TypeError
+};
+
+const rangeError = {
+  code: 'ERR_OUT_OF_RANGE',
+  type: RangeError
+};
+
+[123, 0, true, false].forEach((opts) =>
+  createReadStreamErr(example, opts, typeError)
+);
+
+// Case 0: Should not throw if either start or end is undefined
+[{}, { start: 0 }, { end: Infinity }].forEach((opts) =>
+  fs.createReadStream(example, opts)
+);
+
+// Case 1: Should throw TypeError if either start or end is not of type 'number'
+[
+  { start: 'invalid' },
+  { end: 'invalid' },
+  { start: 'invalid', end: 'invalid' }
+].forEach((opts) => createReadStreamErr(example, opts, typeError));
+
+// Case 2: Should throw RangeError if either start or end is NaN
+[{ start: NaN }, { end: NaN }, { start: NaN, end: NaN }].forEach((opts) =>
+  createReadStreamErr(example, opts, rangeError)
+);
+
+// Case 3: Should throw RangeError if either start or end is negative
+[{ start: -1 }, { end: -1 }, { start: -1, end: -1 }].forEach((opts) =>
+  createReadStreamErr(example, opts, rangeError)
+);
+
+// Case 4: Should throw RangeError if either start or end is fractional
+[{ start: 0.1 }, { end: 0.1 }, { start: 0.1, end: 0.1 }].forEach((opts) =>
+  createReadStreamErr(example, opts, rangeError)
+);
+
+// Case 5: Should not throw if both start and end are whole numbers
+fs.createReadStream(example, { start: 1, end: 5 });
 
-// createReadSteam _should_ throw on NaN
-createReadStreamErr(example, { start: NaN });
-createReadStreamErr(example, { end: NaN });
-createReadStreamErr(example, { start: NaN, end: NaN });
+// Case 6: Should throw RangeError if start is greater than end
+createReadStreamErr(example, { start: 5, end: 1 }, rangeError);
diff --git a/test/parallel/test-fs-read-stream.js b/test/parallel/test-fs-read-stream.js
index 870edf2820e87b..793b474c9b8292 100644
--- a/test/parallel/test-fs-read-stream.js
+++ b/test/parallel/test-fs-read-stream.js
@@ -148,8 +148,8 @@ common.expectsError(
   },
   {
     code: 'ERR_OUT_OF_RANGE',
-    message: 'The value of "start" is out of range. It must be <= "end". ' +
-             'Received {start: 10, end: 2}',
+    message: 'The value of "start" is out of range. It must be <= "end"' +
+             ' (here: 2). Received 10',
     type: RangeError
   });