buffer: validate arg types #23840

refack · 2018-10-24T02:11:36Z

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 · 2018-10-24T02:11:37Z

@refack build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1350/pipeline

mscdex · 2018-10-24T02:24:02Z

lib/buffer.js

-  };
+function validateOffsetType(val, name) {
+  if (val) {
+    if ((typeof val !== 'number')) {


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

refack · 2018-10-24T02:31:08Z

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

mscdex · 2018-10-24T02:39:58Z

lib/buffer.js

+function validateOffsetType(val, name) {
+  if (val) {
+    if (typeof val !== 'number') {
+      throw new ERR_INVALID_ARG_TYPE(


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

refack · 2018-10-24T02:54:43Z

New CI: https://ci.nodejs.org/job/node-test-commit/22591/

Trott · 2018-10-24T03:04:34Z

lib/buffer.js

+Buffer.prototype.copy = function(target, targetStart, sourceStart, sourceEnd) {
+  if (!isUint8Array(target)) {
+    throw new ERR_INVALID_ARG_TYPE(
+      'otherBuffer', ['Buffer', 'Uint8Array'], target);


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

Trott · 2018-10-24T03:05:18Z

test/parallel/test-buffer-alloc.js

@@ -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 ' +


Trott · 2018-10-24T03:09:27Z

test/parallel/test-buffer-copy.js

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


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

(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...)

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);

Trott

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

addaleax · 2018-10-24T05:51:42Z

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 · 2018-10-24T05:52:21Z

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

refack · 2018-10-24T12:36:04Z

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

/CC @nodejs/release.

Trott · 2018-10-24T13:43:26Z

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 · 2018-10-24T14:07:41Z

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).

BridgeAR · 2018-10-24T18:04:19Z

lib/buffer.js

-    return _copy(this, target, targetStart, sourceStart, sourceEnd);
-  };
+function validateOffsetType(val, name) {
+  if (val) {


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) {

BridgeAR · 2018-10-24T18:13:01Z

lib/buffer.js

+function validateOffsetType(val, name) {
+  if (val) {
+    if (typeof val !== 'number') {
+      throw new ERR_INVALID_ARG_TYPE(name, ['Number'], val);


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

BridgeAR · 2018-10-24T18:28:00Z

lib/buffer.js

+    val = 0;
+  }
+  return val;
+}


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.

refack · 2018-10-25T15:49:55Z

@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

addaleax · 2018-10-25T15:56:08Z

@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 · 2018-10-25T16:06:25Z

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 · 2018-10-25T17:12:06Z

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

refack · 2018-10-25T17:20:35Z

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 · 2018-11-06T05:16:14Z

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

nodejs-github-bot added the buffer Issues and PRs related to the buffer subsystem. label Oct 24, 2018

refack force-pushed the validate-buffer-params branch from 63eb77c to abfbdfe Compare October 24, 2018 02:14

refack added lts-watch-v10.x regression Issues related to regressions. labels Oct 24, 2018

mscdex reviewed Oct 24, 2018

View reviewed changes

refack self-assigned this Oct 24, 2018

Trott reviewed Oct 24, 2018

View reviewed changes

Trott approved these changes Oct 24, 2018

View reviewed changes

addaleax removed the lts-watch-v10.x label Oct 24, 2018

addaleax mentioned this pull request Oct 24, 2018

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

Closed

3 tasks

targos added dont-land-on-v6.x labels Oct 24, 2018

addaleax added the wip Issues and PRs that are still a work in progress. label Oct 24, 2018

BridgeAR reviewed Oct 24, 2018

View reviewed changes

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 added 2 commits October 25, 2018 11:23

buffer: validate args to copy

aeaa698

buffer: validate args to compare

3fc87b4

refack force-pushed the validate-buffer-params branch from 2cdedd1 to 3fc87b4 Compare October 25, 2018 15:24

fixup! use validateUint32

e146b86

refack mentioned this pull request Oct 25, 2018

buffer: validate arg for compare #23878

Closed

3 tasks

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

refack mentioned this pull request Nov 14, 2018

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

Closed

addaleax mentioned this pull request Dec 20, 2018

buffer: fix crash for invalid index types #25154

Closed

3 tasks

refack closed this Dec 21, 2018

refack deleted the validate-buffer-params branch December 21, 2018 01:14

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 restored the validate-buffer-params branch January 22, 2019 21:42

refack removed their assignment Mar 11, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

buffer: validate arg types #23840

buffer: validate arg types #23840

refack commented Oct 24, 2018 •

edited

Loading

nodejs-github-bot commented Oct 24, 2018

mscdex Oct 24, 2018

refack Oct 24, 2018

refack commented Oct 24, 2018

mscdex Oct 24, 2018

refack Oct 24, 2018

refack commented Oct 24, 2018

Trott Oct 24, 2018 •

edited

Loading

refack Oct 24, 2018

Trott Oct 24, 2018

refack Oct 24, 2018

Trott Oct 24, 2018

Trott Oct 24, 2018

refack Oct 24, 2018

Trott left a comment

addaleax commented Oct 24, 2018

addaleax commented Oct 24, 2018

refack commented Oct 24, 2018

Trott commented Oct 24, 2018 •

edited

Loading

targos commented Oct 24, 2018

BridgeAR Oct 24, 2018

BridgeAR Oct 24, 2018

BridgeAR Oct 24, 2018

refack Oct 25, 2018

refack commented Oct 25, 2018 •

edited

Loading

addaleax commented Oct 25, 2018

refack commented Oct 25, 2018

bmeurer commented Oct 25, 2018

refack commented Oct 25, 2018

Trott commented Nov 6, 2018

	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);

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

buffer: validate arg types #23840

buffer: validate arg types #23840

Conversation

refack commented Oct 24, 2018 • edited Loading

Checklist

nodejs-github-bot commented Oct 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented Oct 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented Oct 24, 2018

Trott Oct 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott left a comment

Choose a reason for hiding this comment

addaleax commented Oct 24, 2018

addaleax commented Oct 24, 2018

refack commented Oct 24, 2018

Trott commented Oct 24, 2018 • edited Loading

targos commented Oct 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented Oct 25, 2018 • edited Loading

addaleax commented Oct 25, 2018

refack commented Oct 25, 2018

bmeurer commented Oct 25, 2018

refack commented Oct 25, 2018

Trott commented Nov 6, 2018

refack commented Oct 24, 2018 •

edited

Loading

Trott Oct 24, 2018 •

edited

Loading

Trott commented Oct 24, 2018 •

edited

Loading

refack commented Oct 25, 2018 •

edited

Loading