From f2f5c0c2c4020a55f67a03a70467dd5c0a0f7a5b Mon Sep 17 00:00:00 2001 From: AdityaSrivast Date: Mon, 11 Jun 2018 19:14:52 +0530 Subject: [PATCH 1/6] lib: add case of no argument to ERR_INVALID_ARG_VALUE in errors.js ERR_INVALID_ARG_VALUE is an error for wrong arguments given to the function. But calling a function without argument should also generate a sensible error message. For no argument case, parameter 'value' should be passed with zero value and the error message is generated. In readSync(fd, buffer, offset, length, position), triggers validateOffsetLengthRead() in lib/internal/fs/utils.js for validation. When buffer is empty, a weird message is generated "The value of offset is out of range.It must be >= 0 && <= 0. Received 0". There should be a special case when buffer is empty or bufferLength is zero, which should trigger ERR_INVALID_ARG_VALUE error. Fixes: https://github.com/nodejs/node/issues/21193 --- lib/internal/errors.js | 2 ++ lib/internal/fs/utils.js | 6 ++++-- test/parallel/test-internal-errors.js | 28 ++++++++++++++++++--------- 3 files changed, 25 insertions(+), 11 deletions(-) diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 54201d0d1e7f4c..718009cb2ade22 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -637,6 +637,8 @@ E('ERR_INVALID_ARG_VALUE', (name, value, reason = 'is invalid') => { let inspected = util.inspect(value); if (inspected.length > 128) { inspected = `${inspected.slice(0, 128)}...`; + } else if (inspected === '0') { + inspected = 'No value.'; } return `The argument '${name}' ${reason}. Received ${inspected}`; }, TypeError, RangeError); diff --git a/lib/internal/fs/utils.js b/lib/internal/fs/utils.js index 7aa19605e92a6a..7598c530161c74 100644 --- a/lib/internal/fs/utils.js +++ b/lib/internal/fs/utils.js @@ -291,8 +291,10 @@ function validateBuffer(buffer) { function validateOffsetLengthRead(offset, length, bufferLength) { let err; - - if (offset < 0 || offset >= bufferLength) { + if (bufferLength === 0) { + err = new ERR_INVALID_ARG_VALUE('buffer', 0, + "is empty and can't be written."); + } else if (offset < 0 || offset >= bufferLength) { err = new ERR_OUT_OF_RANGE('offset', `>= 0 && <= ${bufferLength}`, offset); } else if (length < 0 || offset + length > bufferLength) { err = new ERR_OUT_OF_RANGE('length', diff --git a/test/parallel/test-internal-errors.js b/test/parallel/test-internal-errors.js index b5e08911b8219e..d8849b3d2165c5 100644 --- a/test/parallel/test-internal-errors.js +++ b/test/parallel/test-internal-errors.js @@ -41,15 +41,25 @@ errors.E('TEST_ERROR_2', (a, b) => `${a} ${b}`, Error); assert.strictEqual(err.code, 'TEST_ERROR_2'); } -{ - assert.throws( - () => new errors.codes.TEST_ERROR_1(), - { - message: 'Code: TEST_ERROR_1; The provided arguments ' + - 'length (0) does not match the required ones (1).' - } - ); -} +// { +// assert.throws(() => new errors.codes.TEST_ERROR_1(){ +// message: 'Code: TEST_ERROR_1; The provided arguments ' + +// 'length (0) does not match the required ones (1).' +// } +// ); +// } + +assert.expectsError(() => { + const fs = require('fs'); + const file = '/home/aditya/node/test/nodetest/testcodes'; + const keyfile = fs.openSync(file, 'r'); + const string = new Uint8Array(); + const num = fs.readSync(keyfile, string, 0, 10, 0); + console.log(num); +}, { code: 'ERR_OUT_OF_RANGE', + type: RangeError, + message: 'Just Testing' +}); // Tests for common.expectsError common.expectsError(() => { From e05f49c58bb72f66fb5d2e63adea2210975b6335 Mon Sep 17 00:00:00 2001 From: AdityaSrivast Date: Tue, 12 Jun 2018 01:20:41 +0530 Subject: [PATCH 2/6] lib: add case of no argument to ERR_INVALID_ARG_VALUE in errors.js ERR_INVALID_ARG_VALUE is an error for wrong arguments given to the function. But calling a function without argument should also generate a sensible error message. For no argument case, parameter 'value' should be passed with zero value and the error message is generated. In readSync(fd, buffer, offset, length, position), triggers validateOffsetLengthRead() in lib/internal/fs/utils.js for validation. When buffer is empty, a weird message is generated "The value of offset is out of range.It must be >= 0 && <= 0. Received 0". There should be a special case when buffer is empty or bufferLength is zero, which should trigger ERR_INVALID_ARG_VALUE error. Fixes: #21193 --- lib/internal/errors.js | 2 +- lib/internal/fs/utils.js | 14 +++++++++----- test/parallel/test-internal-errors.js | 28 +++++++++------------------ 3 files changed, 19 insertions(+), 25 deletions(-) diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 718009cb2ade22..7656b6a1fa7181 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -638,7 +638,7 @@ E('ERR_INVALID_ARG_VALUE', (name, value, reason = 'is invalid') => { if (inspected.length > 128) { inspected = `${inspected.slice(0, 128)}...`; } else if (inspected === '0') { - inspected = 'No value.'; + inspected = 'no value.'; } return `The argument '${name}' ${reason}. Received ${inspected}`; }, TypeError, RangeError); diff --git a/lib/internal/fs/utils.js b/lib/internal/fs/utils.js index 7598c530161c74..3486b96735e974 100644 --- a/lib/internal/fs/utils.js +++ b/lib/internal/fs/utils.js @@ -291,11 +291,15 @@ function validateBuffer(buffer) { function validateOffsetLengthRead(offset, length, bufferLength) { let err; - if (bufferLength === 0) { - err = new ERR_INVALID_ARG_VALUE('buffer', 0, - "is empty and can't be written."); - } else if (offset < 0 || offset >= bufferLength) { - err = new ERR_OUT_OF_RANGE('offset', `>= 0 && <= ${bufferLength}`, offset); + + if (offset < 0 || offset >= bufferLength) { + if (bufferLength === 0) { + throw ERR_INVALID_ARG_VALUE('buffer', 0, + 'is empty and can\'t be written'); + } else { + err = new ERR_OUT_OF_RANGE('offset', + `>= 0 && <= ${bufferLength}`, offset); + } } else if (length < 0 || offset + length > bufferLength) { err = new ERR_OUT_OF_RANGE('length', `>= 0 && <= ${bufferLength - offset}`, length); diff --git a/test/parallel/test-internal-errors.js b/test/parallel/test-internal-errors.js index d8849b3d2165c5..b5e08911b8219e 100644 --- a/test/parallel/test-internal-errors.js +++ b/test/parallel/test-internal-errors.js @@ -41,25 +41,15 @@ errors.E('TEST_ERROR_2', (a, b) => `${a} ${b}`, Error); assert.strictEqual(err.code, 'TEST_ERROR_2'); } -// { -// assert.throws(() => new errors.codes.TEST_ERROR_1(){ -// message: 'Code: TEST_ERROR_1; The provided arguments ' + -// 'length (0) does not match the required ones (1).' -// } -// ); -// } - -assert.expectsError(() => { - const fs = require('fs'); - const file = '/home/aditya/node/test/nodetest/testcodes'; - const keyfile = fs.openSync(file, 'r'); - const string = new Uint8Array(); - const num = fs.readSync(keyfile, string, 0, 10, 0); - console.log(num); -}, { code: 'ERR_OUT_OF_RANGE', - type: RangeError, - message: 'Just Testing' -}); +{ + assert.throws( + () => new errors.codes.TEST_ERROR_1(), + { + message: 'Code: TEST_ERROR_1; The provided arguments ' + + 'length (0) does not match the required ones (1).' + } + ); +} // Tests for common.expectsError common.expectsError(() => { From 7af6bb38b7a3cc3edff60af5189b17d3c58c621f Mon Sep 17 00:00:00 2001 From: AdityaSrivast Date: Tue, 12 Jun 2018 02:39:10 +0530 Subject: [PATCH 3/6] lib: add case of bufferLength to validateOffsetLengthRead() in utils.js In validateOffsetLengthRead(), an error message is generated if offset or length are out of range. There should also be an error message if buffer is empty, in which case it cannot write data in it. Generally this validateOffsetLengthRead() is triggered with fs.readSync(fd, buffer, offset, length, position), where if buffer is empty, the case for zero bufferLength is triggered and the corresponding message is thrown. Fixes: https://github.com/nodejs/node/issues/21193 --- lib/internal/errors.js | 2 -- lib/internal/fs/utils.js | 3 +-- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 7656b6a1fa7181..54201d0d1e7f4c 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -637,8 +637,6 @@ E('ERR_INVALID_ARG_VALUE', (name, value, reason = 'is invalid') => { let inspected = util.inspect(value); if (inspected.length > 128) { inspected = `${inspected.slice(0, 128)}...`; - } else if (inspected === '0') { - inspected = 'no value.'; } return `The argument '${name}' ${reason}. Received ${inspected}`; }, TypeError, RangeError); diff --git a/lib/internal/fs/utils.js b/lib/internal/fs/utils.js index 3486b96735e974..5b99435dc586c2 100644 --- a/lib/internal/fs/utils.js +++ b/lib/internal/fs/utils.js @@ -294,8 +294,7 @@ function validateOffsetLengthRead(offset, length, bufferLength) { if (offset < 0 || offset >= bufferLength) { if (bufferLength === 0) { - throw ERR_INVALID_ARG_VALUE('buffer', 0, - 'is empty and can\'t be written'); + err = new ERR_OUT_OF_RANGE('bufferLength', '> 0', bufferLength); } else { err = new ERR_OUT_OF_RANGE('offset', `>= 0 && <= ${bufferLength}`, offset); From 7c1d3654883b19ecd6ad8da5ebc011874a706d7d Mon Sep 17 00:00:00 2001 From: AdityaSrivast Date: Tue, 12 Jun 2018 17:26:04 +0530 Subject: [PATCH 4/6] lib: add case of bufferLength to validateOffsetLengthRead() in utils.js In validateOffsetLengthRead(), an error message is generated if offset or length are out of range. But there should also be an error message if buffer is empty, when no data can be written on it. Generally, validateOffsetLengthRead() is triggered with fs.readSync(fd, buffer, offset, length, position), where if 'buffer' is empty, the case for zero bufferLength should be triggered and the error message for empty buffer should be thrown. Fixes: https://github.com/nodejs/node/issues/21193 --- test/fixtures/testcodes.txt | 12 ++++++++++++ test/parallel/test-fs-read-empty-buffer.js | 18 ++++++++++++++++++ 2 files changed, 30 insertions(+) create mode 100644 test/fixtures/testcodes.txt create mode 100644 test/parallel/test-fs-read-empty-buffer.js diff --git a/test/fixtures/testcodes.txt b/test/fixtures/testcodes.txt new file mode 100644 index 00000000000000..ee860ba0d59f61 --- /dev/null +++ b/test/fixtures/testcodes.txt @@ -0,0 +1,12 @@ +QXFPEEPNAT +ZIEEVVEZCU +CUPUMUEUPR +ISMNOTJCYG +TBKEWVBWRH +GAXHUDAUAZ +QFYWDRFDVA +WJOPKBXPMK +RZAIQNZQZK +OIRPQKIQDU +OONZZNMZNH +TPBPBPKHPQ \ No newline at end of file diff --git a/test/parallel/test-fs-read-empty-buffer.js b/test/parallel/test-fs-read-empty-buffer.js new file mode 100644 index 00000000000000..b2716d6a07215e --- /dev/null +++ b/test/parallel/test-fs-read-empty-buffer.js @@ -0,0 +1,18 @@ +'use strict'; +require('../common'); +const fixtures = require('../common/fixtures'); +const assert = require('assert'); +const fs = require('fs'); +const filepath = fixtures.path('testcodes.txt'); +const fd = fs.openSync(filepath, 'r'); + +const buffer = new Uint8Array(); + +assert.throws( + () => fs.readSync(fd, buffer, 0, 10, 0), + { + code: 'ERR_OUT_OF_RANGE', + message: 'The value of "bufferLength" is out of range. ' + + 'It must be > 0. Received 0' + } +); From 80437556a576e087c0da42d2276422dc26654cfb Mon Sep 17 00:00:00 2001 From: AdityaSrivast Date: Sat, 16 Jun 2018 15:07:51 +0530 Subject: [PATCH 5/6] lib: add empty buffer case to read and readSync function in fs While using read() or readSync() function, it should be verified that the arguments of the function are in proper range and hold legitimate values, for a successful read process. For this validateOffsetLengthRead() function is called, which gives an error message for offset and length passed by the user, if not in order. But there should also be an error when an empty buffer is passed as argument, when it cannot be written over. The empty buffer case should be checked before calling validateOffsetLengthRead() and ERR_INVALID_ARG_VALUE should be thrown corresponding to the empty buffer argument in read and readSync function. Fixes: https://github.com/nodejs/node/issues/21193 --- lib/fs.js | 11 +++++++++++ lib/internal/fs/promises.js | 6 ++++++ lib/internal/fs/utils.js | 8 ++------ test/parallel/test-fs-read-empty-buffer.js | 4 +--- 4 files changed, 20 insertions(+), 9 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index f80bb6ac33c649..f79dd0d3cd7888 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -47,6 +47,7 @@ const { Buffer, kMaxLength } = require('buffer'); const errors = require('internal/errors'); const { ERR_FS_FILE_TOO_LARGE, + ERR_INVALID_ARG_VALUE, ERR_INVALID_ARG_TYPE, ERR_INVALID_CALLBACK } = errors.codes; @@ -457,6 +458,11 @@ function read(fd, buffer, offset, length, position, callback) { }); } + if (buffer.length === 0) { + throw new ERR_INVALID_ARG_VALUE('buffer', buffer, + 'is empty and cannot be written'); + } + validateOffsetLengthRead(offset, length, buffer.length); if (!Number.isSafeInteger(position)) @@ -487,6 +493,11 @@ function readSync(fd, buffer, offset, length, position) { return 0; } + if (buffer.length === 0) { + throw new ERR_INVALID_ARG_VALUE('buffer', buffer, + 'is empty and cannot be written'); + } + validateOffsetLengthRead(offset, length, buffer.length); if (!Number.isSafeInteger(position)) diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js index 750d0d7579706f..57d36173b72b06 100644 --- a/lib/internal/fs/promises.js +++ b/lib/internal/fs/promises.js @@ -12,6 +12,7 @@ const { Buffer, kMaxLength } = require('buffer'); const { ERR_FS_FILE_TOO_LARGE, ERR_INVALID_ARG_TYPE, + ERR_INVALID_ARG_VALUE, ERR_METHOD_NOT_IMPLEMENTED } = require('internal/errors').codes; const { getPathFromURL } = require('internal/url'); @@ -207,6 +208,11 @@ async function read(handle, buffer, offset, length, position) { if (length === 0) return { bytesRead: length, buffer }; + if (buffer.length === 0) { + throw new ERR_INVALID_ARG_VALUE('buffer', buffer, + 'is empty and cannot be written'); + } + validateOffsetLengthRead(offset, length, buffer.length); if (!Number.isSafeInteger(position)) diff --git a/lib/internal/fs/utils.js b/lib/internal/fs/utils.js index 5b99435dc586c2..a2f1015c62b8ff 100644 --- a/lib/internal/fs/utils.js +++ b/lib/internal/fs/utils.js @@ -293,12 +293,8 @@ function validateOffsetLengthRead(offset, length, bufferLength) { let err; if (offset < 0 || offset >= bufferLength) { - if (bufferLength === 0) { - err = new ERR_OUT_OF_RANGE('bufferLength', '> 0', bufferLength); - } else { - err = new ERR_OUT_OF_RANGE('offset', - `>= 0 && <= ${bufferLength}`, offset); - } + err = new ERR_OUT_OF_RANGE('offset', + `>= 0 && <= ${bufferLength}`, offset); } else if (length < 0 || offset + length > bufferLength) { err = new ERR_OUT_OF_RANGE('length', `>= 0 && <= ${bufferLength - offset}`, length); diff --git a/test/parallel/test-fs-read-empty-buffer.js b/test/parallel/test-fs-read-empty-buffer.js index b2716d6a07215e..9d100b0c40618a 100644 --- a/test/parallel/test-fs-read-empty-buffer.js +++ b/test/parallel/test-fs-read-empty-buffer.js @@ -11,8 +11,6 @@ const buffer = new Uint8Array(); assert.throws( () => fs.readSync(fd, buffer, 0, 10, 0), { - code: 'ERR_OUT_OF_RANGE', - message: 'The value of "bufferLength" is out of range. ' + - 'It must be > 0. Received 0' + code: 'ERR_INVALID_ARG_VALUE', } ); From a8c61fd8d89fa70f95d05b61e69c19d6e89cd49e Mon Sep 17 00:00:00 2001 From: AdityaSrivast Date: Wed, 20 Jun 2018 02:59:19 +0530 Subject: [PATCH 6/6] lib: add empty buffer case to read and readSync function in fs While using read() or readSync() function, it should be verified that the arguments of the function are in proper range and hold legitimate values, for a successful read process. For this validateOffsetLengthRead() function is called, which gives an error message for offset and length passed by the user, if not in order. But there should also be an error when an empty buffer is passed as argument, when it cannot be written over. The empty buffer case should be checked before calling validateOffsetLengthRead() and ERR_INVALID_ARG_VALUE should be thrown corresponding to the empty buffer argument in read and readSync function. Fixes: https://github.com/nodejs/node/issues/21193 --- test/fixtures/testcodes.txt | 12 ------------ test/parallel/test-fs-read-empty-buffer.js | 4 +++- 2 files changed, 3 insertions(+), 13 deletions(-) delete mode 100644 test/fixtures/testcodes.txt diff --git a/test/fixtures/testcodes.txt b/test/fixtures/testcodes.txt deleted file mode 100644 index ee860ba0d59f61..00000000000000 --- a/test/fixtures/testcodes.txt +++ /dev/null @@ -1,12 +0,0 @@ -QXFPEEPNAT -ZIEEVVEZCU -CUPUMUEUPR -ISMNOTJCYG -TBKEWVBWRH -GAXHUDAUAZ -QFYWDRFDVA -WJOPKBXPMK -RZAIQNZQZK -OIRPQKIQDU -OONZZNMZNH -TPBPBPKHPQ \ No newline at end of file diff --git a/test/parallel/test-fs-read-empty-buffer.js b/test/parallel/test-fs-read-empty-buffer.js index 9d100b0c40618a..281d2675e72730 100644 --- a/test/parallel/test-fs-read-empty-buffer.js +++ b/test/parallel/test-fs-read-empty-buffer.js @@ -3,7 +3,7 @@ require('../common'); const fixtures = require('../common/fixtures'); const assert = require('assert'); const fs = require('fs'); -const filepath = fixtures.path('testcodes.txt'); +const filepath = fixtures.path('x.txt'); const fd = fs.openSync(filepath, 'r'); const buffer = new Uint8Array(); @@ -12,5 +12,7 @@ assert.throws( () => fs.readSync(fd, buffer, 0, 10, 0), { code: 'ERR_INVALID_ARG_VALUE', + message: 'The argument \'buffer\' is empty and cannot be written. ' + + 'Received Uint8Array [ ]' } );