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

buffer: fix range checking for slowToString #4019

Closed
wants to merge 1 commit into from
Closed

buffer: fix range checking for slowToString #4019

wants to merge 1 commit into from

Conversation

matthewloring
Copy link

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

@matthewloring
Copy link
Author

@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?

@mscdex mscdex added the buffer Issues and PRs related to the buffer subsystem. label Nov 25, 2015
@@ -326,13 +326,34 @@ Object.defineProperty(Buffer.prototype, 'offset', {
function slowToString(encoding, start, end) {
var loweredCase = false;

start = start >>> 0;
end = end === undefined || end === Infinity ? this.length : end >>> 0;
if (start >= this.length || end <= 0 || Number.isNaN(end))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@trevnorris
Copy link
Contributor

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.

@matthewloring
Copy link
Author

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?

@trevnorris
Copy link
Contributor

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.

@matthewloring
Copy link
Author

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

@@ -172,7 +172,7 @@ inline MUST_USE_RESULT bool ParseArrayIndex(v8::Local<v8::Value> arg,
return true;
}

int32_t tmp_i = arg->Int32Value();
int32_t tmp_i = arg->Uint32Value();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll discuss this in another PR.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@trevnorris
Copy link
Contributor

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

// 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@trevnorris
Copy link
Contributor

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

If no applicable issues then LGTM.

@matthewloring
Copy link
Author

Failure seems unrelated.

trevnorris pushed a commit that referenced this pull request Dec 7, 2015
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>
thefourtheye added a commit that referenced this pull request Dec 7, 2015
Verify that start and end are coerced properly.

Ref: #2919
PR-URL: #4019
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@trevnorris
Copy link
Contributor

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.

@trevnorris trevnorris closed this Dec 8, 2015
rvagg pushed a commit that referenced this pull request Dec 8, 2015
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>
thefourtheye added a commit that referenced this pull request Dec 8, 2015
Verify that start and end are coerced properly.

Ref: #2919
PR-URL: #4019
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@rvagg rvagg mentioned this pull request Dec 17, 2015
@jasnell jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Dec 17, 2015
@matthewloring 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
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>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
Verify that start and end are coerced properly.

Ref: nodejs#2919
PR-URL: nodejs#4019
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants