fs: move type checking for fs.read to js

PR-URL: https://github.com/nodejs/node/pull/17334
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

lib/fs.js

@@ -691,18 +691,38 @@ fs.openSync = function(path, flags, mode) {
 };
 
 fs.read = function(fd, buffer, offset, length, position, callback) {
+  if (!Number.isInteger(fd))
+    throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'fd', 'number');
+  if (fd < 0 || fd > 0xFFFFFFFF)
+    throw new errors.RangeError('ERR_OUT_OF_RANGE', 'fd');
+  if (!isUint8Array(buffer))
+    throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'buffer',
+                               ['Buffer', 'Uint8Array']);
+
+  offset |= 0;
+  length |= 0;
+
   if (length === 0) {
     return process.nextTick(function() {
       callback && callback(null, 0, buffer);
     });
   }
 
+  if (offset < 0 || offset >= buffer.length)
+    throw new errors.RangeError('ERR_OUT_OF_RANGE', 'offset');
+
+  if (length < 0 || offset + length > buffer.length)
+    throw new errors.RangeError('ERR_OUT_OF_RANGE', 'length');
+
+  if (!Number.isInteger(position))
+    position = -1;
+
   function wrapper(err, bytesRead) {
     // Retain a reference to buffer so that it can't be GC'ed too soon.
     callback && callback(err, bytesRead || 0, buffer);
   }
 
-  var req = new FSReqWrap();
+  const req = new FSReqWrap();
   req.oncomplete = wrapper;
 
   binding.read(fd, buffer, offset, length, position, req);
@@ -712,10 +732,30 @@ Object.defineProperty(fs.read, internalUtil.customPromisifyArgs,
                       { value: ['bytesRead', 'buffer'], enumerable: false });
 
 fs.readSync = function(fd, buffer, offset, length, position) {
+  if (!Number.isInteger(fd))
+    throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'fd', 'number');
+  if (fd < 0 || fd > 0xFFFFFFFF)
+    throw new errors.RangeError('ERR_OUT_OF_RANGE', 'fd');
+  if (!isUint8Array(buffer))
+    throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'buffer',
+                               ['Buffer', 'Uint8Array']);
+
+  offset |= 0;
+  length |= 0;
+
   if (length === 0) {
     return 0;
   }
 
+  if (offset < 0 || offset >= buffer.length)
+    throw new errors.RangeError('ERR_OUT_OF_RANGE', 'offset');
+
+  if (length < 0 || offset + length > buffer.length)
+    throw new errors.RangeError('ERR_OUT_OF_RANGE', 'length');
+
+  if (!Number.isInteger(position))
+    position = -1;
+
   return binding.read(fd, buffer, offset, length, position);
 };
 

src/node_file.cc

@@ -1178,12 +1178,8 @@ static void WriteString(const FunctionCallbackInfo<Value>& args) {
 static void Read(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
 
-  if (args.Length() < 2)
+  CHECK(args[0]->IsInt32());
-    return TYPE_ERROR("fd and buffer are required");
+  CHECK(Buffer::HasInstance(args[1]));
-  if (!args[0]->IsInt32())
-    return TYPE_ERROR("fd must be a file descriptor");
-  if (!Buffer::HasInstance(args[1]))
-    return TYPE_ERROR("Second argument needs to be a buffer");
 
   int fd = args[0]->Int32Value();
 
@@ -1199,13 +1195,10 @@ static void Read(const FunctionCallbackInfo<Value>& args) {
   size_t buffer_length = Buffer::Length(buffer_obj);
 
   size_t off = args[2]->Int32Value();
-  if (off >= buffer_length) {
+  CHECK_LT(off, buffer_length);
-    return env->ThrowError("Offset is out of bounds");
-  }
 
   len = args[3]->Int32Value();
-  if (!Buffer::IsWithinBounds(off, len, buffer_length))
+  CHECK(Buffer::IsWithinBounds(off, len, buffer_length));
-    return env->ThrowRangeError("Length extends beyond buffer");
 
   pos = GET_OFFSET(args[4]);
 

test/parallel/test-fs-read-type.js

@@ -1,6 +1,5 @@
 'use strict';
 const common = require('../common');
-const assert = require('assert');
 const fs = require('fs');
 const fixtures = require('../common/fixtures');
 
@@ -9,14 +8,20 @@ const fd = fs.openSync(filepath, 'r');
 const expected = 'xyz\n';
 
 // Error must be thrown with string
-assert.throws(() => {
+common.expectsError(
-  fs.read(fd,
+  () => fs.read(fd, expected.length, 0, 'utf-8', common.mustNotCall()),
-          expected.length,
+  {
-          0,
+    code: 'ERR_INVALID_ARG_TYPE',
-          'utf-8',
+    type: TypeError,
-          common.mustNotCall());
+    message: 'The "buffer" argument must be one of type Buffer or Uint8Array'
-}, /^TypeError: Second argument needs to be a buffer$/);
+  }
+);
 
-assert.throws(() => {
+common.expectsError(
-  fs.readSync(fd, expected.length, 0, 'utf-8');
+  () => fs.readSync(fd, expected.length, 0, 'utf-8'),
-}, /^TypeError: Second argument needs to be a buffer$/);
+  {
+    code: 'ERR_INVALID_ARG_TYPE',
+    type: TypeError,
+    message: 'The "buffer" argument must be one of type Buffer or Uint8Array'
+  }
+);