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 #9815

Closed
wants to merge 3 commits into from

Conversation

thefourtheye
Copy link
Contributor

@thefourtheye thefourtheye commented Nov 27, 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 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
@nodejs-github-bot 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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@thefourtheye
Copy link
Contributor Author

@thefourtheye
Copy link
Contributor Author

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

Copy link
Member

@targos targos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@thefourtheye
Copy link
Contributor Author

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

@mcollina
Copy link
Member

mcollina commented Dec 2, 2016

LGTM

@thefourtheye
Copy link
Contributor Author

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

@thefourtheye
Copy link
Contributor Author

Landed in 720d01f

@thefourtheye thefourtheye deleted the redo-9492 branch December 5, 2016 12:47
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 <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Fishrock123 pushed 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 <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@Fishrock123 Fishrock123 mentioned this pull request Dec 5, 2016
2 tasks
@Trott
Copy link
Member

Trott commented Dec 7, 2016

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
Copy link
Member

Trott commented Dec 7, 2016

(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
Copy link
Member

Trott commented Dec 7, 2016

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

jmdarling pushed 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 <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@MylesBorins
Copy link
Contributor

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

@thefourtheye
Copy link
Contributor Author

@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
Copy link
Contributor

@thefourtheye please do

@trevnorris
Copy link
Contributor

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

@trevnorris
Copy link
Contributor

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

@MylesBorins
Copy link
Contributor

ping @thefourtheye

@thefourtheye thefourtheye mentioned this pull request Feb 5, 2017
3 tasks
@thefourtheye
Copy link
Contributor Author

@MylesBorins I backported this in #11176

@thefourtheye
Copy link
Contributor Author

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

jasnell pushed 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 <[email protected]>
  Reviewed-By: Trevor Norris <[email protected]>
  Reviewed-By: James M Snell <[email protected]>
  Reviewed-By: Colin Ihrig <[email protected]>
  Reviewed-By: Matteo Collina <[email protected]>

Backport-Of: #9815
PR-URL: #11176
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed 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 <[email protected]>
  Reviewed-By: Trevor Norris <[email protected]>
  Reviewed-By: James M Snell <[email protected]>
  Reviewed-By: Colin Ihrig <[email protected]>
  Reviewed-By: Matteo Collina <[email protected]>

Backport-Of: #9815
PR-URL: #11176
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants