buffer: fix range checking for slowToString by matthewloring · Pull Request #4019 · nodejs/node · GitHub

If start is not a valid number in the range, then the default value
zero will be used. Same way, if end is not a valid number in the
accepted range, then, by default, the length of the buffer is assumed.

Ref: #2668

PR-URL: #2919

@trevnorris @thefourtheye I believe I've actioned all of the comments in #2919 except changing int32_t to uint32_t in src/node_internals.h as this change breaks the range errors that should be thrown on negative index. What was the rational in modifying this to Uint32Value in the original PR?

I forget. What was the rational behind checking for NaN explicitly while allowing other values that would coerce to NaN get through?

On the other pull request you indicated that you wanted the same behavior as typed arrays

let ab = new ArrayBuffer(8);
new Uint8Array(ab, 0, false).length === 0;
new Uint8Array(ab, 0, null).length === 0;
new Uint8Array(ab, 0, NaN).length === 0;
new Uint8Array(ab, 0, undefined).length === 8;

Assigning the default is normally captured by the check below end > this.length || end >>> 0 !== parseInt(+end) but end >>> 0 !== parseInt(+end) is false for end = null and end = false and is true for end = undefined and end = NaN. To ensure that the default value is not used when end = NaN, I special cased it above. If there is a more elegant check that could be used below that would be nicer.

You're special casing for NaN. i.e. Number.isNaN(false) === false. Are you thinking isNaN()?

The hope was to check only for NaN and let false, null, etc be handled below as they were already handled correctly.

Here's the change:

function slowToString(encoding, start, end) {
  var loweredCase = false;

  // No need to verify that "this.length <= MAX_UINT32" since it's a read-only
  // property of a typed array.

  // This behaves neither like String nor Uint8Array in that we set start/end
  // to their upper/lower bounds if the value passed is out of range.
  if (start === undefined || start < 0)
    start = 0;
  // Return early if start > this.length. Done here to prevent potential uint32
  // coercion fail below.
  if (start > this.length)
    return '';

  if (end === undefined || end > this.length)
    end = this.length;

  // Force coersion to uint32. This will also coerce falsey/NaN values to 0.
  start >>>= 0;
  end >>>= 0;

  if (end <= start)
    return '';

There's a different between this and this PR's added tests. All NaN-ish/falsy values, except undefined, will always evaluate to 0 instead of this.length for end. This more closely matches current node behavior.

This change causes range errors to be thrown. If end = -1.2 and start = 1, then we'll pass the checks because -1.2 >>> 0 === 4294967295 and 4294967295 > 1 but this should return with ''. When I remove these lines:

 // Force coersion to uint32. This will also coerce falsey/NaN values to 0.
 start >>>= 0;
 end >>>= 0;

and change the tests so that all falsy values evaluate to a length of 0, the tests pass. Are we clear to remove these shifts or were they required for a reason not captured by the tests?

Missed that while trying to handle another case. If both start and end are both NaN then it won't short circut and return an empty string, and was attempting to get around the isNaN() check by using the bit shift. Basically, was going for this:

  if (end <= start)
    return '';

  // Force coersion to uint32. This will also coerce falsey/NaN values to 0.
  start >>>= 0;
  end >>>= 0;

  if (end <= start)
    return '';

Looks dumb, but should capture all values we're looking for. If we're going to have this same check in C++ then might as well skip the second one and allow the native side to return an empty string in that unlikely case.

I think I've captured this desired logic in the latest iteration.

We'll discuss this in another PR.

Ok, unfortunately I don't have context on why this change was made in the first place.

yeah. and the changes you've made so far are excellent and I don't want to hold them back while we discuss how to handle this case.

but for future reference, the idea was to keep the numeric type the same all the way through. Since JS is doing uint32 coercions to the values, wanted to propagate this. Otherwise a uint32 value > max int32 could case to strange behavior. not a problem thus far b/c length can't get that large, but not a great way to be.

@matthewloring If there's no response by Friday, ping me and we'll get this in.

May want to make mention here that the reason for only checking undefined here is adhering to ECMA-262 6th Edition, Section 13.3.3.7 Runtime Semantics: KeyedBindingInitialization. In that if the argument is undefined then the "defaultValue" is used. Otherwise other arguments are coerced normally.

Done.

If `start` is not a valid number in the range, then the default value zero will be used. Same way, if `end` is not a valid number in the accepted range, then, by default, the length of the buffer is assumed. Ref: #2668 Ref: #2919 PR-URL: #4019

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

If no applicable issues then LGTM.

Failure seems unrelated.

If `start` is not a valid number in the range, then the default value zero will be used. Same way, if `end` is not a valid number in the accepted range, then, by default, the length of the buffer is assumed. Fixes: #2668 Ref: #2919 PR-URL: #4019 Reviewed-By: Trevor Norris <trev.norris@gmail.com>

Verify that start and end are coerced properly. Ref: #2919 PR-URL: #4019 Reviewed-By: Trevor Norris <trev.norris@gmail.com>

Landed in ec83654 and a04721d. Split the changes in test/ so @thefourtheye is the author. Thanks much getting this finished!

@jasnell This change may be semver-major, at the minimum is semver-minor.

If `start` is not a valid number in the range, then the default value zero will be used. Same way, if `end` is not a valid number in the accepted range, then, by default, the length of the buffer is assumed. Fixes: #2668 Ref: #2919 PR-URL: #4019 Reviewed-By: Trevor Norris <trev.norris@gmail.com>

Verify that start and end are coerced properly. Ref: #2919 PR-URL: #4019 Reviewed-By: Trevor Norris <trev.norris@gmail.com>

If `start` is not a valid number in the range, then the default value zero will be used. Same way, if `end` is not a valid number in the accepted range, then, by default, the length of the buffer is assumed. Fixes: nodejs#2668 Ref: nodejs#2919 PR-URL: nodejs#4019 Reviewed-By: Trevor Norris <trev.norris@gmail.com>

Verify that start and end are coerced properly. Ref: nodejs#2919 PR-URL: nodejs#4019 Reviewed-By: Trevor Norris <trev.norris@gmail.com>

mscdex added the buffer Issues and PRs related to the buffer subsystem. label Nov 25, 2015

trevnorris reviewed Nov 30, 2015
View reviewed changes

trevnorris reviewed Dec 2, 2015
View reviewed changes

trevnorris reviewed Dec 4, 2015
View reviewed changes

buffer: fix range checking for slowToString

6b9623e

If `start` is not a valid number in the range, then the default value zero will be used. Same way, if `end` is not a valid number in the accepted range, then, by default, the length of the buffer is assumed. Ref: #2668 Ref: #2919 PR-URL: #4019

thefourtheye added a commit that referenced this pull request Dec 7, 2015

test: check range fix for slowToString

a04721d

Verify that start and end are coerced properly. Ref: #2919 PR-URL: #4019 Reviewed-By: Trevor Norris <trev.norris@gmail.com>

trevnorris closed this Dec 8, 2015

thefourtheye added a commit that referenced this pull request Dec 8, 2015

test: check range fix for slowToString

1d50819

Verify that start and end are coerced properly. Ref: #2919 PR-URL: #4019 Reviewed-By: Trevor Norris <trev.norris@gmail.com>

This was referenced Dec 8, 2015

Release proposal: v5.2.0 #4180

Closed

Release proposal: v5.2.0 (Stable) #4181

Merged

rvagg mentioned this pull request Dec 17, 2015

Release 4.2.4 Planning #4321

Closed

jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Dec 17, 2015

matthewloring deleted the range-fix branch February 25, 2016 19:54

scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016

test: check range fix for slowToString

6c451ca

Verify that start and end are coerced properly. Ref: nodejs#2919 PR-URL: nodejs#4019 Reviewed-By: Trevor Norris <trev.norris@gmail.com>

buffer: fix range checking for slowToString by matthewloring · Pull Request #4019 · nodejs/node · GitHub

Uh oh!

Uh oh!

buffer: fix range checking for slowToString - #4019

buffer: fix range checking for slowToString#4019matthewloring wants to merge 1 commit intonodejs:masternodejs/node:masterfrom matthewloring:range-fixCopy head branch name to clipboard

Conversation

matthewloring commented Nov 25, 2015

Uh oh!

matthewloring commented Nov 25, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trevnorris commented Dec 1, 2015

Uh oh!

matthewloring commented Dec 1, 2015

Uh oh!

trevnorris commented Dec 2, 2015

Uh oh!

matthewloring commented Dec 2, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trevnorris commented Dec 2, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trevnorris commented Dec 7, 2015

Uh oh!

matthewloring commented Dec 7, 2015

Uh oh!

trevnorris commented Dec 8, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

buffer: fix range checking for slowToString#4019
matthewloring wants to merge 1 commit into
nodejs:masterfrom
matthewloring:range-fix