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: validate arg types #23840

Closed
wants to merge 3 commits into from
Closed

Conversation

refack
Copy link
Contributor

@refack refack commented Oct 24, 2018

Validate buffer.copy arguments type and range.
Since this fixes a crash in node11, it might be considered semver-patch.

Alternative to: #23795
Fixes: #23668

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

@nodejs-github-bot nodejs-github-bot added the buffer Issues and PRs related to the buffer subsystem. label Oct 24, 2018
@refack refack added lts-watch-v10.x regression Issues related to regressions. labels Oct 24, 2018
lib/buffer.js Outdated
};
function validateOffsetType(val, name) {
if (val) {
if ((typeof val !== 'number')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the extra set of parentheses can be dropped here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack

@refack
Copy link
Contributor Author

refack commented Oct 24, 2018

lib/buffer.js Outdated
function validateOffsetType(val, name) {
if (val) {
if (typeof val !== 'number') {
throw new ERR_INVALID_ARG_TYPE(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can be all on one line as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack

@refack
Copy link
Contributor Author

refack commented Oct 24, 2018

@refack refack self-assigned this Oct 24, 2018
lib/buffer.js Outdated
Buffer.prototype.copy = function(target, targetStart, sourceStart, sourceEnd) {
if (!isUint8Array(target)) {
throw new ERR_INVALID_ARG_TYPE(
'otherBuffer', ['Buffer', 'Uint8Array'], target);
Copy link
Member

@Trott Trott Oct 24, 2018

Choose a reason for hiding this comment

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

I believe it should be 'target' to match documentation and stack traces, and not 'otherBuffer'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack

@@ -964,7 +964,8 @@ common.expectsError(
{
code: 'ERR_INVALID_ARG_TYPE',
type: TypeError,
message: 'argument must be a buffer'
message: 'The "otherBuffer" argument must be one of type Buffer or ' +
Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack

code: 'ERR_INVALID_ARG_TYPE',
type: TypeError,
message: 'The "targetStart" argument must be of type ' +
'Number. Received type string'
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason for the inconsistent capitalization between 'string' and 'Number'?

Copy link
Member

Choose a reason for hiding this comment

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

(that above is a super-minor non-blocking nit, probably applies to lots of other errors in our code base, can be cleaned up later, but if you're feeling in a perfectionist mood...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's the dark magic of ERR_INVALID_ARG_TYPE('targetStart', ['Number'], targetStart). I'm assuming it does .name to the types, and typeof val to the val...

node/lib/internal/errors.js

Lines 674 to 699 in acb363f

E('ERR_INVALID_ARG_TYPE',
(name, expected, actual) => {
assert(typeof name === 'string', "'name' must be a string");
// determiner: 'must be' or 'must not be'
let determiner;
if (typeof expected === 'string' && expected.startsWith('not ')) {
determiner = 'must not be';
expected = expected.replace(/^not /, '');
} else {
determiner = 'must be';
}
let msg;
if (name.endsWith(' argument')) {
// For cases like 'first argument'
msg = `The ${name} ${determiner} ${oneOf(expected, 'type')}`;
} else {
const type = name.includes('.') ? 'property' : 'argument';
msg = `The "${name}" ${type} ${determiner} ${oneOf(expected, 'type')}`;
}
// TODO(BridgeAR): Improve the output by showing `null` and similar.
msg += `. Received type ${typeof actual}`;
return msg;
}, TypeError);

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM for a fix for 11.x. Less sure about backporting to 10.x, but that can be to-be-determined.

@addaleax
Copy link
Member

This does not just affect buffer.copy(), it affects all methods using ParseArrayIndex(), directly or indirectly – please address that or remove the Fixes: tag.

@addaleax
Copy link
Member

(And, 👍 for considering this semver-patch for v11.x, 👎 to landing this on v10.x, ever, where it would still be semver-major.)

@refack
Copy link
Contributor Author

refack commented Oct 24, 2018

to landing this on v10.x, ever, where it would still be semver-major

/CC @nodejs/release.

@Trott
Copy link
Member

Trott commented Oct 24, 2018

to landing this on v10.x, ever, where it would still be semver-major

/CC @nodejs/release.

So that the release folks don't have to educate themselves on the entire history here in other issues etc.:

Through Node.js 10.9.0:

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

After 10.9.0, the b.copy() call above crashed.

With this patch, it throws.

I believe what @refack is saying is that the move from crash to throws is semver patch so can be landed/backported to the 10.x line.

I believe what @addaleax is saying (and I'm inclined to agree) is that the crash was a regression and moving back to the behavior in 10.9.0 is the change that would be considered semver patch and not semver major. Moving on to throwing an error is a breaking change.

So I believe that's what @refack is soliciting feedback on.

FWIW, in 11.0.0, it crashes, so changing that to a throw in a subsequent 11.x release is semver patch. I'm mentioning it because it would not receive a semver major callout in the change log for 11.0.0, so a case could be made that it's not even semver patch there. (I disagree with that. If something never worked in a 11.x, then it's not a breaking change to fix it differently than in previous release lines. But others might feel differently or there might be a policy somewhere?)

@targos
Copy link
Member

targos commented Oct 24, 2018

Since the previous behaviour was not completely broken (as the example with the number being passed as a string shows), I think we should revert it on v10.x (ideally before it transitions to LTS).
I'm +1 on throwing in the next v11.x (this PR).

@addaleax addaleax added the wip Issues and PRs that are still a work in progress. label Oct 24, 2018
lib/buffer.js Outdated
return _copy(this, target, targetStart, sourceStart, sourceEnd);
};
function validateOffsetType(val, name) {
if (val) {
Copy link
Member

Choose a reason for hiding this comment

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

We should only use a default in case it's definitely not set. So I would write it as:

Suggested change
if (val) {
if (val != null) {

lib/buffer.js Outdated
function validateOffsetType(val, name) {
if (val) {
if (typeof val !== 'number') {
throw new ERR_INVALID_ARG_TYPE(name, ['Number'], val);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
throw new ERR_INVALID_ARG_TYPE(name, ['Number'], val);
throw new ERR_INVALID_ARG_TYPE(name, 'number', val);

This resolves https://github.com/nodejs/node/pull/23840/files#r227630142

lib/buffer.js Outdated
val = 0;
}
return val;
}
Copy link
Member

Choose a reason for hiding this comment

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

I suggest to replace this whole function with the validateUint32() function that we already have in the validators file. It will be a stricter check by also verifying that the input is an integer in the correct range.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@BridgeAR BridgeAR added semver-major PRs that contain breaking changes and should be released in the next major version. dont-land-on-v10.x and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels Oct 25, 2018
@refack refack mentioned this pull request Oct 25, 2018
3 tasks
@refack
Copy link
Contributor Author

refack commented Oct 25, 2018

@addaleax AFAICT buf.copy is the only public API that does not validate or explicitly corerses it's args (please correct me if I'm wrong).

I've made #23878 (semver-major) to make buf.compare be inline with the docs and buf.copy

@refack refack removed the wip Issues and PRs that are still a work in progress. label Oct 25, 2018
@addaleax
Copy link
Member

@refack So … there’s also methods like buffer.utf8Write() or buffer.utf8Slice(), which are not technically public API, but listed as methods on the prototype and everything. And suggestions for what to do for those?

I think the main options would be wrapping them (which might create some overhead we don’t want?), doing the typechecks in C++ in those methods, or incorporating something like #23795 into this PR. I’m fine with any of them, but I’d like us to do something, even if they are not technically public…

@refack
Copy link
Contributor Author

refack commented Oct 25, 2018

I think the main options would be wrapping them (which might create some overhead we don’t want?), doing the typechecks in C++ in those methods, or incorporating something like #23795 into this PR. I’m fine with any of them, but I’d like us to do something, even if they are not technically public…

Ok.
I'm thinking about this...
If anyone has a preference, advice would be much appreciated.
(I think the trend was to move typechecks to JS... but in this case it will require js wrappers...)
@bmeurer a priori what's your intuition?

@bmeurer
Copy link
Member

bmeurer commented Oct 25, 2018

@refack I guess I'd try to stay in JS if possible.

@refack
Copy link
Contributor Author

refack commented Oct 25, 2018

I have an idea. Rename the unwrapped methods, and expose new wrapped methods with the old names. This way the documented API doesn't regress.

@Trott
Copy link
Member

Trott commented Nov 6, 2018

@refack This is being worked on? (Should we add an in progress label?)

@refack refack closed this Dec 21, 2018
@refack refack deleted the validate-buffer-params branch December 21, 2018 01:14
@addaleax addaleax added semver-major PRs that contain breaking changes and should be released in the next major version. and removed dont-land-on-v10.x labels Dec 31, 2018
@refack refack restored the validate-buffer-params branch January 22, 2019 21:42
@refack refack removed their assignment Mar 11, 2019
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. regression Issues related to regressions. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

src\node_buffer.cc:173: Assertion `arg->IsNumber()' failed.
8 participants