Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Confusing error message in fs.utils #32871

Closed
force-net opened this issue Apr 15, 2020 · 10 comments
Closed

Confusing error message in fs.utils #32871

force-net opened this issue Apr 15, 2020 · 10 comments
Labels
confirmed-bug Issues with confirmed bugs. fs Issues and PRs related to the fs subsystem / file system.

Comments

@force-net
Copy link

throw new ERR_OUT_OF_RANGE('offset',

This check can produce confusing error message when offset is equal to bufferLength. Which is error, but message shows, that it is correct (<=)

if (offset < 0 || offset >= bufferLength) {
      throw new ERR_OUT_OF_RANGE('offset',
                                 `>= 0 && <= ${bufferLength}`, offset);
    }
@addaleax
Copy link
Member

I think the check should be offset > bufferLength, yes. Do you want to open a PR?

@addaleax addaleax added confirmed-bug Issues with confirmed bugs. fs Issues and PRs related to the fs subsystem / file system. labels Apr 15, 2020
@force-net
Copy link
Author

I think, that check is correct. Problem in text of error message. I do not want to open PR myself, because I do not know correct solution. It can be:

  • Change error message to >= 0 && < ${bufferLength}
  • Change error message to >= 0 && <= ${bufferLength-1}
  • Remove this method completely and replace it with validateInteger(offset, "offset", 0, bufferLength - 1)

@addaleax
Copy link
Member

@force-net The check is incorrect, passing offset === bufferLength and length === 0 should be a valid use case.

@force-net
Copy link
Author

I really do not know what is correct behavior. There are some special checks for empty buffer and length === 0 in fs.read

node/lib/fs.js

Line 506 in f22a9ca

if (length === 0) {

Also, in this situation there are no problem to use any offset with zero length. E.g. bufferLength == 10 length == 0 and offset == 100, because we will not fill any values in buffer.

But for arrays correct range from zero to length - 1, so it logically incorrect to use array index equal to length.

May be this condition should be removed completely and full check should be rewrote to:

  • offset should be >= 0
  • length should be >= 0
  • offset + length should be <= bufferLength
const validateOffsetLengthRead = hideStackFrames(
  (offset, length, bufferLength) => {
    if (offset < 0) {
      throw new ERR_OUT_OF_RANGE('offset', `>= 0`, offset);
    }
    if (length < 0) {
      throw new ERR_OUT_OF_RANGE('length', `>= 0`, length);
    }
    if (offset + length > bufferLength) {
      throw new ERR_OUT_OF_RANGE('offset + length',` <= ${bufferLength}`, length);
    }
  }
);

@addaleax
Copy link
Member

I really do not know what is correct behavior. There are some special checks for empty buffer and length === 0 in fs.read

node/lib/fs.js

Line 506 in f22a9ca

if (length === 0) {

Yeah, thanks for pointing out that the length === 0 case would not be reached in the validator 👍

Also, in this situation there are no problem to use any offset with zero length. E.g. bufferLength == 10 length == 0 and offset == 100, because we will not fill any values in buffer.

I don’t think that’s necessarily a bad thing – as you say, length == 0 will not result in out-of-bounds accesses to the buffer.

But for arrays correct range from zero to length - 1, so it logically incorrect to use array index equal to length.

That’s the range for valid indices that can be accessed, not the range for valid offsets, though. Generally, you can take a zero-length slice of an array at its end without that being an error, that’s the most consistent behavior.

May be this condition should be removed completely and full check should be rewrote to:

* offset should be >= 0

* length should be >= 0

* offset + length should be <= bufferLength
const validateOffsetLengthRead = hideStackFrames(
  (offset, length, bufferLength) => {
    if (offset < 0) {
      throw new ERR_OUT_OF_RANGE('offset', `>= 0`, offset);
    }
    if (length < 0) {
      throw new ERR_OUT_OF_RANGE('length', `>= 0`, length);
    }
    if (offset + length > bufferLength) {
      throw new ERR_OUT_OF_RANGE('offset + length',` <= ${bufferLength}`, length);
    }
  }
);

That sounds good to me, yes 👍

@daemon1024
Copy link
Contributor

daemon1024 commented Apr 16, 2020

If @force-net is not interested in opening a PR, should I open a PR for the discussed changes?

@force-net
Copy link
Author

@daemon1024 👍 I'm not sure that I can do it in immediate time. It would be fine, if you can do this.

@daemon1024
Copy link
Contributor

Great @force-net .

@addaleax can I start working in this?

@addaleax
Copy link
Member

@daemon1024 Sury, why not :)

@daemon1024
Copy link
Contributor

@addaleax can you please review my PR?

BethGriggs pushed a commit that referenced this issue Apr 27, 2020
PR-URL: #32896
Fixes: #32871
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Andrey Pechkurov <[email protected]>
BridgeAR pushed a commit that referenced this issue Apr 28, 2020
PR-URL: #32896
Fixes: #32871
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Andrey Pechkurov <[email protected]>
targos pushed a commit that referenced this issue Apr 30, 2020
PR-URL: #32896
Fixes: #32871
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Andrey Pechkurov <[email protected]>
targos pushed a commit that referenced this issue May 13, 2020
PR-URL: #32896
Fixes: #32871
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Andrey Pechkurov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants