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

Backport 9815 & 10161 to v6.x #11176

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
5 participants
@thefourtheye
Contributor

thefourtheye commented Feb 5, 2017

Backport of #9815 & #10161


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

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

buffer


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

thefourtheye and others added some commits Nov 27, 2016

buffer: convert offset & length to int properly
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>
test: move long-running test to sequential
test-buffer-creation-regression is flaky on some SmartOS hosts in CI,
timing out. Move to sequential so it does not compete with other tests
for resources. Reduce three test cases to just the one needed to
identify the regression.

PR-URL: #10161
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
@jasnell

jasnell approved these changes Feb 6, 2017

@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Feb 21, 2017

Member

@thefourtheye looks like failures in CI

Member

MylesBorins commented Feb 21, 2017

@thefourtheye looks like failures in CI

@thefourtheye

This comment has been minimized.

Show comment
Hide comment
@thefourtheye

thefourtheye Feb 22, 2017

Contributor

One more CI run, as the old jobs are not found now. https://ci.nodejs.org/job/node-test-pull-request/6538/

Contributor

thefourtheye commented Feb 22, 2017

One more CI run, as the old jobs are not found now. https://ci.nodejs.org/job/node-test-pull-request/6538/

@MylesBorins

This comment has been minimized.

Show comment
Hide comment

@MylesBorins MylesBorins closed this Mar 3, 2017

@MylesBorins MylesBorins reopened this Mar 3, 2017

jasnell added a commit that referenced this pull request Mar 3, 2017

buffer: convert offset & length to int properly
  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 added a commit that referenced this pull request Mar 3, 2017

test: move long-running test to sequential
  test-buffer-creation-regression is flaky on some SmartOS hosts in CI,
  timing out. Move to sequential so it does not compete with other tests
  for resources. Reduce three test cases to just the one needed to
  identify the regression.

  PR-URL: #10161
  Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
  Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
  Reviewed-By: Italo A. Casas <me@italoacasas.com>

Backport-Of: #10161
PR-URL: #11176
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell

This comment has been minimized.

Show comment
Hide comment
Member

jasnell commented Mar 3, 2017

Landed in 82a008b...4e90a13

@jasnell jasnell closed this Mar 3, 2017

MylesBorins added a commit that referenced this pull request Mar 9, 2017

buffer: convert offset & length to int properly
  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

test: move long-running test to sequential
  test-buffer-creation-regression is flaky on some SmartOS hosts in CI,
  timing out. Move to sequential so it does not compete with other tests
  for resources. Reduce three test cases to just the one needed to
  identify the regression.

  PR-URL: #10161
  Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
  Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
  Reviewed-By: Italo A. Casas <me@italoacasas.com>

Backport-Of: #10161
PR-URL: #11176
Reviewed-By: James M Snell <jasnell@gmail.com>

@MylesBorins MylesBorins referenced this pull request Mar 9, 2017

Merged

v6.10.1 proposal #11759

@thefourtheye thefourtheye deleted the thefourtheye:backport-9815-v6.x branch Mar 13, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment