1berry/nodejs
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

buffer: use stricter range checks

Browse Source 
This validates the input to make sure the arguments do not overflow.
Before, if the input would overflow, it would cause the write to be
performt in the wrong spot / result in unexpected behavior.
Instead, just use a strict number validation.

PR-URL: https://github.com/nodejs/node/pull/27045
Fixes: https://github.com/nodejs/node/issues/27043
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
This commit is contained in:
Ruben Bridgewater 2019-04-02 00:01:29 +02:00
parent 2fed83dee8
commit 693401d0dd
No known key found for this signature in database
GPG Key ID: F07496B3EB3C1762
7 changed files with 98 additions and 102 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

12
doc/api/errors.md
View File

@ -1564,12 +1564,6 @@ OpenSSL crypto support.
An attempt was made to use features that require [ICU][], but Node.js was not
compiled with ICU support.
<a id="ERR_NO_LONGER_SUPPORTED"></a>
### ERR_NO_LONGER_SUPPORTED
A Node.js API was called in an unsupported manner, such as
`Buffer.write(string, encoding, offset[, length])`.
<a id="ERR_OUT_OF_RANGE"></a>
### ERR_OUT_OF_RANGE
@ -2096,6 +2090,12 @@ removed: v10.0.0
Used by the `N-API` when `Constructor.prototype` is not an object.
<a id="ERR_NO_LONGER_SUPPORTED"></a>
### ERR_NO_LONGER_SUPPORTED
A Node.js API was called in an unsupported manner, such as
`Buffer.write(string, encoding, offset[, length])`.
<a id="ERR_OUTOFMEMORY"></a>
### ERR_OUTOFMEMORY
<!-- YAML

73
lib/buffer.js
View File

@ -65,17 +65,18 @@ const {
const {
codes: {
ERR_BUFFER_OUT_OF_BOUNDS,
ERR_OUT_OF_RANGE,
ERR_INVALID_ARG_TYPE,
ERR_INVALID_ARG_VALUE,
ERR_INVALID_BUFFER_SIZE,
ERR_INVALID_OPT_VALUE,
ERR_NO_LONGER_SUPPORTED,
ERR_UNKNOWN_ENCODING
},
hideStackFrames
} = require('internal/errors');
const { validateString } = require('internal/validators');
const {
validateInt32,
validateString
} = require('internal/validators');
const {
FastBuffer,
@ -445,7 +446,7 @@ Buffer.concat = function concat(list, length) {
}
}
} else {
length = length >>> 0;
validateInt32(length, 'length', 0);
}
const buffer = Buffer.allocUnsafe(length);
@ -700,35 +701,27 @@ Buffer.prototype.compare = function compare(target,
if (targetStart === undefined)
targetStart = 0;
else if (targetStart < 0)
throw new ERR_OUT_OF_RANGE('targetStart', '>= 0', targetStart);
else
targetStart >>>= 0;
validateInt32(targetStart, 'targetStart', 0);
if (targetEnd === undefined)
targetEnd = target.length;
else if (targetEnd > target.length)
throw new ERR_OUT_OF_RANGE('targetEnd', `<= ${target.length}`, targetEnd);
else
targetEnd >>>= 0;
validateInt32(targetEnd, 'targetEnd', 0, target.length);
if (sourceStart === undefined)
sourceStart = 0;
else if (sourceStart < 0)
throw new ERR_OUT_OF_RANGE('sourceStart', '>= 0', sourceStart);
else
sourceStart >>>= 0;
validateInt32(sourceStart, 'sourceStart', 0);
if (sourceEnd === undefined)
sourceEnd = this.length;
else if (sourceEnd > this.length)
throw new ERR_OUT_OF_RANGE('sourceEnd', `<= ${this.length}`, sourceEnd);
else
sourceEnd >>>= 0;
validateInt32(sourceEnd, 'sourceEnd', 0, this.length);
if (sourceStart >= sourceEnd)
return (targetStart >= targetEnd ? 0 : -1);
else if (targetStart >= targetEnd)
if (targetStart >= targetEnd)
return 1;
return compareOffset(this, target, targetStart, sourceStart, targetEnd,
@ -867,17 +860,13 @@ function _fill(buf, value, offset, end, encoding) {
offset = 0;
end = buf.length;
} else {
validateInt32(offset, 'offset', 0);
// Invalid ranges are not set to a default, so can range check early.
if (offset < 0)
throw new ERR_OUT_OF_RANGE('offset', '>= 0', offset);
if (end === undefined) {
end = buf.length;
} else {
if (end > buf.length || end < 0)
throw new ERR_OUT_OF_RANGE('end', `>= 0 and <= ${buf.length}`, end);
end = end >>> 0;
validateInt32(end, 'end', 0, buf.length);
}
offset = offset >>> 0;
if (offset >= end)
return buf;
}
@ -896,39 +885,37 @@ Buffer.prototype.write = function write(string, offset, length, encoding) {
// Buffer#write(string);
if (offset === undefined) {
return this.utf8Write(string, 0, this.length);
}
// Buffer#write(string, encoding)
} else if (length === undefined && typeof offset === 'string') {
if (length === undefined && typeof offset === 'string') {
encoding = offset;
length = this.length;
offset = 0;
// Buffer#write(string, offset[, length][, encoding])
} else if (isFinite(offset)) {
offset = offset >>> 0;
if (isFinite(length)) {
length = length >>> 0;
} else {
if (offset === undefined) {
offset = 0;
} else {
encoding = length;
length = undefined;
validateInt32(offset, 'offset', 0, this.length);
}
const remaining = this.length - offset;
if (length === undefined || length > remaining)
length = remaining;
if (string.length > 0 && (length < 0 || offset < 0))
throw new ERR_BUFFER_OUT_OF_BOUNDS();
} else {
// If someone is still calling the obsolete form of write(), tell them.
// we don't want eg buf.write("foo", "utf8", 10) to silently turn into
// buf.write("foo", "utf8"), so we can't ignore extra args
throw new ERR_NO_LONGER_SUPPORTED(
'Buffer.write(string, encoding, offset[, length])'
);
if (length === undefined) {
length = remaining;
} else if (typeof length === 'string') {
encoding = length;
length = remaining;
} else {
validateInt32(length, 'length', 0, this.length);
if (length > remaining)
length = remaining;
}
}
if (!encoding) return this.utf8Write(string, offset, length);
if (!encoding)
return this.utf8Write(string, offset, length);
encoding += '';
switch (encoding.length) {

1
lib/internal/errors.js
View File

@ -994,7 +994,6 @@ E('ERR_NO_CRYPTO',
'Node.js is not compiled with OpenSSL crypto support', Error);
E('ERR_NO_ICU',
'%s is not supported on Node.js compiled without ICU', TypeError);
E('ERR_NO_LONGER_SUPPORTED', '%s is no longer supported', Error);
E('ERR_OUT_OF_RANGE',
(str, range, input, replaceDefaultBoolean = false) => {
assert(range, 'Missing "range" argument');

30
test/parallel/test-buffer-alloc.js
View File

@ -5,6 +5,12 @@ const vm = require('vm');
const SlowBuffer = require('buffer').SlowBuffer;
// Verify the maximum Uint8Array size. There is no concrete limit by spec. The
// internal limits should be updated if this fails.
assert.throws(
() => new Uint8Array(2 ** 31),
{ message: 'Invalid typed array length: 2147483648' }
);
const b = Buffer.allocUnsafe(1024);
assert.strictEqual(b.length, 1024);
@ -59,8 +65,7 @@ assert.throws(() => b.write('test string', 0, 5, 'invalid'),
/Unknown encoding: invalid/);
// Unsupported arguments for Buffer.write
assert.throws(() => b.write('test', 'utf8', 0),
/is no longer supported/);
{ code: 'ERR_INVALID_ARG_TYPE' });
// Try to create 0-length buffers. Should not throw.
Buffer.from('');
@ -74,27 +79,22 @@ new Buffer('', 'latin1');
new Buffer('', 'binary');
Buffer(0);
const outOfBoundsError = {
code: 'ERR_BUFFER_OUT_OF_BOUNDS',
type: RangeError
};
const outOfRangeError = {
code: 'ERR_OUT_OF_RANGE',
type: RangeError
};
// Try to write a 0-length string beyond the end of b
common.expectsError(() => b.write('', 2048), outOfBoundsError);
common.expectsError(() => b.write('', 2048), outOfRangeError);
// Throw when writing to negative offset
common.expectsError(() => b.write('a', -1), outOfBoundsError);
common.expectsError(() => b.write('a', -1), outOfRangeError);
// Throw when writing past bounds from the pool
common.expectsError(() => b.write('a', 2048), outOfBoundsError);
common.expectsError(() => b.write('a', 2048), outOfRangeError);
// Throw when writing to negative offset
common.expectsError(() => b.write('a', -1), outOfBoundsError);
common.expectsError(() => b.write('a', -1), outOfRangeError);
// Try to copy 0 bytes worth of data into an empty buffer
b.copy(Buffer.alloc(0), 0, 0, 0);
@ -110,8 +110,12 @@ b.copy(Buffer.alloc(1), 0, 2048, 2048);
{
const writeTest = Buffer.from('abcdes');
writeTest.write('n', 'ascii');
writeTest.write('o', '1', 'ascii');
writeTest.write('d', '2', 'ascii');
assert.throws(
() => writeTest.write('o', '1', 'ascii'),
{ code: 'ERR_INVALID_ARG_TYPE' }
);
writeTest.write('o', 1, 'ascii');
writeTest.write('d', 2, 'ascii');
writeTest.write('e', 3, 'ascii');
writeTest.write('j', 4, 'ascii');
assert.strictEqual(writeTest.toString(), 'nodejs');

42
test/parallel/test-buffer-compare-offset.js
View File

@ -10,7 +10,7 @@ assert.strictEqual(a.compare(b), -1);
// Equivalent to a.compare(b).
assert.strictEqual(a.compare(b, 0), -1);
assert.strictEqual(a.compare(b, '0'), -1);
assert.throws(() => a.compare(b, '0'), { code: 'ERR_INVALID_ARG_TYPE' });
assert.strictEqual(a.compare(b, undefined), -1);
// Equivalent to a.compare(b).
@ -18,7 +18,10 @@ assert.strictEqual(a.compare(b, 0, undefined, 0), -1);
// Zero-length target, return 1
assert.strictEqual(a.compare(b, 0, 0, 0), 1);
assert.strictEqual(a.compare(b, '0', '0', '0'), 1);
assert.throws(
() => a.compare(b, 0, '0', '0'),
{ code: 'ERR_INVALID_ARG_TYPE' }
);
// Equivalent to Buffer.compare(a, b.slice(6, 10))
assert.strictEqual(a.compare(b, 6, 10), 1);
@ -45,24 +48,41 @@ assert.strictEqual(a.compare(b, 0, 7, 4), -1);
// Equivalent to Buffer.compare(a.slice(4, 6), b.slice(0, 7));
assert.strictEqual(a.compare(b, 0, 7, 4, 6), -1);
// zero length target
assert.strictEqual(a.compare(b, 0, null), 1);
// Null is ambiguous.
assert.throws(
() => a.compare(b, 0, null),
{ code: 'ERR_INVALID_ARG_TYPE' }
);
// Coerces to targetEnd == 5
assert.strictEqual(a.compare(b, 0, { valueOf: () => 5 }), -1);
// Values do not get coerced.
assert.throws(
() => a.compare(b, 0, { valueOf: () => 5 }),
{ code: 'ERR_INVALID_ARG_TYPE' }
);
// zero length target
assert.strictEqual(a.compare(b, Infinity, -Infinity), 1);
// Infinity should not be coerced.
assert.throws(
() => a.compare(b, Infinity, -Infinity),
{ code: 'ERR_OUT_OF_RANGE' }
);
// Zero length target because default for targetEnd <= targetSource
assert.strictEqual(a.compare(b, '0xff'), 1);
assert.strictEqual(a.compare(b, 0xff), 1);
const oor = common.expectsError({ code: 'ERR_OUT_OF_RANGE' }, 7);
assert.throws(
() => a.compare(b, '0xff'),
{ code: 'ERR_INVALID_ARG_TYPE' }
);
assert.throws(
() => a.compare(b, 0, '0xff'),
{ code: 'ERR_INVALID_ARG_TYPE' }
);
const oor = { code: 'ERR_OUT_OF_RANGE' };
assert.throws(() => a.compare(b, 0, 100, 0), oor);
assert.throws(() => a.compare(b, 0, 1, 0, 100), oor);
assert.throws(() => a.compare(b, -1), oor);
assert.throws(() => a.compare(b, 0, '0xff'), oor);
assert.throws(() => a.compare(b, 0, Infinity), oor);
assert.throws(() => a.compare(b, 0, 1, -1), oor);
assert.throws(() => a.compare(b, -Infinity, Infinity), oor);

23
test/parallel/test-buffer-fill.js
View File

@ -333,34 +333,17 @@ assert.strictEqual(
// Make sure "end" is properly checked, even if it's magically mangled using
// Symbol.toPrimitive.
{
let elseWasLast = false;
const expectedErrorMessage =
'The value of "end" is out of range. It must be >= 0 and <= 1. Received -1';
common.expectsError(() => {
let ctr = 0;
const end = {
[Symbol.toPrimitive]() {
// We use this condition to get around the check in lib/buffer.js
if (ctr === 0) {
elseWasLast = false;
ctr++;
return 1;
}
elseWasLast = true;
// Once buffer.js calls the C++ implementation of fill, return -1
return -1;
return 1;
}
};
Buffer.alloc(1).fill(Buffer.alloc(1), 0, end);
}, {
code: 'ERR_OUT_OF_RANGE',
type: RangeError,
message: expectedErrorMessage
code: 'ERR_INVALID_ARG_TYPE',
message: 'The "end" argument must be of type number. Received type object'
});
// Make sure -1 is making it to Buffer::Fill().
assert.ok(elseWasLast,
'internal API changed, -1 no longer in correct location');
}
// Testing process.binding. Make sure "end" is properly checked for -1 wrap

19
test/parallel/test-buffer-write.js
View File

@ -3,14 +3,17 @@
const common = require('../common');
const assert = require('assert');
const outsideBounds = common.expectsError({
code: 'ERR_BUFFER_OUT_OF_BOUNDS',
type: RangeError,
message: 'Attempt to write outside buffer bounds'
}, 2);
assert.throws(() => Buffer.alloc(9).write('foo', -1), outsideBounds);
assert.throws(() => Buffer.alloc(9).write('foo', 10), outsideBounds);
[-1, 10].forEach((offset) => {
assert.throws(
() => Buffer.alloc(9).write('foo', offset),
{
code: 'ERR_OUT_OF_RANGE',
name: 'RangeError',
message: 'The value of "offset" is out of range. ' +
`It must be >= 0 && <= 9. Received ${offset}`
}
);
});
const resultMap = new Map([
['utf8', Buffer.from([102, 111, 111, 0, 0, 0, 0, 0, 0])],