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: fix range checks for slice() #9174

Merged
merged 1 commit into from Oct 20, 2016

Conversation

Projects
None yet
7 participants
@trevnorris
Contributor

trevnorris commented Oct 18, 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

Using the black magic of Symbol.toPrimitive the numeric value of
start/end can be changed when Uint32Value() is called once
Buffer::Fill() is entered. Allowing the CHECK() to be bypassed.

The bug report was only for "start", but the same can be done with
"end". Perform checks for both in node::Buffer::Fill() to make sure the
issue can't be triggered, even if process.binding is used directly.

Include tests for each case. Along with a check to make sure the last
time the value is accessed returns -1. This should be enough to make
sure Buffer::Fill() is receiving the correct value. Along with two tests
against process.binding directly.

Fixes: #9149

R=@nodejs/buffer

@trevnorris trevnorris added the buffer label Oct 18, 2016

@jasnell

LGTM with green CI

@jasnell jasnell added this to the 7.0.0 milestone Oct 18, 2016

+ const start = {
+ [Symbol.toPrimitive]() {
+ // We use this condition to get around the check in lib/buffer.js
+ if (ctr <= 0) {

This comment has been minimized.

@mscdex

mscdex Oct 19, 2016

Contributor

Minor nit: this can be just ctr === 0.

@mscdex

mscdex Oct 19, 2016

Contributor

Minor nit: this can be just ctr === 0.

This comment has been minimized.

@trevnorris

trevnorris Oct 19, 2016

Contributor

no. in case the internals change then cntr may be less than 0. if that's the case then the value needs to be updated. which is why i have the followup check below of

assert.ok(elseWasLast,
          'internal API changed, -1 no longer in correct location');
@trevnorris

trevnorris Oct 19, 2016

Contributor

no. in case the internals change then cntr may be less than 0. if that's the case then the value needs to be updated. which is why i have the followup check below of

assert.ok(elseWasLast,
          'internal API changed, -1 no longer in correct location');
@mscdex

This comment has been minimized.

Show comment
Hide comment
@deian

This comment has been minimized.

Show comment
Hide comment
@deian

deian Oct 19, 2016

Member

Here is another test that this fixes that would otherwise have triggered the CHECK:

const buf = new Buffer('w00t');

Object.defineProperty(buf, 'length', {
  value: 1337,
  enumerable: true
});
buf.fill('');
Member

deian commented Oct 19, 2016

Here is another test that this fixes that would otherwise have triggered the CHECK:

const buf = new Buffer('w00t');

Object.defineProperty(buf, 'length', {
  value: 1337,
  enumerable: true
});
buf.fill('');
@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Oct 19, 2016

Contributor

+1 to adding more tests!

Contributor

mscdex commented Oct 19, 2016

+1 to adding more tests!

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Oct 20, 2016

Member

I'd like to get this landed today to be included in the v7.0.0 RC1. Can we add the additional tests in a separate PR?

Member

jasnell commented Oct 20, 2016

I'd like to get this landed today to be included in the v7.0.0 RC1. Can we add the additional tests in a separate PR?

buffer: fix range checks for slice()
Using the black magic of Symbol.toPrimitive the numeric value of
start/end can be changed when Uint32Value() is called once
Buffer::Fill() is entered. Allowing the CHECK() to be bypassed.

The bug report was only for "start", but the same can be done with
"end". Perform checks for both in node::Buffer::Fill() to make sure the
issue can't be triggered, even if process.binding is used directly.

Include tests for each case. Along with a check to make sure the last
time the value is accessed returns -1. This should be enough to make
sure Buffer::Fill() is receiving the correct value. Along with two tests
against process.binding directly.

Fixes: #9149
PR-URL: #9174
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <ranziska.hinkelmann@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

@trevnorris trevnorris merged commit 7bffe23 into nodejs:master Oct 20, 2016

jasnell added a commit that referenced this pull request Oct 20, 2016

buffer: fix range checks for slice()
Using the black magic of Symbol.toPrimitive the numeric value of
start/end can be changed when Uint32Value() is called once
Buffer::Fill() is entered. Allowing the CHECK() to be bypassed.

The bug report was only for "start", but the same can be done with
"end". Perform checks for both in node::Buffer::Fill() to make sure the
issue can't be triggered, even if process.binding is used directly.

Include tests for each case. Along with a check to make sure the last
time the value is accessed returns -1. This should be enough to make
sure Buffer::Fill() is receiving the correct value. Along with two tests
against process.binding directly.

Fixes: #9149
PR-URL: #9174
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <ranziska.hinkelmann@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

MylesBorins added a commit that referenced this pull request Dec 20, 2016

buffer: fix range checks for slice()
Using the black magic of Symbol.toPrimitive the numeric value of
start/end can be changed when Uint32Value() is called once
Buffer::Fill() is entered. Allowing the CHECK() to be bypassed.

The bug report was only for "start", but the same can be done with
"end". Perform checks for both in node::Buffer::Fill() to make sure the
issue can't be triggered, even if process.binding is used directly.

Include tests for each case. Along with a check to make sure the last
time the value is accessed returns -1. This should be enough to make
sure Buffer::Fill() is receiving the correct value. Along with two tests
against process.binding directly.

Fixes: #9149
PR-URL: #9174
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <ranziska.hinkelmann@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Dec 20, 2016

Member

@trevnorris I've gone ahead and landed this on v6.x. If you would like to see it on v4.x please submit a manual backport. Otherwise could you change the label to dont-land-on-v4.x

Member

MylesBorins commented Dec 20, 2016

@trevnorris I've gone ahead and landed this on v6.x. If you would like to see it on v4.x please submit a manual backport. Otherwise could you change the label to dont-land-on-v4.x

MylesBorins added a commit that referenced this pull request Dec 21, 2016

buffer: fix range checks for slice()
Using the black magic of Symbol.toPrimitive the numeric value of
start/end can be changed when Uint32Value() is called once
Buffer::Fill() is entered. Allowing the CHECK() to be bypassed.

The bug report was only for "start", but the same can be done with
"end". Perform checks for both in node::Buffer::Fill() to make sure the
issue can't be triggered, even if process.binding is used directly.

Include tests for each case. Along with a check to make sure the last
time the value is accessed returns -1. This should be enough to make
sure Buffer::Fill() is receiving the correct value. Along with two tests
against process.binding directly.

Fixes: #9149
PR-URL: #9174
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <ranziska.hinkelmann@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

@MylesBorins MylesBorins referenced this pull request Dec 21, 2016

Merged

V6.9.3 proposal #10394

@trevnorris trevnorris referenced this pull request Dec 22, 2016

Merged

async_wrap: clear destroy_ids vector #10400

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