Revert "buffer: convert offset & length to int properly" #9814

Trott · 2016-11-26T17:55:25Z

Checklist

make -j8 test (UNIX), or vcbuild test nosign (Windows) passes

Affected core subsystem(s)

buffer

Description of change

This reverts commit ca37fa5.

The commit was landed without running through CI. A test provided by the commit fails on most (but not all) platforms on CI. See, for example, https://ci.nodejs.org/job/node-test-commit-linux/6250/nodes=centos5-32/console:

not ok 24 parallel/test-buffer-creation-regression
  ---
  duration_ms: 0.248
  severity: fail
  stack: |-
    /home/iojs/build/workspace/node-test-commit-linux/nodes/centos5-32/test/parallel/test-buffer-creation-regression.js:7
      const arrayBuffer = new ArrayBuffer(size);
                          ^
    
    RangeError: Invalid array buffer length
        at new ArrayBuffer (<anonymous>)
        at test (/home/iojs/build/workspace/node-test-commit-linux/nodes/centos5-32/test/parallel/test-buffer-creation-regression.js:7:23)
        at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux/nodes/centos5-32/test/parallel/test-buffer-creation-regression.js:21:1)
        at Module._compile (module.js:571:32)
        at Object.Module._extensions..js (module.js:580:10)
        at Module.load (module.js:488:32)
        at tryModuleLoad (module.js:447:12)
        at Function.Module._load (module.js:439:3)
        at Module.runMain (module.js:605:10)
        at run (bootstrap_node.js:420:7)

Refs: #9492

This reverts commit ca37fa5.

Trott · 2016-11-26T17:56:47Z

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

Trott · 2016-11-26T18:12:59Z

One failure on Linux is infrastructure-related, but let's re-run anyway (Linux-only): https://ci.nodejs.org/job/node-test-commit-linux/6252/

Trott · 2016-11-26T18:24:08Z

Linux re-run is green, rest of first CI is also green.

I think this warrants landing Real Soon Now rather than waiting the 72 hours, but I'd feel better getting a 👍 on that from a CTC person or two. Any takers?

cjihrig · 2016-11-26T18:30:55Z

Go for it!

ChALkeR · 2016-11-26T18:44:07Z

/cc @thefourtheye — it looks like #9492 should be re-done after this lands.

This reverts commit ca37fa5. A test provided by the commit fails on most (but not all) platforms on CI. PR-URL: nodejs#9814 Ref: nodejs#9492 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>

Trott · 2016-11-26T19:42:42Z

Landed in 6b2aa1a. Thanks.

brodycj · 2016-11-26T20:35:46Z

As a nit: I am happy to see the problematic test (test\parallel\test-buffer-creation-regression.js) but not the whole change reverted. The change does seem to solve a problem, it does not break any other tests, and the other 2 tests seem to pass OK if I am not mistaken here.

I think a better solution would have been to simply skip the "problematic"test, and better yet don't skip the "problematic" test on certain platform(s) where we know it will pass (macOS which is known to be 64-bit UNIX, for example). I think this would have kept the project history a little cleaner. Just a suggestion for the future.

/cc @thefourtheye

MylesBorins · 2016-11-27T01:53:34Z

@brodybits in general when we revert we do so of the entire atomic change. As the original change did not go through CI I think it makes a bunch of sense to revert the entire thing.

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

thefourtheye · 2016-11-27T10:31:52Z

I am extremely sorry for the trouble I created. Thanks @Trott for fixing this immediately.

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>

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>

This reverts commit ca37fa5. A test provided by the commit fails on most (but not all) platforms on CI. PR-URL: nodejs#9814 Ref: nodejs#9492 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>

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>

Revert "buffer: convert offset & length to int properly"

169603f

This reverts commit ca37fa5.

Trott added the buffer Issues and PRs related to the buffer subsystem. label Nov 26, 2016

nodejs-github-bot added the util Issues and PRs related to the built-in util module. label Nov 26, 2016

Trott mentioned this pull request Nov 26, 2016

buffer: convert offset & length to int properly #9492

Closed

3 tasks

bnoordhuis approved these changes Nov 26, 2016

View reviewed changes

cjihrig approved these changes Nov 26, 2016

View reviewed changes

targos approved these changes Nov 26, 2016

View reviewed changes

ChALkeR approved these changes Nov 26, 2016

View reviewed changes

Trott closed this Nov 26, 2016

thefourtheye mentioned this pull request Nov 27, 2016

buffer: convert offset & length to int properly #9815

Closed

3 tasks

addaleax added dont-land-on-v4.x labels Dec 5, 2016

thefourtheye mentioned this pull request Feb 5, 2017

Backport 9815 & 10161 to v6.x #11176

Closed

3 tasks

jasnell mentioned this pull request Apr 4, 2017

8.0.0 Release Proposal #12220

Closed

Trott deleted the revert9492 branch January 13, 2022 22:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revert "buffer: convert offset & length to int properly" #9814

Revert "buffer: convert offset & length to int properly" #9814

Trott commented Nov 26, 2016

Trott commented Nov 26, 2016

Trott commented Nov 26, 2016

Trott commented Nov 26, 2016

cjihrig commented Nov 26, 2016

ChALkeR commented Nov 26, 2016

Trott commented Nov 26, 2016

brodycj commented Nov 26, 2016

MylesBorins commented Nov 27, 2016

thefourtheye commented Nov 27, 2016

Revert "buffer: convert offset & length to int properly" #9814

Revert "buffer: convert offset & length to int properly" #9814

Conversation

Trott commented Nov 26, 2016

Checklist

Affected core subsystem(s)

Description of change

Trott commented Nov 26, 2016

Trott commented Nov 26, 2016

Trott commented Nov 26, 2016

cjihrig commented Nov 26, 2016

ChALkeR commented Nov 26, 2016

Trott commented Nov 26, 2016

brodycj commented Nov 26, 2016

MylesBorins commented Nov 27, 2016

thefourtheye commented Nov 27, 2016