lib: add case of no argument to ERR_INVALID_ARG_VALUE in errors.js #21262

AdityaSrivast · 2018-06-11T17:15:42Z

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

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

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: nodejs#21193

joyeecheung · 2018-06-11T17:38:25Z

Hi, welcome and thank you for the contribution!

Unfortunately looking at the issue closely I don't think my suggestion in #21193 (comment) is valid. The proper fix should be adding a validation for the buffer before each call of validateOffsetLengthRead in the code base since the incorrect argument is the buffer, not the length that we retrieve out of the buffer.

Replacing every call of validateOffsetLengthRead() with something like

if (buffer.length === 0) {
  throw ERR_INVALID_ARG_VALUE('buffer', buffer, 'is empty and can\'t be written');
}
validateOffsetLengthRead(offset, length, buffer.length);

would be more appropriate.

Can you apply this fix instead?

joyeecheung · 2018-06-11T17:40:05Z

lib/internal/fs/utils.js

-  if (offset < 0 || offset >= bufferLength) {
+  if (bufferLength === 0) {
+    err = new ERR_INVALID_ARG_VALUE('buffer', 0,
+                                    "is empty and can't be written.");


By the way, in this code base we use single quotes. Please run make lint (non-Windows) or vcbuild.bat lint (Windows) to run the tests.

Please also move this statement in the already existing if statement and change it to an ERR_OUT_OF_RANGE error. That way the extra check is only triggered in the error case as in:

if (offset < 0 || offset >= bufferLength) { if (bufferLength === 0) { err = new ERR_OUT_OF_RANGE('bufferLength', '> 0', bufferLength); } else { err = new ...; } } else ...

joyeecheung · 2018-06-11T17:40:28Z

test/parallel/test-internal-errors.js

-    }
-  );
-}
+// {


In general, we do not leave the code commented in the code base.

joyeecheung · 2018-06-11T17:41:57Z

test/parallel/test-internal-errors.js

+//   );
+// }
+
+assert.expectsError(() => {


Can you write the tests in the corresponding test/parallel/test-fs-read*.js files? Or add a new test for the new error and name it with something like test/parallel/test-fs-read-empty-buffer.js.

BridgeAR · 2018-06-11T17:54:54Z

lib/internal/fs/utils.js

-  if (offset < 0 || offset >= bufferLength) {
+  if (bufferLength === 0) {
+    err = new ERR_INVALID_ARG_VALUE('buffer', 0,
+                                    "is empty and can't be written.");


Please also move this statement in the already existing if statement and change it to an ERR_OUT_OF_RANGE error. That way the extra check is only triggered in the error case as in:

if (offset < 0 || offset >= bufferLength) { if (bufferLength === 0) { err = new ERR_OUT_OF_RANGE('bufferLength', '> 0', bufferLength); } else { err = new ...; } } else ...

BridgeAR · 2018-06-11T17:55:52Z

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.';


I am not sure I understand this change.

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: nodejs#21193

BridgeAR · 2018-06-11T20:20:02Z

lib/internal/fs/utils.js

@@ -293,7 +293,13 @@ function validateOffsetLengthRead(offset, length, bufferLength) {
  let err;

  if (offset < 0 || offset >= bufferLength) {
-    err = new ERR_OUT_OF_RANGE('offset', `>= 0 && <= ${bufferLength}`, offset);
+    if (bufferLength === 0) {
+      throw ERR_INVALID_ARG_VALUE('buffer', 0,


Please change this to an ERR_OUT_OF_RANGE error that corresponds to the bufferLength. I do not know what buffer should stand for in this case and invalid argument value errors are normally used in different contexts. Here it should definitely be an out of range error as I also suggested in my code example in my earlier comment.

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: nodejs#21193

BridgeAR · 2018-06-11T21:45:25Z

The change itself looks good. we have to add a test though to get this landed. Have a look at the other tests and try to add one. Then run make -j4 (it will take a long time if you do that the first time) and then you can run python ./tools/test.py parallel/$filename so make sure your specific test works. If it does, run make -j4 test to make sure all tests pass including the linter.

joyeecheung · 2018-06-12T09:29:17Z

@BridgeAR Note that the user does not pass bufferLength, it’s computed by us so the error in this patch is still confusing for users who pass an empty buffer to read a file into.

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: nodejs#21193

AdityaSrivast · 2018-06-12T16:48:01Z

Updated @BridgeAR

AdityaSrivast · 2018-06-15T07:01:44Z

@BridgeAR @joyeecheung Is there still any discrepencies or changes to be made. If so, please suggest. I will be most happy to work them out. 🙂

joyeecheung · 2018-06-15T10:10:48Z

Have you considered my suggestion in #21262 (comment) ? bufferLength is not something that's passed by users, they only pass the buffer to us, so the error message is still going to be confusing.

BridgeAR · 2018-06-15T10:33:42Z

@joyeecheung AFAIK we normally only use the arguments of the currently executed function, no matter if it that argument is passed through directly or not. If we really want to say that the buffer itself is not OK (because the user passed that through as an argument), then an ERR_INVALID_ARG_VALUE would probably be best. The question for me is though if that is really less confusing.
Currently the validation function is only used by fs\.(promises\.)?read(Sync)? but when used anywhere else an invalid arg would likely be wrong. So for me this is somewhat of an edge case but after looking at your example again, I am also fine with that if you definitely prefer that. We could go ahead and rename the function though so it will always only be used by read.

@AdityaSrivast sorry for the inconvenience here. Some times it is not trivial to really get the errors right and they need a couple iterations. So thanks a lot for sticking to it! 👍

joyeecheung · 2018-06-15T12:11:51Z

@BridgeAR I prefer validating the Buffer itself since validateOffsetLengthRead is just an internal function. If abstraction like this prevent us from providing a good error message to user, we might as well just copy-paste instead of wrapping the validation into a function. Code reuse itself does not worth sacrificing user-friendliness IMO.

AdityaSrivast · 2018-06-15T18:34:17Z

@joyeecheung I have tried using buffer.length before validateOffsetLengthRead() and while linting I got an error that 'buffer' is not defined. Even inside the function we cannot use buffer.length and it gives the same error. But I do think that bufferLength === 0 case is just the perfect option as with empty buffer, this part of code was getting executed:
if (offset < 0 || offset >= bufferLength) {
err = new ERR_OUT_OF_RANGE('offset',
>= 0 && <= ${bufferLength}, offset);
}

Here, the message which we were getting was:
RangeError [ERR_OUT_OF_RANGE]: The value of "offset" is out of range. It must be >= 0 && <= 0. Received 0.
Please note that the second "0" corresponds to the value of bufferLength, which becomes 0, when no argument value is given to the buffer (i.e. Uint8array( ) ).

Regarding user friendliness, I feel that since the user is using readSync(fd, buffer, offset, length, position), there are arguments "offset" and "length", which give meaningful error messages for "offset" or "length" being out of the range. But "bufferLength" is not an argument in readSync(), which the user uses. So, it may result into slight confusion for users.
This way I feel that ERR_INVALID_ARG_VALUE may be a slightly better option as along with this error, it will throw a message that "The argument buffer is empty and cannot be written. Received no value", which should make it clear for the user about the error.
Note that there is only slight difference between both and we can go with either.

@BridgeAR Thanks for acknowledging. It's my pleasure contributing to node and nodejs. 😄

joyeecheung · 2018-06-16T03:06:39Z

@AdityaSrivast

I have tried using buffer.length before validateOffsetLengthRead() and while linting I got an error that 'buffer' is not defined.

That's weird, I can lint with this diff just fine.

diff --git a/lib/fs.js b/lib/fs.js
index f80bb6ac33..b5f8e4d3a2 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,14 @@ function read(fd, buffer, offset, length, position, callback) {
     });
   }
 
+  if (buffer.length === 0) {
+    throw ERR_INVALID_ARG_VALUE(
+      'buffer',
+      buffer,
+      'is empty and cannot be written'
+    );
+  }
+
   validateOffsetLengthRead(offset, length, buffer.length);
 
   if (!Number.isSafeInteger(position))
@@ -487,6 +496,14 @@ function readSync(fd, buffer, offset, length, position) {
     return 0;
   }
 
+  if (buffer.length === 0) {
+    throw 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 750d0d7579..2d1fa8818a 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,13 @@ async function read(handle, buffer, offset, length, position) {
   if (length === 0)
     return { bytesRead: length, buffer };
 
+  if (buffer.length === 0) {
+    throw ERR_INVALID_ARG_VALUE(
+      'buffer',
+      buffer,
+      'is empty and cannot be written'
+    );
+  }
   validateOffsetLengthRead(offset, length, buffer.length);
 
   if (!Number.isSafeInteger(position))

If you call something like throw ERR_INVALID_ARG_VALUE('buffer', buffer, 'is empty and cannot be written'); that should give you The argument "buffer" is empty and cannot be written. Received Buffer <>.

AdityaSrivast · 2018-06-16T06:20:05Z

@joyeecheung Thanks a lot for correcting. Actually, I was applying the fix in utils.js before validateOffsetLengthRead();, which was accordingly throwing the error. But this fix works. I will apply it.
I am also curious on how to find in which files read(), readSync() or any other function is used. Can you please tell me how to do so?

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: nodejs#21193

AdityaSrivast · 2018-06-16T12:30:22Z

@joyeecheung @BridgeAR Updated

AdityaSrivast · 2018-06-19T19:39:35Z

@joyeecheung Did I miss out something, please? 😕

joyeecheung · 2018-06-19T19:44:42Z

@AdityaSrivast Sorry, missed the notification.

I am also curious on how to find in which files read(), readSync() or any other function is used. Can you please tell me how to do so?

I use visual studio code so I just use the search tab there with ./lib as the location to search. If you use the command line, grep -nr 'validateOffsetLengthRead' ./lib should do the trick as well.

joyeecheung · 2018-06-19T19:45:56Z

test/parallel/test-fs-read-empty-buffer.js

+const fixtures = require('../common/fixtures');
+const assert = require('assert');
+const fs = require('fs');
+const filepath = fixtures.path('testcodes.txt');


This and the additional fixtures should be unnecessary, we can use any existing fixtures for this test, like fixtures.path('elipses.txt')

joyeecheung · 2018-06-19T19:49:12Z

test/parallel/test-fs-read-empty-buffer.js

+assert.throws(
+  () => fs.readSync(fd, buffer, 0, 10, 0),
+  {
+    code: 'ERR_INVALID_ARG_VALUE',


Can you validate the message? Something like

message: 'The argument \'buffer\' is empty and cannot be written. ' + 'Received <Buffer >'`

should work (or whatever the actual error message is)

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: nodejs#21193

joyeecheung · 2018-06-19T23:03:15Z

CI: https://ci.nodejs.org/job/node-test-pull-request/15527/

AdityaSrivast · 2018-06-25T18:24:22Z

@joyeecheung Is there still anything left out, please?

joyeecheung · 2018-06-25T19:43:08Z

New CI: https://ci.nodejs.org/job/node-test-pull-request/15611/

joyeecheung · 2018-06-25T19:45:11Z

I'd think this is semver-patch because personally I don't think changing the code of a validation error (ERR_OUT_OF_RANGE -> ERR_INVALID_ARG_VALUE) is worth a semver-major. @jasnell WDYT?

BridgeAR · 2018-06-25T20:18:09Z

@joyeecheung AFAIK the intention of the error code was to be able to change the error message without it being semver major but changes to the code would still be semver major. I personally believe we should start adding error groups as they would be far more helpful for most users than the actual error codes as quite a few errors belong all in one category. If we'd have those, I would argue it would not be semver major anymore because an ERR_OUT_OF_RANGE and ERR_INVALID_ARG_VALUE would likely belong in the same category.

jasnell

This LGTM but I do think this is a semver-major

AdityaSrivast · 2018-07-11T15:12:50Z

It's undoubtedly a great feeling contributing to nodejs and node for me. @joyeecheung when can my commit be merged, please? 😄
Also, I want to contribute more to the node. Can you please advise me how should I progress from now, please. 😃
It will surely be a great help

joyeecheung · 2018-07-11T17:19:38Z

New CI: https://ci.nodejs.org/job/node-test-pull-request/15808/

It's undoubtedly a great feeling contributing to nodejs and node for me. @joyeecheung when can my commit be merged, please? 😄

This PR should be ready to land if the CI is green. Sorry, there are no bots here taking care of landing PRs automatically and most of the people who have commit access here are flooded by notifications so if you want to push things forward please ping us by our GitHub handle - people usually respond faster if you do that.

Also, I want to contribute more to the node. Can you please advise me how should I progress from now, please. 😃
It will surely be a great help

I don't have any more good first issues off the top of my head, maybe watching https://github.com/nodejs/node/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22 would help?

AdityaSrivast · 2018-07-13T16:12:01Z

@joyeecheung The CI is green. Can you please merge this PR?

joyeecheung · 2018-07-15T11:46:28Z

Landed in 42bded8, thanks!

AdityaSrivast · 2018-07-15T12:31:49Z

@joyeecheung Thanks a lot, means a lot. ❤️

Fixes: nodejs#21193 PR-URL: nodejs#21262 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. fs Issues and PRs related to the fs subsystem / file system. labels Jun 11, 2018

joyeecheung reviewed Jun 11, 2018

View reviewed changes

BridgeAR reviewed Jun 11, 2018

View reviewed changes

joyeecheung reviewed Jun 19, 2018

View reviewed changes

joyeecheung approved these changes Jun 25, 2018

View reviewed changes

jasnell approved these changes Jun 29, 2018

View reviewed changes

joyeecheung added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jul 11, 2018

joyeecheung closed this Jul 15, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: add case of no argument to ERR_INVALID_ARG_VALUE in errors.js #21262

lib: add case of no argument to ERR_INVALID_ARG_VALUE in errors.js #21262

AdityaSrivast commented Jun 11, 2018 •

edited

Loading

joyeecheung commented Jun 11, 2018 •

edited

Loading

joyeecheung Jun 11, 2018

BridgeAR Jun 11, 2018

joyeecheung Jun 11, 2018

joyeecheung Jun 11, 2018

BridgeAR Jun 11, 2018

BridgeAR Jun 11, 2018

BridgeAR Jun 11, 2018

BridgeAR commented Jun 11, 2018

joyeecheung commented Jun 12, 2018

AdityaSrivast commented Jun 12, 2018

AdityaSrivast commented Jun 15, 2018

joyeecheung commented Jun 15, 2018

BridgeAR commented Jun 15, 2018

joyeecheung commented Jun 15, 2018 •

edited

Loading

AdityaSrivast commented Jun 15, 2018 •

edited

Loading

joyeecheung commented Jun 16, 2018

AdityaSrivast commented Jun 16, 2018

AdityaSrivast commented Jun 16, 2018

AdityaSrivast commented Jun 19, 2018

joyeecheung commented Jun 19, 2018

joyeecheung Jun 19, 2018

joyeecheung Jun 19, 2018

joyeecheung commented Jun 19, 2018

AdityaSrivast commented Jun 25, 2018

joyeecheung commented Jun 25, 2018

joyeecheung commented Jun 25, 2018

BridgeAR commented Jun 25, 2018

jasnell left a comment

AdityaSrivast commented Jul 11, 2018 •

edited

Loading

joyeecheung commented Jul 11, 2018

AdityaSrivast commented Jul 13, 2018

joyeecheung commented Jul 15, 2018

AdityaSrivast commented Jul 15, 2018

lib: add case of no argument to ERR_INVALID_ARG_VALUE in errors.js #21262

lib: add case of no argument to ERR_INVALID_ARG_VALUE in errors.js #21262

Conversation

AdityaSrivast commented Jun 11, 2018 • edited Loading

Checklist

joyeecheung commented Jun 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Jun 11, 2018

joyeecheung commented Jun 12, 2018

AdityaSrivast commented Jun 12, 2018

AdityaSrivast commented Jun 15, 2018

joyeecheung commented Jun 15, 2018

BridgeAR commented Jun 15, 2018

joyeecheung commented Jun 15, 2018 • edited Loading

AdityaSrivast commented Jun 15, 2018 • edited Loading

joyeecheung commented Jun 16, 2018

AdityaSrivast commented Jun 16, 2018

AdityaSrivast commented Jun 16, 2018

AdityaSrivast commented Jun 19, 2018

joyeecheung commented Jun 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyeecheung commented Jun 19, 2018

AdityaSrivast commented Jun 25, 2018

joyeecheung commented Jun 25, 2018

joyeecheung commented Jun 25, 2018

BridgeAR commented Jun 25, 2018

jasnell left a comment

Choose a reason for hiding this comment

AdityaSrivast commented Jul 11, 2018 • edited Loading

joyeecheung commented Jul 11, 2018

AdityaSrivast commented Jul 13, 2018

joyeecheung commented Jul 15, 2018

AdityaSrivast commented Jul 15, 2018

AdityaSrivast commented Jun 11, 2018 •

edited

Loading

joyeecheung commented Jun 11, 2018 •

edited

Loading

joyeecheung commented Jun 15, 2018 •

edited

Loading

AdityaSrivast commented Jun 15, 2018 •

edited

Loading

AdityaSrivast commented Jul 11, 2018 •

edited

Loading