buffer: convert offset & length to int properly by thefourtheye · Pull Request #9815 · nodejs/node

thefourtheye · 2016-11-27T10:30:04Z

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 util

Description of change

As per ecma-262 2015's #sec-%typedarray%-buffer-byteoffset-length,
offset would be an integer, not a 32 bit unsigned integer. Also,
length would be an integer with the maximum value of 2^53 - 1, not a
32 bit unsigned integer.

This would be a problem because, if we create a buffer from an
arraybuffer, from an offset which is greater than 2^32, it would be
actually pointing to a different location in arraybuffer. For example,
if we use 2^40 as offset, then the actual value used will be 0,
because byteOffset >>>= 0 will convert byteOffset to a 32 bit
unsigned int, which is based on 2^32 modulo.

This is a redo, as the ca37fa5 broke
CI.

Refer: #9814
Refer: #9492

cc @nodejs/buffer @Trott

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

As per ecma-262 2015's #sec-%typedarray%-buffer-byteoffset-length, `offset` would be an integer, not a 32 bit unsigned integer. Also, `length` would be an integer with the maximum value of 2^53 - 1, not a 32 bit unsigned integer. This would be a problem because, if we create a buffer from an arraybuffer, from an offset which is greater than 2^32, it would be actually pointing to a different location in arraybuffer. For example, if we use 2^40 as offset, then the actual value used will be 0, because `byteOffset >>>= 0` will convert `byteOffset` to a 32 bit unsigned int, which is based on 2^32 modulo. This is a redo, as the ca37fa5 broke CI. Refer: nodejs#9814 Refer: nodejs#9492

targos · 2016-11-27T10:40:27Z

test/parallel/test-buffer-creation-regression.js

+
+for (let caseIndex = 0; caseIndex < testCases.length; caseIndex += 1) {
+  const [size, offset, length] = testCases[caseIndex];
+  if (os.freemem() < size) {


could you use a try/catch like we have here for example ?

@targos Makes sense. Updated the PR. Thanks :-)

thefourtheye · 2016-11-27T11:09:20Z

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

thefourtheye · 2016-11-27T11:34:25Z

CI Run, with all error messages: https://ci.nodejs.org/job/node-test-pull-request/4992/

targos

LGTM

thefourtheye · 2016-12-02T04:25:29Z

@nodejs/collaborators Landing this tomorrow, if there are no objections.

mcollina · 2016-12-02T05:11:58Z

LGTM

thefourtheye · 2016-12-03T06:27:03Z

CI Run before Landing: https://ci.nodejs.org/job/node-test-pull-request/5142/

thefourtheye · 2016-12-05T12:46:30Z

Landed in 720d01f

As per ecma-262 2015's #sec-%typedarray%-buffer-byteoffset-length, `offset` would be an integer, not a 32 bit unsigned integer. Also, `length` would be an integer with the maximum value of 2^53 - 1, not a 32 bit unsigned integer. This would be a problem because, if we create a buffer from an arraybuffer, from an offset which is greater than 2^32, it would be actually pointing to a different location in arraybuffer. For example, if we use 2^40 as offset, then the actual value used will be 0, because `byteOffset >>>= 0` will convert `byteOffset` to a 32 bit unsigned int, which is based on 2^32 modulo. This is a redo, as the ca37fa5 broke CI. Refer: #9814 Refer: #9492 PR-URL: #9815 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Trott · 2016-12-07T05:07:03Z

I hate to say it, but it sure looks like the test is broken on SmartOS. See, for example, https://ci.nodejs.org/job/node-test-commit-smartos/5632/nodes=smartos15-64/console:

not ok 109 parallel/test-buffer-creation-regression
  ---
  duration_ms: 76.135
  severity: fail
  stack: |-
    timeout

Trott · 2016-12-07T05:10:35Z

(Since it may be just that the test is taking too long to finish, the solution may be to move the test to sequential and/or split each of the three test cases into its own test file so each test case has the full 60 seconds to run.)

Trott · 2016-12-07T05:18:09Z

Moved to sequential in #10161. If that's insufficient, I'll try breaking it into multiple tests.

As per ecma-262 2015's #sec-%typedarray%-buffer-byteoffset-length, `offset` would be an integer, not a 32 bit unsigned integer. Also, `length` would be an integer with the maximum value of 2^53 - 1, not a 32 bit unsigned integer. This would be a problem because, if we create a buffer from an arraybuffer, from an offset which is greater than 2^32, it would be actually pointing to a different location in arraybuffer. For example, if we use 2^40 as offset, then the actual value used will be 0, because `byteOffset >>>= 0` will convert `byteOffset` to a 32 bit unsigned int, which is based on 2^32 modulo. This is a redo, as the ca37fa5 broke CI. Refer: nodejs#9814 Refer: nodejs#9492 PR-URL: nodejs#9815 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

MylesBorins · 2016-12-20T20:15:57Z

@thefourtheye should this be backported? I figure we should likely wait a bit to make sure there are not other breakages

thefourtheye · 2016-12-20T23:45:56Z

@thealphanerd This is actually a bug fix and we can backport this. We just have to backport all the relevant commits in the same order so that CI will not fail. I can backport this, if you are okay.

MylesBorins · 2016-12-21T19:45:29Z

@thefourtheye please do

trevnorris · 2017-01-11T21:27:20Z

@MylesBorins should this be reopened while the commits are backported?

trevnorris · 2017-01-11T21:27:36Z

oh, guess we can't since the branch was deleted.

MylesBorins · 2017-01-23T01:57:20Z

ping @thefourtheye

thefourtheye · 2017-02-05T14:13:29Z

@MylesBorins I backported this in #11176

thefourtheye · 2017-02-05T14:14:29Z

Removed 4.x lts watch, as this patch is not applicable to 4.x branch. It doesn't have the fromArrayBuffer function itself.

As per ecma-262 2015's #sec-%typedarray%-buffer-byteoffset-length, `offset` would be an integer, not a 32 bit unsigned integer. Also, `length` would be an integer with the maximum value of 2^53 - 1, not a 32 bit unsigned integer. This would be a problem because, if we create a buffer from an arraybuffer, from an offset which is greater than 2^32, it would be actually pointing to a different location in arraybuffer. For example, if we use 2^40 as offset, then the actual value used will be 0, because `byteOffset >>>= 0` will convert `byteOffset` to a 32 bit unsigned int, which is based on 2^32 modulo. This is a redo, as the ca37fa5 broke CI. Refer: #9814 Refer: #9492 PR-URL: #9815 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Backport-Of: #9815 PR-URL: #11176 Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. util Issues and PRs related to the built-in util module. labels Nov 27, 2016

targos reviewed Nov 27, 2016

View reviewed changes

avoid failing in race condition

9aa3ab5

include all expected error messages

7d4f1c7

targos approved these changes Nov 27, 2016

View reviewed changes

trevnorris approved these changes Nov 29, 2016

View reviewed changes

jasnell approved these changes Dec 2, 2016

View reviewed changes

cjihrig approved these changes Dec 2, 2016

View reviewed changes

thefourtheye closed this Dec 5, 2016

thefourtheye deleted the redo-9492 branch December 5, 2016 12:47

Fishrock123 mentioned this pull request Dec 5, 2016

v7.2.1 proposal #10127

Merged

2 tasks

Trott mentioned this pull request Dec 7, 2016

test: move long-running test to sequential #10161

Closed

2 tasks

MylesBorins added lts-watch-v4.x labels Dec 20, 2016

thefourtheye mentioned this pull request Feb 5, 2017

Backport 9815 & 10161 to v6.x #11176

Closed

3 tasks

thefourtheye removed the lts-watch-v4.x label Feb 5, 2017

thefourtheye added the dont-land-on-v4.x label Feb 5, 2017

MylesBorins added dont-land-on-v6.x and removed lts-watch-v6.x labels Mar 7, 2017

Uh oh!

Conversation

thefourtheye commented Nov 27, 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!

targos Nov 27, 2016

Choose a reason for hiding this comment

Uh oh!

thefourtheye Nov 27, 2016

Choose a reason for hiding this comment

Uh oh!

thefourtheye commented Nov 27, 2016

Uh oh!

thefourtheye commented Nov 27, 2016

Uh oh!

targos left a comment

Choose a reason for hiding this comment

Uh oh!

thefourtheye commented Dec 2, 2016

Uh oh!

mcollina commented Dec 2, 2016

Uh oh!

thefourtheye commented Dec 3, 2016

Uh oh!

thefourtheye commented Dec 5, 2016

Uh oh!

Trott commented Dec 7, 2016

Uh oh!

Trott commented Dec 7, 2016

Uh oh!

Trott commented Dec 7, 2016

Uh oh!

MylesBorins commented Dec 20, 2016

Uh oh!

thefourtheye commented Dec 20, 2016

Uh oh!

MylesBorins commented Dec 21, 2016

Uh oh!

trevnorris commented Jan 11, 2017

Uh oh!

trevnorris commented Jan 11, 2017

Uh oh!

MylesBorins commented Jan 23, 2017

Uh oh!

thefourtheye commented Feb 5, 2017

Uh oh!

thefourtheye commented Feb 5, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

thefourtheye commented Nov 27, 2016 •

edited

Loading