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

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
constbuf=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:

constbuf=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]);constchunk1=Buffer.from([1,29,0,0,1,143,216,162,92,254,248,63,0]);constchunk2=Buffer.from([0,0,18,184,6,0,175,29,0,8,11,1,0,0]);constmiddle=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

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 [email protected] 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 <[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]>

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 2c829c5Compare 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 8898862Compare November 2, 2016 12:17

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

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

cjihrig merged commit a02b13f into nodejs:masterNov 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

Uh oh!

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

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

Conversation

cjihrig commented Oct 28, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

Fishrock123 commented Oct 28, 2016

Uh oh!

cjihrig commented Oct 28, 2016

Uh oh!

Fishrock123 commented Oct 28, 2016

Uh oh!

thefourtheyeOct 28, 2016

Choose a reason for hiding this comment

Uh oh!

cjihrigOct 28, 2016

Choose a reason for hiding this comment

Uh oh!

trevnorrisOct 30, 2016

Choose a reason for hiding this comment

Uh oh!

cjihrig commented Oct 28, 2016

Uh oh!

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

Uh oh!

Uh oh!

cjihrig commented Oct 29, 2016

Uh oh!

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

Uh oh!

Uh oh!

cjihrig commented Oct 29, 2016

Uh oh!

rvagg commented Oct 29, 2016

Uh oh!

trevnorris commented Oct 30, 2016

Uh oh!

cjihrig commented Oct 30, 2016

Uh oh!

trevnorris commented Nov 1, 2016

Uh oh!

cjihrig commented Nov 2, 2016

Uh oh!

mscdexNov 2, 2016

Choose a reason for hiding this comment

Uh oh!

cjihrigNov 2, 2016

Choose a reason for hiding this comment

Uh oh!

mscdex commented Nov 2, 2016

Uh oh!

cjihrig commented Nov 2, 2016

Uh oh!

jasnell commented Nov 2, 2016

Uh oh!

cjihrig commented Nov 3, 2016

Uh oh!

cjihrig commented Nov 3, 2016

Uh oh!

jasnell commented Nov 3, 2016

Uh oh!

cjihrig commented Nov 3, 2016

Uh oh!

mscdex commented Nov 3, 2016

Uh oh!

trevnorris commented Nov 4, 2016

Uh oh!

jasnell commented Nov 4, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

cjihrig commented Oct 28, 2016•
edited
Loading

mscdex commented Oct 28, 2016•
edited
Loading

mscdex commented Oct 29, 2016•
edited
Loading