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.

Sign up for GitHub

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 checking for slowToString #2919

Closed
wants to merge 1 commit into from
Closed

buffer: fix range checking for slowToString #2919

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 23 additions & 5 deletions lib/buffer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -322,13 +322,31 @@ Object.defineProperty(Buffer.prototype, 'offset', {
function slowToString(encoding, start, end) {
var loweredCase = false;

start = start >>> 0;
end = end === undefined || end === Infinity ? this.length : end >>> 0;
if (start >= this.length || end <= 0)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe end < 0 would better here instead of end <= 0? In order to treat false as this.length, for example.

Copy link
Contributor

Choose a reason for hiding this comment

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

so all falsey values will evaluate to default value? I'd be cool with that, except we also assert that true is coerced to 1. seems like a small inconsistency to evaluate the two types differently.

eh. guess it's instead falsey vs. truthy. okay, i'm cool with it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Going to revise my opinion. I think we should imitate similar behavior as v8 in other scenarios. Here's one using Typed Arrays:

let ab = new ArrayBuffer(8);
new Uint8Array(ab, 0, false).length === 0;
new Uint8Array(ab, 0, null).length === 0;
new Uint8Array(ab, 0, NaN).length === 0;
new Uint8Array(ab, 0, undefined).length === 8;

So end only receives the default length iff end === undefined.

There are cases where Typed Arrays throw where we can't, but we'll leave those aside.

return '';

/*
* if unsigned 32 bit value (>>> 0) of `start` is not the same as `start`,
* use `0` as the default value. Similarly, if the unsigned 32 bit value of
* `end` is not the same as `end`, then use the length of the buffer as the
* end value.
*
* Note that, the comparison is done with abstract equality operator (!=) to
* allow stringified numbers ('1') and Number objects (new Number(5)).
*/
if (start < 0 || start >>> 0 != parseInt(+start))
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if you want to, but maybe also add to comments why both parseInt() and the + are necessary. For example:

parseInt('0o1101') === 0
parseInt(+'0o1101') === 577

but also that parseInt() is necessary to do int coercion at 2^53 to make sure there's no wrap around.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, should be able to use !== here b/c start is being coerced to a number before comparison.

Copy link
Contributor

Choose a reason for hiding this comment

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

but may also want to note that this is technically an int32 check, not uint32, b/c the value will be coerced to a uint32 below with start >>>= 0. so we're saving ourselves an operation.

start = 0;

if (end > this.length || end >>> 0 != parseInt(+end))
end = this.length;

start >>>= 0;
end >>>= 0;

if (end <= start)
return '';

if (!encoding) encoding = 'utf8';
if (start < 0) start = 0;
if (end > this.length) end = this.length;
if (end <= start) return '';

while (true) {
switch (encoding) {
Expand Down
2 changes: 1 addition & 1 deletion src/node_internals.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ inline MUST_USE_RESULT bool ParseArrayIndex(v8::Local<v8::Value> arg,
return true;
}

int32_t tmp_i = arg->Int32Value();
int32_t tmp_i = arg->Uint32Value();
Copy link
Contributor

Choose a reason for hiding this comment

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

need to change int32_t to uint32_t.


if (tmp_i < 0)
return false;
Expand Down
81 changes: 75 additions & 6 deletions test/parallel/test-buffer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,14 +248,83 @@ b.copy(new Buffer(1), 1, 1, 1);
// try to copy 0 bytes from past the end of the source buffer
b.copy(new Buffer(1), 0, 2048, 2048);

// try to toString() a 0-length slice of a buffer, both within and without the
// valid buffer range
assert.equal(new Buffer('abc').toString('ascii', 0, 0), '');
assert.equal(new Buffer('abc').toString('ascii', -100, -100), '');
assert.equal(new Buffer('abc').toString('ascii', 100, 100), '');
const rangeBuffer = new Buffer('abc');

// if start >= buffer's length, empty string will be returned
assert.equal(rangeBuffer.toString('ascii', 3), '');
assert.equal(rangeBuffer.toString('ascii', +Infinity), '');
assert.equal(rangeBuffer.toString('ascii', 3.14, 3), '');
assert.equal(rangeBuffer.toString('ascii', 'Infinity', 3), '');

// if end <= 0, empty string will be returned
assert.equal(rangeBuffer.toString('ascii', 1, 0), '');
assert.equal(rangeBuffer.toString('ascii', 1, -1.2), '');
assert.equal(rangeBuffer.toString('ascii', 1, -100), '');
assert.equal(rangeBuffer.toString('ascii', 1, -Infinity), '');

// if start < 0, start will be taken as zero
assert.equal(rangeBuffer.toString('ascii', -1, 3), 'abc');
assert.equal(rangeBuffer.toString('ascii', -1.99, 3), 'abc');
assert.equal(rangeBuffer.toString('ascii', -Infinity, 3), 'abc');
assert.equal(rangeBuffer.toString('ascii', '-1', 3), 'abc');
assert.equal(rangeBuffer.toString('ascii', '-1.99', 3), 'abc');
assert.equal(rangeBuffer.toString('ascii', '-Infinity', 3), 'abc');

// if start is an invalid integer, start will be taken as zero
assert.equal(rangeBuffer.toString('ascii', 'node.js', 3), 'abc');
assert.equal(rangeBuffer.toString('ascii', {}, 3), 'abc');
assert.equal(rangeBuffer.toString('ascii', [], 3), 'abc');
assert.equal(rangeBuffer.toString('ascii', NaN, 3), 'abc');
assert.equal(rangeBuffer.toString('ascii', null, 3), 'abc');
assert.equal(rangeBuffer.toString('ascii', undefined, 3), 'abc');
assert.equal(rangeBuffer.toString('ascii', false, 3), 'abc');
assert.equal(rangeBuffer.toString('ascii', '', 3), 'abc');

// but, if start is an integer when coerced, then it will be coerced and used.
assert.equal(rangeBuffer.toString('ascii', '-1', 3), 'abc');
assert.equal(rangeBuffer.toString('ascii', '1', 3), 'bc');
assert.equal(rangeBuffer.toString('ascii', '-Infinity', 3), 'abc');
assert.equal(rangeBuffer.toString('ascii', '3', 3), '');
assert.equal(rangeBuffer.toString('ascii', Number(3), 3), '');
assert.equal(rangeBuffer.toString('ascii', '3.14', 3), '');
assert.equal(rangeBuffer.toString('ascii', '1.99', 3), 'bc');
assert.equal(rangeBuffer.toString('ascii', '-1.99', 3), 'abc');
assert.equal(rangeBuffer.toString('ascii', 1.99, 3), 'bc');
assert.equal(rangeBuffer.toString('ascii', true, 3), 'bc');

// if end > buffer's length, end will be taken as buffer's length
assert.equal(rangeBuffer.toString('ascii', 0, 5), 'abc');
assert.equal(rangeBuffer.toString('ascii', 0, 6.99), 'abc');
assert.equal(rangeBuffer.toString('ascii', 0, Infinity), 'abc');
assert.equal(rangeBuffer.toString('ascii', 0, '5'), 'abc');
assert.equal(rangeBuffer.toString('ascii', 0, '6.99'), 'abc');
assert.equal(rangeBuffer.toString('ascii', 0, 'Infinity'), 'abc');

// if end is an invalid integer, end will be taken as buffer's length
assert.equal(rangeBuffer.toString('ascii', 0, 'node.js'), 'abc');
assert.equal(rangeBuffer.toString('ascii', 0, {}), 'abc');
assert.equal(rangeBuffer.toString('ascii', 0, NaN), 'abc');
assert.equal(rangeBuffer.toString('ascii', 0, undefined), 'abc');
assert.equal(rangeBuffer.toString('ascii', 0), 'abc');
assert.equal(rangeBuffer.toString('ascii', 0, null), '');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure if these four tests returning empty strings is okay though.

Copy link
Contributor

Choose a reason for hiding this comment

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

So should end be coerced to 0 instead of this.length?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the end value is not valid, simply ignoring it and assuming the actual length would be logical, wouldn't it?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree.

Copy link
Member

Choose a reason for hiding this comment

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

if (start >= this.length || end <= 0)

Let's try if (start >= this.length || end < 0).

assert.equal(rangeBuffer.toString('ascii', 0, []), '');
assert.equal(rangeBuffer.toString('ascii', 0, false), '');
assert.equal(rangeBuffer.toString('ascii', 0, ''), '');

// but, if end is an integer when coerced, then it will be coerced and used.
assert.equal(rangeBuffer.toString('ascii', 0, '-1'), '');
assert.equal(rangeBuffer.toString('ascii', 0, '1'), 'a');
assert.equal(rangeBuffer.toString('ascii', 0, '-Infinity'), '');
assert.equal(rangeBuffer.toString('ascii', 0, '3'), 'abc');
assert.equal(rangeBuffer.toString('ascii', 0, Number(3)), 'abc');
assert.equal(rangeBuffer.toString('ascii', 0, '3.14'), 'abc');
assert.equal(rangeBuffer.toString('ascii', 0, '1.99'), 'a');
assert.equal(rangeBuffer.toString('ascii', 0, '-1.99'), '');
assert.equal(rangeBuffer.toString('ascii', 0, 1.99), 'a');
assert.equal(rangeBuffer.toString('ascii', 0, true), 'a');

// try toString() with a object as a encoding
assert.equal(new Buffer('abc').toString({toString: function() {
assert.equal(rangeBuffer.toString({toString: function() {
return 'ascii';
}}), 'abc');

Expand Down