From 163869879e2e29a3210dd2ab40eaa19cceb7bf1e Mon Sep 17 00:00:00 2001 From: James M Snell Date: Sun, 26 Nov 2017 20:51:01 -0800 Subject: [PATCH] fs: move type checking for fs.read to js PR-URL: https://github.com/nodejs/node/pull/17334 Reviewed-By: Refael Ackermann Reviewed-By: Timothy Gu Reviewed-By: Jon Moss Reviewed-By: Anna Henningsen Reviewed-By: Joyee Cheung Reviewed-By: Matteo Collina --- lib/fs.js | 42 +++++++++++++++++++++++++++++- src/node_file.cc | 15 +++-------- test/parallel/test-fs-read-type.js | 27 +++++++++++-------- 3 files changed, 61 insertions(+), 23 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index b40d726404e..9554620a040 100644 --- a/lib/fs.js +++ b/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); }; diff --git a/src/node_file.cc b/src/node_file.cc index e8dd7051ca0..0bb5b727999 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -1178,12 +1178,8 @@ static void WriteString(const FunctionCallbackInfo& args) { static void Read(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); - if (args.Length() < 2) - return TYPE_ERROR("fd and buffer are required"); - 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"); + CHECK(args[0]->IsInt32()); + CHECK(Buffer::HasInstance(args[1])); int fd = args[0]->Int32Value(); @@ -1199,13 +1195,10 @@ static void Read(const FunctionCallbackInfo& args) { size_t buffer_length = Buffer::Length(buffer_obj); size_t off = args[2]->Int32Value(); - if (off >= buffer_length) { - return env->ThrowError("Offset is out of bounds"); - } + CHECK_LT(off, buffer_length); len = args[3]->Int32Value(); - if (!Buffer::IsWithinBounds(off, len, buffer_length)) - return env->ThrowRangeError("Length extends beyond buffer"); + CHECK(Buffer::IsWithinBounds(off, len, buffer_length)); pos = GET_OFFSET(args[4]); diff --git a/test/parallel/test-fs-read-type.js b/test/parallel/test-fs-read-type.js index 0014affa1f7..cbbfe4824c1 100644 --- a/test/parallel/test-fs-read-type.js +++ b/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(() => { - fs.read(fd, - expected.length, - 0, - 'utf-8', - common.mustNotCall()); -}, /^TypeError: Second argument needs to be a buffer$/); +common.expectsError( + () => fs.read(fd, expected.length, 0, 'utf-8', common.mustNotCall()), + { + code: 'ERR_INVALID_ARG_TYPE', + type: TypeError, + message: 'The "buffer" argument must be one of type Buffer or Uint8Array' + } +); -assert.throws(() => { - fs.readSync(fd, expected.length, 0, 'utf-8'); -}, /^TypeError: Second argument needs to be a buffer$/); +common.expectsError( + () => fs.readSync(fd, expected.length, 0, 'utf-8'), + { + code: 'ERR_INVALID_ARG_TYPE', + type: TypeError, + message: 'The "buffer" argument must be one of type Buffer or Uint8Array' + } +);