Node: Confusing error message in fs.utils

[force-net]

https://github.com/nodejs/node/blob/f22a9cac36f731d5bdbf1b7c542b36fa4c13f4de/lib/internal/fs/utils.js#L543

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]

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

[force-net]

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]

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

[force-net]

I really do not know what is correct behavior. There are some special checks for empty buffer and length === 0 in fs.read
https://github.com/nodejs/node/blob/f22a9cac36f731d5bdbf1b7c542b36fa4c13f4de/lib/fs.js#L506

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]

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

https://github.com/nodejs/node/blob/f22a9cac36f731d5bdbf1b7c542b36fa4c13f4de/lib/fs.js#L506

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

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 :+1:

[daemon1024]

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

[force-net]

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

[daemon1024]

Great @force-net .

@addaleax can I start working in this?

[addaleax]

@daemon1024 Sury, why not :)

[daemon1024]

@addaleax can you please review my PR?