-
Notifications
You must be signed in to change notification settings - Fork 30k
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
Conversation
@trevnorris @thefourtheye I believe I've actioned all of the comments in #2919 except changing |
@@ -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)) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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()
?
There was a problem hiding this comment.
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.
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 |
This change causes range errors to be thrown. If // 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 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. |
@@ -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(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
@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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
CI: https://ci.nodejs.org/job/node-test-pull-request/947/ If no applicable issues then LGTM. |
Failure seems unrelated. |
Landed in ec83654 and a04721d. Split the changes in @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: 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>
If
start
is not a valid number in the range, then the default valuezero will be used. Same way, if
end
is not a valid number in theaccepted range, then, by default, the length of the buffer is assumed.
Ref: #2668
PR-URL: #2919