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

cjihrig · 2016-10-28T14:41:11Z

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

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: #9101
Refs: #9096

cc: @nodejs/buffer

Fishrock123 · 2016-10-28T14:53:19Z

Does offset >> 0 fix these bugs rather than Math.floor()?

cjihrig · 2016-10-28T14:58:51Z

Did you mean offset >>> 0? If so, that would create an uint32, and slice() supports negative numbers.

Fishrock123 · 2016-10-28T15:07:15Z

@cjihrig no, I mean a signed shift. It does similar to what | 0 does but perhaps with less caveats.

thefourtheye · 2016-10-28T15:10:22Z

lib/buffer.js

-  offset |= 0;
-  if (offset === 0) {
+  offset = Math.floor(+offset);
+  if (offset === 0 || Number.isNaN(offset)) {


Wouldn't it be better if we had Number.isInteger here and drop the Math.floor in the previous line?

So, for example, an offset of 3.3 would become 0? That seems like it would be a surprising breaking change.

Since slice() supports negative numbers Math.round() must be used. A simple test for that would be

const buf = Buffer.from('abc'); assert.strictEqual(buf.slice(-0.5).toString(), buf.toString();

cjihrig · 2016-10-28T15:32:45Z

no, I mean a signed shift.

It looks like it has the same problems. Using the example from #9101:

> (-(-1 >>> 0) + 1) | 0
2
> (-(-1 >>> 0) + 1) >> 0
2

mscdex · 2016-10-28T18:47:58Z

I just noticed that this PR actually fixes another bug (that has existed since v6.3.0 or ac8e1bf) not covered by the regression test in this PR. Here is a test case to add:

const buf = Buffer.from([
  1, 29,  0,   0, 1, 143, 216, 162, 92, 254, 248, 63, 0,
  0,  0, 18, 184, 6,   0, 175,  29,  0,   8,  11,  1, 0, 0
]);
const chunk1 = Buffer.from([
  1, 29, 0, 0, 1, 143, 216, 162, 92, 254, 248, 63, 0
]);
const chunk2 = Buffer.from([
  0, 0, 18, 184, 6, 0, 175, 29, 0, 8, 11, 1, 0, 0
]);
const middle = buf.length / 2;
assert.deepStrictEqual(buf.slice(0, middle), chunk1);
assert.deepStrictEqual(buf.slice(middle), chunk2);

cjihrig · 2016-10-29T17:47:54Z

@mscdex that code is passing for me on master already, probably because of 14d1a8a.

mscdex · 2016-10-29T17:50:41Z

@cjihrig It doesn't pass on v6.x though. Will both commits get backported to v6.x?

cjihrig · 2016-10-29T18:01:09Z

I think it would be simplest to backport both together.

rvagg · 2016-10-29T23:26:41Z

Might be good to add that new test case to this PR even if it passes on master. Moar regression tests!

This will be good to expedite into the next v6 patch release but it'd be preferable to get it into a v7 asap.

trevnorris · 2016-10-30T04:34:17Z

@cjihrig My bad. I meant to say to use round() in my PR review.

cjihrig · 2016-10-30T12:46:23Z

Using round() seems like it could lead to some confusion. What about Math.trunc() to just drop everything after the decimal point?

trevnorris · 2016-11-01T21:27:12Z

@cjihrig thanks for the correction. Math.trunc() is the correct solution.

cjihrig · 2016-11-02T01:14:45Z

OK, moved from Math.floor() to Math.trunc(), and added the code from #9341 (comment) and #9341 (comment) as regression tests.

mscdex · 2016-11-02T01:28:22Z

lib/buffer.js

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


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


Is the + here still necessary? It seems Math.trunc() handles conversions already?

I think you're right. Removed the +

mscdex · 2016-11-02T12:24:52Z

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

cjihrig · 2016-11-02T12:25:13Z

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

jasnell · 2016-11-02T14:44:20Z

given that use of | is pretty common already, would you mind adding a code comment indicating why Math.trunc() is used so that someone later on doesn't come along and change it? Otherwise LGTM

cjihrig · 2016-11-03T00:14:11Z

@jasnell I added a comment. Does it work for you?

cjihrig · 2016-11-03T00:15:03Z

Looks like there were two simultaneous CI runs today. One was green. The other had a single Jenkins issue.

jasnell · 2016-11-03T00:31:43Z

Yep! Thank you!

cjihrig · 2016-11-03T12:13:12Z

@trevnorris @mscdex @rvagg are any of you interested in signing off on this?

mscdex · 2016-11-03T12:20:25Z

LGTM

trevnorris · 2016-11-04T17:52:39Z

LGTM

@jasnell we use binary conversions in several places we shouldn't. Should probably be fixed at some point.

jasnell · 2016-11-04T17:56:13Z

Agreed

On Friday, November 4, 2016, Trevor Norris notifications@github.com wrote:

LGTM

@jasnell https://github.com/jasnell we use binary conversions in
several places we shouldn't. Should probably be fixed at some point.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#9341 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAa2eazC9UGhivwqia5zjnVbztBtippKks5q63DugaJpZM4Kjgw5
.

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 <thechargingvolcano@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: Trevor Norris <trev.norris@gmail.com>

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 <thechargingvolcano@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: Trevor Norris <trev.norris@gmail.com>

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

cjihrig mentioned this pull request Oct 28, 2016

buffer: coerce slice parameters consistenly #9101

Closed

3 tasks

thefourtheye reviewed Oct 28, 2016

View reviewed changes

thefourtheye approved these changes Oct 28, 2016

View reviewed changes

mscdex mentioned this pull request Oct 28, 2016

node-crc failing on node 6.x only #9342

Closed

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

cjihrig force-pushed the buf branch from bfa3fe6 to 2c829c5 Compare November 2, 2016 01:12

cjihrig changed the title ~~buffer: coerce offset using Math.floor()~~ buffer: coerce offset using Math.trunc() Nov 2, 2016

mscdex reviewed Nov 2, 2016

View reviewed changes

cjihrig force-pushed the buf branch from 2c829c5 to 8898862 Compare November 2, 2016 12:17

cjihrig force-pushed the buf branch from 8898862 to 9c71bf7 Compare November 3, 2016 00:13

cjihrig force-pushed the buf branch from 9c71bf7 to a02b13f Compare November 5, 2016 00:44

cjihrig merged commit a02b13f into nodejs:master Nov 5, 2016

cjihrig deleted the buf branch November 5, 2016 00:45

evanlucas mentioned this pull request Nov 7, 2016

v7.1.0 proposal - 2016-11-08 #9438

Merged

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 offset using Math.trunc() #9341

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

cjihrig commented Oct 28, 2016 •

edited

Loading

Fishrock123 commented Oct 28, 2016

cjihrig commented Oct 28, 2016

Fishrock123 commented Oct 28, 2016

thefourtheye Oct 28, 2016

cjihrig Oct 28, 2016

trevnorris Oct 30, 2016

cjihrig commented Oct 28, 2016

mscdex commented Oct 28, 2016 •

edited

Loading

cjihrig commented Oct 29, 2016

mscdex commented Oct 29, 2016 •

edited

Loading

cjihrig commented Oct 29, 2016

rvagg commented Oct 29, 2016

trevnorris commented Oct 30, 2016

cjihrig commented Oct 30, 2016

trevnorris commented Nov 1, 2016

cjihrig commented Nov 2, 2016

mscdex Nov 2, 2016

cjihrig Nov 2, 2016

mscdex commented Nov 2, 2016

cjihrig commented Nov 2, 2016

jasnell commented Nov 2, 2016

cjihrig commented Nov 3, 2016

cjihrig commented Nov 3, 2016

jasnell commented Nov 3, 2016

cjihrig commented Nov 3, 2016

mscdex commented Nov 3, 2016

trevnorris commented Nov 4, 2016

jasnell commented Nov 4, 2016

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

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

Conversation

cjihrig commented Oct 28, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

Fishrock123 commented Oct 28, 2016

cjihrig commented Oct 28, 2016

Fishrock123 commented Oct 28, 2016

thefourtheye Oct 28, 2016

Choose a reason for hiding this comment

cjihrig Oct 28, 2016

Choose a reason for hiding this comment

trevnorris Oct 30, 2016

Choose a reason for hiding this comment

cjihrig commented Oct 28, 2016

mscdex commented Oct 28, 2016 • edited Loading

cjihrig commented Oct 29, 2016

mscdex commented Oct 29, 2016 • edited Loading

cjihrig commented Oct 29, 2016

rvagg commented Oct 29, 2016

trevnorris commented Oct 30, 2016

cjihrig commented Oct 30, 2016

trevnorris commented Nov 1, 2016

cjihrig commented Nov 2, 2016

mscdex Nov 2, 2016

Choose a reason for hiding this comment

cjihrig Nov 2, 2016

Choose a reason for hiding this comment

mscdex commented Nov 2, 2016

cjihrig commented Nov 2, 2016

jasnell commented Nov 2, 2016

cjihrig commented Nov 3, 2016

cjihrig commented Nov 3, 2016

jasnell commented Nov 3, 2016

cjihrig commented Nov 3, 2016

mscdex commented Nov 3, 2016

trevnorris commented Nov 4, 2016

jasnell commented Nov 4, 2016

cjihrig commented Oct 28, 2016 •

edited

Loading

mscdex commented Oct 28, 2016 •

edited

Loading

mscdex commented Oct 29, 2016 •

edited

Loading