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

lib/buffer.js

@@ -807,7 +807,7 @@ Buffer.prototype.toJSON = function() {


 function adjustOffset(offset, length) {
-  offset = +offset;
+  offset = ~~offset;


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

@cjihrig Sure. Changed it now.

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

lib/buffer.js

@@ -807,7 +807,7 @@ Buffer.prototype.toJSON = function() {


 function adjustOffset(offset, length) {
-  offset = +offset;
+  offset = offset | 0;


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

targos

LGTM

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

test/parallel/test-buffer-slice.js

+{
+  // Test included in https://github.com/nodejs/node/issues/9096
+  // If the offset or ending value provided is a floating point number, it
+  // should be coerced to integer consistenly. Otherwise the actual length of


typo: consistently

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

@jasnell @targos Changed according to your suggestions..

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

lib/buffer.js

@@ -807,7 +807,7 @@ Buffer.prototype.toJSON = function() {


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


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).

addaleax

LGTM with @mscdex’ suggestion

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

test/parallel/test-buffer-slice.js

+  // the slice calculation will be messed up.
+  const original = Buffer.from('1234567890ABCDEFG', 'ascii');
+  const chunk1 = original.slice(0, original.length / 3);
+  const chunk2 = original.slice(original.length / 3, 17);


nit: 17 can probably be original.length.

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

test/parallel/test-buffer-slice.js

+  // Test included in https://github.com/nodejs/node/issues/9096
+  // If the offset or ending value provided is a floating point number, it
+  // should be coerced to integer consistently. Otherwise the actual length of
+  // the slice calculation will be messed up.


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

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

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

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

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

thefourtheye · 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'

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

What would the semver-iness of this change be?

lpinca

LGTM

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

I think it's a bugfix.

cjihrig · 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

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

+1 for a bugfix.

addaleax · 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

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

Landing as a semver-patch!

As shown in #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: #9096 PR-URL: #9101 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Brian White <[email protected]>

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

Landed in ee14690

As shown in #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: #9096 PR-URL: #9101 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Brian White <[email protected]>

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

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

cjihrig · 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.

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

lib/buffer.js

@@ -807,11 +807,10 @@ Buffer.prototype.toJSON = function() {


 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.

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

lib/buffer.js

    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.

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

test/parallel/test-buffer-slice.js

+  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());

trevnorris · 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.

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

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

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

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

This is a partial revert of 14d1a8a, which coerced the offset of Buffer#slice() using the | operator. This causes some edge cases to be handled incorrectly. This commit restores the old behavior, but converts offsets to integers using Math.trunc(). This commit does not revert any tests, and adds an additional regression test. Refs: nodejs#9096 Refs: nodejs#9101 PR-URL: nodejs#9341 Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Brian White <[email protected]> Reviewed-By: Trevor Norris <[email protected]>

This is a partial revert of 14d1a8a, which coerced the offset of Buffer#slice() using the | operator. This causes some edge cases to be handled incorrectly. This commit restores the old behavior, but converts offsets to integers using Math.trunc(). This commit does not revert any tests, and adds an additional regression test. Refs: #9096 Refs: #9101 PR-URL: #9341 Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Brian White <[email protected]> Reviewed-By: Trevor Norris <[email protected]>

As shown in #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: #9096 PR-URL: #9101 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Brian White <[email protected]>

This is a partial revert of 14d1a8a, which coerced the offset of Buffer#slice() using the | operator. This causes some edge cases to be handled incorrectly. This commit restores the old behavior, but converts offsets to integers using Math.trunc(). This commit does not revert any tests, and adds an additional regression test. Refs: #9096 Refs: #9101 PR-URL: #9341 Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Brian White <[email protected]> Reviewed-By: Trevor Norris <[email protected]>

As shown in #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: #9096 PR-URL: #9101 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Brian White <[email protected]>

This is a partial revert of 14d1a8a, which coerced the offset of Buffer#slice() using the | operator. This causes some edge cases to be handled incorrectly. This commit restores the old behavior, but converts offsets to integers using Math.trunc(). This commit does not revert any tests, and adds an additional regression test. Refs: #9096 Refs: #9101 PR-URL: #9341 Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Brian White <[email protected]> Reviewed-By: Trevor Norris <[email protected]>

This LTS release comes with 144 commits. This includes 47 that are docs related, 46 that are test related, 15 which are build / tools related, and 9 commits which are updates to dependencies Notable Changes: * buffer: - coerce slice parameters consistently (Sakthipriyan Vairamani (thefourtheye)) #9101 * deps: - *npm*: - upgrade npm to 3.10.9 (Kat Marchán) #9286 - *V8*: - Various fixes to destructuring edge cases - cherry-pick 3c39bac from V8 upstream (Cristian Cavalli) #9138 - cherry pick 7166503 from upstream v8 (Cristian Cavalli) #9173 * gtest: - the test reporter now outputs tap comments as yamlish (Johan Bergström) #9262 * inspector: - inspector now prompts user to use 127.0.0.1 rather than localhost (Eugene Ostroukhov) #9451 * tls: - fix memory leak when writing data to TLSWrap instance during handshake (Fedor Indutny) #9586 PR-URL: #9735

This LTS release comes with 144 commits. This includes 47 that are docs related, 46 that are test related, 15 which are build / tools related, and 9 commits which are updates to dependencies Notable Changes: * buffer: - coerce slice parameters consistently (Sakthipriyan Vairamani (thefourtheye)) nodejs/node#9101 * deps: - *npm*: - upgrade npm to 3.10.9 (Kat Marchan) nodejs/node#9286 - *V8*: - Various fixes to destructuring edge cases - cherry-pick 3c39bac from V8 upstream (Cristian Cavalli) nodejs/node#9138 - cherry pick 7166503 from upstream v8 (Cristian Cavalli) nodejs/node#9173 * gtest: - the test reporter now outputs tap comments as yamlish (Johan Bergstrom) nodejs/node#9262 * inspector: - inspector now prompts user to use 127.0.0.1 rather than localhost (Eugene Ostroukhov) nodejs/node#9451 * tls: - fix memory leak when writing data to TLSWrap instance during handshake (Fedor Indutny) nodejs/node#9586 PR-URL: nodejs/node#9735 Signed-off-by: Ilkka Myller <[email protected]>

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

targos approved these changes Oct 14, 2016

View reviewed changes

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

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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

buffer: coerce slice parameters consistenly #9101

buffer: coerce slice parameters consistenly #9101

thefourtheye commented Oct 14, 2016

jasnell left a comment

cjihrig Oct 14, 2016

thefourtheye Oct 14, 2016

jasnell Oct 14, 2016

targos left a comment

targos Oct 14, 2016

thefourtheye commented Oct 14, 2016

mscdex Oct 14, 2016

addaleax left a comment

addaleax Oct 14, 2016

mscdex Oct 15, 2016 •

edited

Loading

thefourtheye Oct 15, 2016

thefourtheye Oct 15, 2016

mscdex commented Oct 15, 2016 •

edited

Loading

trevnorris commented Oct 17, 2016

thefourtheye commented Oct 17, 2016

jasnell commented Oct 17, 2016

lpinca left a comment

lpinca commented Oct 17, 2016

cjihrig commented Oct 17, 2016

ChALkeR commented Oct 17, 2016

addaleax commented Oct 18, 2016

jasnell commented Oct 18, 2016

jasnell commented Oct 18, 2016

trevnorris commented Oct 27, 2016 •

edited

Loading

cjihrig commented Oct 27, 2016

trevnorris Oct 17, 2016

trevnorris Oct 17, 2016

trevnorris Oct 17, 2016

trevnorris Oct 17, 2016

trevnorris commented Oct 28, 2016

cjihrig commented Oct 28, 2016

cjihrig commented Oct 28, 2016

buffer: coerce slice parameters consistenly #9101

buffer: coerce slice parameters consistenly #9101

Conversation

thefourtheye commented Oct 14, 2016

Checklist

Affected core subsystem(s)

Description of change

jasnell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

targos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thefourtheye commented Oct 14, 2016

Choose a reason for hiding this comment

addaleax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex Oct 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex commented Oct 15, 2016 • edited Loading

trevnorris commented Oct 17, 2016

thefourtheye commented Oct 17, 2016

jasnell commented Oct 17, 2016

lpinca left a comment

Choose a reason for hiding this comment

lpinca commented Oct 17, 2016

cjihrig commented Oct 17, 2016

ChALkeR commented Oct 17, 2016

addaleax commented Oct 18, 2016

jasnell commented Oct 18, 2016

jasnell commented Oct 18, 2016

trevnorris commented Oct 27, 2016 • edited Loading

cjihrig commented Oct 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trevnorris commented Oct 28, 2016

cjihrig commented Oct 28, 2016

cjihrig commented Oct 28, 2016

mscdex Oct 15, 2016 •

edited

Loading

mscdex commented Oct 15, 2016 •

edited

Loading

trevnorris commented Oct 27, 2016 •

edited

Loading