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

[v10.x] buffer: fix crash for invalid index types #23795

Closed
wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

2555cb4 introduced a crash
when a non-number value was passed to ParseArrayIndex().

We do not always have JS typechecking for that in place, though.
This returns back to the previous behavior of defaulting to 0,
which is certainly questionable.

Refs: #22129
Fixes: #23668

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

@addaleax addaleax added regression Issues related to regressions. lts-watch-v10.x labels Oct 21, 2018
@nodejs-github-bot nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. labels Oct 21, 2018
@addaleax
Copy link
Member Author

refack
refack previously requested changes Oct 21, 2018
Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

IMHO this should be only for node10. Masking errors is bad practice, with possible security implications.
Let 11 crash until we implement a clearer error message.

@addaleax addaleax added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Oct 21, 2018
@addaleax
Copy link
Member Author

@nodejs/tsc PTAL … if there are objections standing on Wednesday I’ll call for a vote.

@mcollina
Copy link
Member

I don’t think this is a good one. I would prefer a PR that throwed a JS error instead of crashing. Why is that approach unfeasible?

@addaleax
Copy link
Member Author

addaleax commented Oct 21, 2018

@mcollina I was going for the existing behavior on v10.9.0 and below to avoid a breaking change… I’d be happy to implement that as a semver-major follow up, or, if the TSC thinks it could land on v10.x and v11.x, we could make an exception and add typechecking in a patch release?

Edit: I’ve also decided to not go for the breaking change because the fact that there’s an issue on this demonstrates that this does affect in-the-wild code. I fully agree with throwing, but I’d prefer to keep breaking changes semver-major…

@mcollina
Copy link
Member

+1 as a follow-up semver-major.

Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

LGTM since it just restores the previous behavior. A proper (semver-major?) fix would be nice but we don't have to fix it here.

Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

LGTM since it just restores the previous behavior. A proper (semver-major?) fix would be nice but we don't have to fix it here - or we can go with reverting 2555cb4 but I don't think it's necessary with this change.

Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

LGTM since it just restores the previous behavior. A proper (semver-major?) fix would be nice but we don't have to fix it here - or we can go with reverting 2555cb4 but I don't think it's necessary with this change.

Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

LGTM since it just restores the previous behavior. A proper (semver-major?) fix would be nice but we don't have to fix it here - or we can go with reverting 2555cb4 but I don't think it's necessary with this change.

Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

LGTM since it just restores the previous behavior. A proper (semver-major?) fix would be nice but we don't have to fix it here - or we can go with reverting 2555cb4 but I don't think it's necessary with this change.

@joyeecheung
Copy link
Member

(Oops, multiple reviews submitted during the GitHub outage)

Copy link
Contributor

@ryzokuken ryzokuken left a comment

Choose a reason for hiding this comment

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

+1 to backporting this, fixing the bug. Let me know what behavior everyone agrees with for the actual semver-major fix and I'll gladly open a PR.

@refack
Copy link
Contributor

refack commented Oct 23, 2018

Edit: I’ve also decided to not go for the breaking change because the fact that there’s an issue on this demonstrates that this does affect in-the-wild code. I fully agree with throwing, but I’d prefer to keep breaking changes semver-major…

BTW the bug (#23668) was opened for v11.0.0-nightly20181007061e09891c.

The actual bug (wyhaya/updns#7) is in https://github.com/wyhaya/updns/blob/4fb62c6fa6863089c8443d737ad1de61031280af/lib/parse.js#L125-L126

    question.qtype.copy(buf, 12 + qname.length, question.qtype, 2)
    question.qclass.copy(buf, 12 + qname.length + 2, question.qclass, 2)

third argument is the buffer, and not a bufferStart number

This module has been failing silently since the module was published, possibly leaking memory content.

So IIUC we have a crash instead of a silent fail in the node10 line, and that is going like that into LTS?
Also node11 just went out with this behaviour.
So IMHO there's an argument to be made that turning a crash into an exception is semver-patch.

@refack
Copy link
Contributor

refack commented Oct 23, 2018

This returns back to the previous behavior of defaulting to 0,
which is certainly questionable.

P.S. The previous behavior is undefined, it's not default to 0.
e.g. on node 10.9.0

> const b = Buffer.allocUnsafe(1024);
> const c = Buffer.allocUnsafe(512);
> b.copy(c, 'not a valid offset');
512
> b.copy(c, '4');
508
> const d = Buffer.alloc(1)
> d[0]='8'.charCodeAt(0)
> b.copy(c, d);
504

So this PR is semver-major

@refack refack added the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 23, 2018
@refack
Copy link
Contributor

refack commented Oct 24, 2018

Summery:

buf.copy(target[, targetStart[, sourceStart[, sourceEnd]]])

Added in: v0.1.90

  • target | A Buffer or Uint8Array to copy into.
  • targetStart The offset within target at which to begin writing. Default: 0.
  • sourceStart The offset within buf from which to begin copying. Default: 0.
  • sourceEnd The offset within buf at which to stop copying (not inclusive). Default: buf.length.
  • Returns: The number of bytes copied.

node <= 10.9.0:

> const b = Buffer.allocUnsafe(1024);
undefined
> const c = Buffer.allocUnsafe(512);
undefined
> b.copy(c, 8)
504
> b.copy(c, 8, 'gaga')
504
> b.copy(c, '8', '600')
424
> b.copy(c, 'gaga', 600)
424
> b.copy(c, '112', 600)
400

node >= 10.10.0 || 11.0.0

> const b = Buffer.allocUnsafe(1024);
undefined
> const c = Buffer.allocUnsafe(512);
undefined
> b.copy(c, 8)
504
> b.copy(c, 8, 'gaga')
# node11.0.0.exe  [27956]: src\node_buffer.cc:173: Assertion `arg->IsNumber()' failed.

i.e. Crash

this patch

> const b = Buffer.allocUnsafe(1024);
undefined
> const c = Buffer.allocUnsafe(512);
undefined
> b.copy(c, 8)
504
> b.copy(c, 'gaga')
512
> b.copy(c, '8')
512
> b.copy(c, 'gaga', 600)
424
> b.copy(c, '112', 600)
424

my suggestion

> const b = Buffer.allocUnsafe(1024);
undefined
> const c = Buffer.allocUnsafe(512);
undefined
> b.copy(c, 8)
504
> b.copy(c, 'gaga')
RangeError: Index out of range
    at Buffer.copy (buffer.js:637:12)
> b.copy(c, '8')
RangeError: Index out of range
    at Buffer.copy (buffer.js:637:12)
> b.copy(c, 'gaga', 600)
RangeError: Index out of range
    at Buffer.copy (buffer.js:637:12)
> b.copy(c, '112', 600)
RangeError: Index out of range
    at Buffer.copy (buffer.js:637:12)

@refack refack mentioned this pull request Oct 24, 2018
3 tasks
@Trott
Copy link
Member

Trott commented Oct 24, 2018

After talking a bunch with @refack and doing a minimal bit of tinkering, it does seem like this does not quite restore the previous behavior. This might duplicate comments above by @refack, but this is a small behavior change:

Node.js 10.9.0 (which is the version with the behavior we wish to restore):

> var b = Buffer.allocUnsafe(1024)
undefined
> var c = Buffer.allocUnsafe(512)
undefined
> b.copy(c, '112', 600)
400
> b.copy(c, 112, 600)
400
>

With this patch:

$ ./node
> var b = Buffer.allocUnsafe(1024)
undefined
> var c = Buffer.allocUnsafe(512)
undefined
> b.copy(c, '112', 600)
424
> b.copy(c, 112, 600)
400
> 

Can we patch that up? Or is that not desirable for a reason I don't see?

@addaleax
Copy link
Member Author

addaleax commented Oct 25, 2018

I’ve re-targeted this PR to v10.x, under the assumption that we can do something like #23840 a) for all affected methods and b) without significant performance impact.

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

@addaleax addaleax changed the base branch from master to v10.x-staging October 25, 2018 14:38
@refack refack dismissed their stale review October 25, 2018 14:44

IMHO this is a valid option for 10.x

@jasnell
Copy link
Member

jasnell commented Oct 25, 2018

@nodejs/tsc ... We have two competing PRs addressing the same issue. We need a resolution. This PR is one option, #23875 is the other.

@MylesBorins
Copy link
Contributor

I personally defer to the review of folks that know this code but would like to see something land ASAP so we can test 10.13.0 before pushing it out

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@ryzokuken
Copy link
Contributor

@MylesBorins for the record (as the person who originally made the change that broke things, sorry for that) I'd prefer this one over #23875 as long as it uses V8 functions that'll be deprecated soon.

@richardlau
Copy link
Member

With regards to v10.x:

https://github.com/nodejs/node/blob/f14274714d00bb5b58482bef15e84cdda9237910/COLLABORATOR_GUIDE.md#when-breaking-changes-actually-break-things

  • If a breaking commit does accidentally land in a Current or LTS branch, an attempt to fix the issue will be made before the next release; If no fix is provided then the commit will be reverted.

2555cb4 is the breaking commit that landed in v10.x (v10.10.0, although the changelog has this as c637d41b9d).

#23795 is an attempt to fix the issue while #23875 is a revert.

#23795 has a number of approvals, so IMHO can land as a fix.

The alternative is to revert (#23875) and then immediately re-backport #22129 including the changes from #23795.

@addaleax
Copy link
Member Author

@MylesBorins @richardlau I think all we need at this point is a green CI.

We could either rebase out the commit that broke the bootstrap module count for Workers CI, or land #23876 as soon as we can.

@Trott
Copy link
Member

Trott commented Oct 25, 2018

@MylesBorins
Copy link
Contributor

Another attempt at CI: https://ci.nodejs.org/job/node-test-pull-request/18166/

@Trott
Copy link
Member

Trott commented Oct 26, 2018

2555cb4 introduced a crash
when a non-number value was passed to `ParseArrayIndex()`.

We do not always have JS typechecking for that in place, though.
This returns back to the previous behavior of coercing values
to integers, which is certainly questionable.

Refs: nodejs#22129
Fixes: nodejs#23668
@addaleax
Copy link
Member Author

I’ve removed #23625 from v10.x-staging.

CI should be good now: https://ci.nodejs.org/job/node-test-pull-request/18181/

@addaleax
Copy link
Member Author

@MylesBorins
Copy link
Contributor

@refack
Copy link
Contributor

refack commented Oct 29, 2018

I've reimplemented the workaround for #22006
https://ci.nodejs.org/job/node-test-pull-request/18213/
The better solution is to backport #23733

@refack refack added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 29, 2018
@MylesBorins
Copy link
Contributor

landed in the v10.13.0-proposal branch in 2ba6010

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. regression Issues related to regressions.
Projects
None yet
Development

Successfully merging this pull request may close these issues.