buffer: coerce slice parameters consistenly - #9101

thefourtheye · 2016-10-14T19:32:45Z

Checklist

make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

buffer

Description of change

As shown in #9096, the offset and
end value of the slice call are just coerced to numbers and then
passed to FastBuffer, which internally truncates the mantissa part
if the number is actually a floating point number. This actually
affects the new length of the slice calculation. For example,

> const original = Buffer.from('abcd');
undefined
> original.slice(original.length / 3).toString()
'bc'

This happens because, starting value of the slice is 4 / 3, which is
1.33 (approximately). Now, the length of the slice is calculated as
the difference between the actual length of the buffer and the starting
offset. So, it becomes 2.67 (4 - 1.33). Now, a new FastBuffer is
constructed, with these values, actual buffer object, starting value
which is 1.33 and the the length 2.67. The underlying C++ code
truncates the numbers and they become 1 and 2. That is why the result
is just bc.

This patch makes sure that all the offsets are coerced to an integer.

Fixes: #9096

cc @nodejs/buffer

jasnell

LGTM if CI is green

cjihrig · 2016-10-14T19:37:44Z

It looks like | 0 is used more frequently than ~~ throughout the lib/ code.

@cjihrig Sure. Changed it now.

jasnell · 2016-10-14T20:00:19Z

extremely minor nit... offset |= 0; is even more concise ;-)

targos

LGTM

targos · 2016-10-14T20:05:21Z

typo: consistently

thefourtheye · 2016-10-14T20:24:08Z

@jasnell @targos Changed according to your suggestions..

mscdex · 2016-10-14T22:09:39Z

The Number.isNaN(offset) portion of this conditional should be able to be removed with this change since | will cause invalid inputs to always be zero (whereas + will not).

ghost

LGTM with @mscdex’ suggestion

ghost · 2016-10-14T23:43:10Z

nit: 17 can probably be original.length.

ghost · 2016-10-15T00:00:06Z

This comment seems a little confusing to me. The offsets are consistently converted to integers (in C++). The real problem is that the end value is calculated in javascript land before it gets passed to C++. If the length calculation (used to determine end) was done inside C++ this wouldn't be an issue.

It's also possible to use floating point numbers with slice() and still get a reasonable result. The only time it becomes an issue is when the end argument is not supplied and start is a floating point number.

You mean to say the slice implementation would be better done in C++?

Honestly, I would prefer to throw an error if the input is not an integer, which would avoid all this.

I removed the text in the comments now and improved the commit log a little.

As shown in nodejs#9096, the offset and end value of the `slice` call are coerced to numbers and then passed to `FastBuffer`, which internally truncates the mantissa part if the number is actually a floating point number. This actually affects the new length of the slice calculation. For example, > const original = Buffer.from('abcd'); undefined > original.slice(original.length / 3).toString() 'bc' This happens because, starting value of the slice is 4 / 3, which is 1.33 (approximately). Now, the length of the slice is calculated as the difference between the actual length of the buffer and the starting offset. So, it becomes 2.67 (4 - 1.33). Now, a new `FastBuffer` is constructed, with the following values as parameters, 1. actual buffer object, 2. starting value, which is 1.33 and 3. the length 2.67. The underlying C++ code truncates the numbers and they become 1 and 2. That is why the result is just `bc`. This patch makes sure that all the offsets are coerced to integers before any calculations are done. Fixes: nodejs#9096

ghost · 2016-10-15T03:13:24Z

LGTM if CI is ok with it: https://ci.nodejs.org/job/node-test-pull-request/4526/

ghost · 2016-10-17T10:52:43Z

For any gage how this should work, would be most similar to String#slice().

ghost · 2016-10-17T11:24:14Z

@trevnorris String's slice also works like this only

> const str = "abcd"
undefined
> str.slice(str.length / 3)
'bcd'
> str.slice(str.length / 3, str.length)
'bcd'

ghost · 2016-10-17T16:29:03Z

What would the semver-iness of this change be?

ghost

LGTM

ghost · 2016-10-17T19:02:38Z

I think it's a bugfix.

ghost · 2016-10-17T19:03:37Z

@jasnell I'd classify it as a bug fix, but if anything substantial relies on the old behavior a lot of angry people could show up :-P

ghost · 2016-10-17T19:41:30Z

+1 for a bugfix.

ghost · 2016-10-18T13:14:45Z

+1 to considering this a bugfix, too.

CITGM to account for the “but if anything substantial relies on the old behavior” part: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/425

ghost · 2016-10-18T22:53:00Z

Landing as a semver-patch!

ghost · 2016-10-18T22:53:39Z

Landed in ee14690

ghost · 2016-10-27T20:56:34Z

@jasnell Did my comment about introducing another bug by using |=, and the test that accompanied that, get overlooked?

ghost · 2016-10-27T21:17:38Z

@trevnorris it looks like there is a good chance of that. What comment are you referring to? This is the only old comment I'm seeing on this thread.

ghost · 2016-10-17T10:46:05Z

 function adjustOffset(offset, length) {
-  offset = +offset;
-  if (offset === 0 || Number.isNaN(offset)) {
+  offset |= 0;


If you want the floor then please use Math.floor(). Using | allows wrap around. One reason why + was used before.

@thefourtheye

String's slice also works like this only

This fails edge cases. For example:

'abcdefg'.slice(-(-1 >>> 0) - 1) === 'abcdefg';

But as this currently stands offset will be coerced to 2 with the above example.

ghost · 2016-10-17T10:47:19Z

    return 0;
-  }
-  if (offset < 0) {
+  } else if (offset < 0) {


Here's where the problem lies. There's no range check before the int32 coercion. A large number could fail this.

ghost · 2016-10-17T20:03:40Z

+  assert.strictEqual(
+    buf.slice(buf.length / 3, buf.length).toString(),
+    'bcd'
+  );


If we're making the change to operate like String#slice() then edge case tests should be included. e.g.:

const b = Buffer.from('abcdefg'); assert.strictEqual(b.slice(-(-1 >>> 0) + 1).toString(), b.toString());

ghost · 2016-10-28T05:25:41Z

@cjihrig oh shiet. this was my first time using the review thing before, and didn't realize I needed to properly submit it before the notes could be seen. just did, and that'll show the regression this introduced.

ghost · 2016-10-28T14:09:26Z

The old "forgot to submit the review" problem. Got me on multiple PRs the first day I used it :-)

ghost · 2016-10-28T14:42:09Z

@trevnorris reverted the lib/buffer.js changes and used Math.floor() in #9341.

nodejs-github-bot added the buffer Issues and PRs related to the buffer subsystem. label Oct 14, 2016

jasnell approved these changes Oct 14, 2016

View reviewed changes

cjihrig reviewed Oct 14, 2016

View reviewed changes

thefourtheye force-pushed the make-buffer-slice-offset-integer branch from 17bcccd to 6929c09 Compare October 14, 2016 19:45

jasnell reviewed Oct 14, 2016

View reviewed changes

Comment thread lib/buffer.js Outdated

jasnell Oct 14, 2016

Copy link
Copy Markdown

Member

Choose a reason for hiding this comment

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

extremely minor nit... offset |= 0; is even more concise ;-)

targos approved these changes Oct 14, 2016

View reviewed changes

Comment thread test/parallel/test-buffer-slice.js Outdated

targos Oct 14, 2016

Copy link
Copy Markdown

Member

Choose a reason for hiding this comment

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

typo: consistently

thefourtheye force-pushed the make-buffer-slice-offset-integer branch from 6929c09 to 94d3642 Compare October 14, 2016 20:23

mscdex reviewed Oct 14, 2016

View reviewed changes

addaleax approved these changes Oct 14, 2016

View reviewed changes

Comment thread test/parallel/test-buffer-slice.js Outdated

ghost Oct 14, 2016

Copy link
Copy Markdown

Member

Choose a reason for hiding this comment

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

nit: 17 can probably be original.length.

mscdex reviewed Oct 15, 2016

View reviewed changes

thefourtheye force-pushed the make-buffer-slice-offset-integer branch from 94d3642 to e648180 Compare October 15, 2016 02:57

thefourtheye force-pushed the make-buffer-slice-offset-integer branch from e648180 to 9fc562c Compare October 15, 2016 03:02

fhinkel approved these changes Oct 16, 2016

View reviewed changes

cjihrig approved these changes Oct 16, 2016

View reviewed changes

lpinca approved these changes Oct 17, 2016

View reviewed changes

rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:02

jasnell self-assigned this Oct 18, 2016

jasnell closed this Oct 18, 2016

mscdex added the lts-watch-v6.x label Oct 19, 2016

thefourtheye deleted the make-buffer-slice-offset-integer branch October 19, 2016 04:19

trevnorris reviewed Oct 28, 2016

View reviewed changes

cjihrig mentioned this pull request Oct 28, 2016

buffer: coerce offset using Math.trunc() #9341

Merged

3 tasks done

MylesBorins added dont-land-on-v4.x and removed lts-watch-v6.x labels Nov 18, 2016

MylesBorins mentioned this pull request Nov 22, 2016

v6.9.2 proposal #9735

Merged

Conversation

thefourtheye commented Oct 14, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

cjihrig Oct 14, 2016

Choose a reason for hiding this comment

Uh oh!

thefourtheye Oct 14, 2016

Choose a reason for hiding this comment

Uh oh!

jasnell Oct 14, 2016

Choose a reason for hiding this comment

Uh oh!

targos left a comment

Choose a reason for hiding this comment

Uh oh!

targos Oct 14, 2016

Choose a reason for hiding this comment

Uh oh!

thefourtheye commented Oct 14, 2016

Uh oh!

mscdex Oct 14, 2016

Choose a reason for hiding this comment

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

ghost Oct 14, 2016

Choose a reason for hiding this comment

Uh oh!

ghost Oct 15, 2016 • edited by mscdex Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost Oct 15, 2016

Choose a reason for hiding this comment

Uh oh!

ghost Oct 15, 2016

Choose a reason for hiding this comment

Uh oh!

ghost commented Oct 15, 2016 • edited by mscdex Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Oct 17, 2016

Uh oh!

ghost commented Oct 17, 2016

Uh oh!

ghost commented Oct 17, 2016

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Oct 17, 2016

Uh oh!

ghost commented Oct 17, 2016

Uh oh!

ghost commented Oct 17, 2016

Uh oh!

ghost commented Oct 18, 2016

Uh oh!

ghost commented Oct 18, 2016

Uh oh!

ghost commented Oct 18, 2016

Uh oh!

ghost commented Oct 27, 2016 • edited by trevnorris Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Oct 27, 2016

Uh oh!

ghost Oct 17, 2016

Choose a reason for hiding this comment

Uh oh!

ghost Oct 17, 2016

Choose a reason for hiding this comment

Uh oh!

ghost Oct 15, 2016 •

edited by mscdex

Loading

ghost commented Oct 15, 2016 •

edited by mscdex

Loading

ghost commented Oct 27, 2016 •

edited by trevnorris

Loading