Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

buffer: convert offset & length to int properly #9492

Closed

Conversation

@thefourtheye
Copy link
Contributor

thefourtheye commented Nov 6, 2016

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


cc @nodejs/buffer

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.
@@ -0,0 +1,19 @@
// Flags: --expose-internals

This comment has been minimized.

Copy link
@cjihrig

cjihrig Nov 8, 2016

Contributor

This should probably test for a wider range of values. test/parallel/test-net-internal.js has some good examples.

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Nov 12, 2016

Author Contributor

Done!

@thefourtheye
Copy link
Contributor Author

thefourtheye commented Nov 12, 2016

Ping!

Copy link
Contributor

cjihrig left a comment

LGTM, but someone from @nodejs/buffer should sign off.

@thefourtheye
Copy link
Contributor Author

thefourtheye commented Nov 17, 2016

Bump!

Copy link
Member

jasnell left a comment

Small nit in the tests but otherwise LGTM

uint8Array[i] = 1;
}

const buffer = new Buffer(arrayBuffer, offset, length);

This comment has been minimized.

Copy link
@jasnell

jasnell Nov 18, 2016

Member

s/new Buffer/Buffer.from

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Nov 20, 2016

Author Contributor

@jasnell Done!


/*
* Implementation of ToLength as per ECMAScript Specification
* Refer: http://www.ecma-international.org/ecma-262/6.0/#sec-tolength

This comment has been minimized.

Copy link
@trevnorris

trevnorris Nov 23, 2016

Contributor

I'm curious why the step If len is +∞, return 253-1. is in there when the min() operation makes it seem unnecessary. I can't see any difference if that condition is left out.

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Nov 23, 2016

Author Contributor

True. In our implementation I omitted that, as min itself is sufficient.

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Nov 23, 2016

Author Contributor

@trevnorris I raised a PR tc39/ecma262#738. Let's see what tc39 people feel.

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Nov 23, 2016

Author Contributor

@trevnorris And it got merged :-)

This comment has been minimized.

Copy link
@trevnorris

trevnorris Nov 25, 2016

Contributor

@thefourtheye excellent!

@thefourtheye
Copy link
Contributor Author

thefourtheye commented Nov 26, 2016

Landed in ca37fa5

@thefourtheye thefourtheye deleted the thefourtheye:buffer-from-array-buffer branch Nov 26, 2016
thefourtheye added a commit that referenced this pull request Nov 26, 2016
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.

PR-URL: #9492

Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@Trott
Copy link
Member

Trott commented Nov 26, 2016

It looks like this landed without being run through CI. This is a guess right now, so I could be wrong, but it appears this is the likely culprit in having broken CI. See https://ci.nodejs.org/job/node-test-commit-arm/6094/nodes=armv7-ubuntu1404/console and https://ci.nodejs.org/job/node-test-commit-linux/6250/nodes=centos5-32/console and a dozen or two others:

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)
@Trott
Copy link
Member

Trott commented Nov 26, 2016

Update: Oh, yeah, that test was added in this PR so yeah, all but certainly this change is the problem. Is there a quick fix? Or should we revert and take whatever time is needed to fix it?

@Trott Trott mentioned this pull request Nov 26, 2016
1 of 1 task complete
@Trott
Copy link
Member

Trott commented Nov 26, 2016

Opened #9814 for a quick revert. Can always re-do the change after it's fixed. PTAL over there...

Trott added a commit to Trott/io.js that referenced this pull request Nov 26, 2016
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
Copy link
Member

Trott commented Nov 26, 2016

Revert for this landed in 6b2aa1a. Hopefully the fix is easy enough and this can go back in soon. /cc @thefourtheye

@brodybits
Copy link
Contributor

brodybits commented Nov 26, 2016

Also test\parallel\test-buffer-creation-regression.js with this change gave me a RangeError exception on Windows.

Path: parallel/test-buffer-creation-regression                                                                          
C:\Users\Chris\Documents\work\node\test\parallel\test-buffer-creation-regression.js:7
  const arrayBuffer = new ArrayBuffer(size);
                      ^

RangeError: Invalid array buffer length
    at new ArrayBuffer (<anonymous>)
    at test (C:\Users\Chris\Documents\work\node\test\parallel\test-buffer-creation-regression.js:7:23)
    at Object.<anonymous> (C:\Users\Chris\Documents\work\node\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)
Command: C:\Users\Chris\Documents\work\node\Release\node.exe C:\Users\Chris\Documents\work\node\test\parallel\test-buffer-creation-regression.js
thefourtheye added a commit to thefourtheye/io.js that referenced this pull request Nov 27, 2016
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 thefourtheye mentioned this pull request Nov 27, 2016
3 of 3 tasks complete
thefourtheye added a commit that referenced this pull request Dec 5, 2016
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>
Fishrock123 added a commit that referenced this pull request Dec 5, 2016
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>
jmdarling added a commit to jmdarling/node that referenced this pull request Dec 8, 2016
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>
italoacasas added a commit to italoacasas/node that referenced this pull request Jan 18, 2017
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.

PR-URL: nodejs#9492

Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
italoacasas added a commit to italoacasas/node that referenced this pull request Jan 18, 2017
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>
@thefourtheye thefourtheye mentioned this pull request Feb 5, 2017
3 of 3 tasks complete
jasnell added a commit that referenced this pull request Mar 3, 2017
  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>
MylesBorins added a commit that referenced this pull request Mar 9, 2017
  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>
@jasnell jasnell mentioned this pull request Apr 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.