Skip to content

buffer: prevent failed assertions #1922

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

Closed
brendanashworth wants to merge 2 commits into from
Closed

buffer: prevent failed assertions #1922

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ca408dc
buffer: prevent failed assertions
brendanashworth Jun 9, 2015
ece2b4e
!fixup remove checks besides toString()
brendanashworth Jun 9, 2015
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
42 changes: 38 additions & 4 deletions lib/buffer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
const binding = process.binding('buffer');
const smalloc = process.binding('smalloc');
const util = require('util');
const assert = require('assert');
const internalUtil = require('internal/util');
const alloc = smalloc.alloc;
const truncate = smalloc.truncate;
Expand Down Expand Up @@ -335,6 +336,14 @@ Buffer.byteLength = byteLength;

// toString(encoding, start=0, end=buffer.length)
Buffer.prototype.toString = function(encoding, start, end) {
// .toString() can be called automatically on non-Buffers.
Copy link
Copy Markdown
Contributor

@domenic domenic Jun 9, 2015

Choose a reason for hiding this comment

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

How?

Copy link
Copy Markdown
Contributor Author

@brendanashworth brendanashworth Jun 9, 2015

Choose a reason for hiding this comment

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

+Buffer.prototype, it isn't "supported" but it was decided that it shouldn't kill the process. I can add that to the comment, if you'd like.

Copy link
Copy Markdown
Contributor

@domenic domenic Jun 9, 2015

Choose a reason for hiding this comment

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

Yes, and some more detail on what "automatically" means would be great...

The fix seems overcomplicated in any case. Why throw for arguments.length !== 0?

Copy link
Copy Markdown
Contributor

@trevnorris trevnorris Jun 9, 2015

Choose a reason for hiding this comment

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

To get here you'd have to call toString.{call,apply}() or to copy it to the prototype of another constructor, etc. Either way you're hosed. Like @domenic mentions, just throw a TypeError if the instanceof check fails.

if (!(this instanceof Buffer)) {
if (arguments.length === 0)
Copy link
Copy Markdown
Contributor

@domenic domenic Jun 9, 2015

Choose a reason for hiding this comment

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

I still don't understand the purpose of this check at all. Why not always return O.p.toString when used on a non-buffer.

return Object.prototype.toString.call(this);
else
assert(this instanceof Buffer);
}

var loweredCase = false;

start = start >>> 0;
Expand Down Expand Up @@ -380,8 +389,7 @@ Buffer.prototype.toString = function(encoding, start, end) {


Buffer.prototype.equals = function equals(b) {
if (!(b instanceof Buffer))
throw new TypeError('Argument must be a Buffer');
assert(this instanceof Buffer && b instanceof Buffer);

if (this === b)
return true;
Expand All @@ -404,8 +412,7 @@ Buffer.prototype.inspect = function inspect() {


Buffer.prototype.compare = function compare(b) {
if (!(b instanceof Buffer))
throw new TypeError('Argument must be a Buffer');
assert(this instanceof Buffer && b instanceof Buffer);

if (this === b)
return 0;
Expand All @@ -415,6 +422,8 @@ Buffer.prototype.compare = function compare(b) {


Buffer.prototype.indexOf = function indexOf(val, byteOffset) {
assert(this instanceof Buffer);

if (byteOffset > 0x7fffffff)
byteOffset = 0x7fffffff;
else if (byteOffset < -0x80000000)
Expand All @@ -433,6 +442,8 @@ Buffer.prototype.indexOf = function indexOf(val, byteOffset) {


Buffer.prototype.fill = function fill(val, start, end) {
assert(this instanceof Buffer);

start = start >> 0;
end = (end === undefined) ? this.length : end >> 0;

Expand All @@ -455,6 +466,13 @@ Buffer.prototype.fill = function fill(val, start, end) {
};


Buffer.prototype.copy = function copy(target, start, sourceStart, sourceEnd) {
assert(this instanceof Buffer && target instanceof Buffer);

return binding.copy(this, target, start, sourceStart, sourceEnd);
};


// XXX remove in v0.13
Buffer.prototype.get = util.deprecate(function get(offset) {
offset = ~~offset;
Expand Down Expand Up @@ -779,6 +797,8 @@ Buffer.prototype.readInt32BE = function(offset, noAssert) {


Buffer.prototype.readFloatLE = function readFloatLE(offset, noAssert) {
assert(this instanceof Buffer);

offset = offset >>> 0;
if (!noAssert)
checkOffset(offset, 4, this.length);
Expand All @@ -787,6 +807,8 @@ Buffer.prototype.readFloatLE = function readFloatLE(offset, noAssert) {


Buffer.prototype.readFloatBE = function readFloatBE(offset, noAssert) {
assert(this instanceof Buffer);

offset = offset >>> 0;
if (!noAssert)
checkOffset(offset, 4, this.length);
Expand All @@ -795,6 +817,8 @@ Buffer.prototype.readFloatBE = function readFloatBE(offset, noAssert) {


Buffer.prototype.readDoubleLE = function readDoubleLE(offset, noAssert) {
assert(this instanceof Buffer);

offset = offset >>> 0;
if (!noAssert)
checkOffset(offset, 8, this.length);
Expand All @@ -803,6 +827,8 @@ Buffer.prototype.readDoubleLE = function readDoubleLE(offset, noAssert) {


Buffer.prototype.readDoubleBE = function readDoubleBE(offset, noAssert) {
assert(this instanceof Buffer);

offset = offset >>> 0;
if (!noAssert)
checkOffset(offset, 8, this.length);
Expand Down Expand Up @@ -1025,6 +1051,8 @@ function checkFloat(buffer, value, offset, ext) {


Buffer.prototype.writeFloatLE = function writeFloatLE(val, offset, noAssert) {
assert(this instanceof Buffer);

val = +val;
offset = offset >>> 0;
if (!noAssert)
Expand All @@ -1035,6 +1063,8 @@ Buffer.prototype.writeFloatLE = function writeFloatLE(val, offset, noAssert) {


Buffer.prototype.writeFloatBE = function writeFloatBE(val, offset, noAssert) {
assert(this instanceof Buffer);

val = +val;
offset = offset >>> 0;
if (!noAssert)
Expand All @@ -1045,6 +1075,8 @@ Buffer.prototype.writeFloatBE = function writeFloatBE(val, offset, noAssert) {


Buffer.prototype.writeDoubleLE = function writeDoubleLE(val, offset, noAssert) {
assert(this instanceof Buffer);

val = +val;
offset = offset >>> 0;
if (!noAssert)
Expand All @@ -1055,6 +1087,8 @@ Buffer.prototype.writeDoubleLE = function writeDoubleLE(val, offset, noAssert) {


Buffer.prototype.writeDoubleBE = function writeDoubleBE(val, offset, noAssert) {
assert(this instanceof Buffer);

val = +val;
offset = offset >>> 0;
if (!noAssert)
Expand Down
16 changes: 6 additions & 10 deletions src/node_buffer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,22 +303,19 @@ void Base64Slice(const FunctionCallbackInfo<Value>& args) {
void Copy(const FunctionCallbackInfo<Value> &args) {
Environment* env = Environment::GetCurrent(args);

if (!HasInstance(args[0]))
return env->ThrowTypeError("first arg should be a Buffer");
Local<Object> target = args[1]->ToObject(env->isolate());

Local<Object> target = args[0]->ToObject(env->isolate());

ARGS_THIS(args.This())
ARGS_THIS(args[0].As<Object>());
size_t target_length = target->GetIndexedPropertiesExternalArrayDataLength();
char* target_data = static_cast<char*>(
target->GetIndexedPropertiesExternalArrayData());
size_t target_start;
size_t source_start;
size_t source_end;

CHECK_NOT_OOB(ParseArrayIndex(args[1], 0, &target_start));
CHECK_NOT_OOB(ParseArrayIndex(args[2], 0, &source_start));
CHECK_NOT_OOB(ParseArrayIndex(args[3], obj_length, &source_end));
CHECK_NOT_OOB(ParseArrayIndex(args[2], 0, &target_start));
CHECK_NOT_OOB(ParseArrayIndex(args[3], 0, &source_start));
CHECK_NOT_OOB(ParseArrayIndex(args[4], obj_length, &source_end));

// Copy 0 bytes; we're done
if (target_start >= target_length || source_start >= source_end)
Expand Down Expand Up @@ -723,8 +720,6 @@ void SetupBufferJS(const FunctionCallbackInfo<Value>& args) {
env->SetMethod(proto, "ucs2Write", Ucs2Write);
env->SetMethod(proto, "utf8Write", Utf8Write);

env->SetMethod(proto, "copy", Copy);

// for backwards compatibility
proto->ForceSet(env->offset_string(),
Uint32::New(env->isolate(), 0),
Expand All @@ -742,6 +737,7 @@ void Initialize(Handle<Object> target,
env->SetMethod(target, "byteLengthUtf8", ByteLengthUtf8);
env->SetMethod(target, "compare", Compare);
env->SetMethod(target, "fill", Fill);
env->SetMethod(target, "copy", Copy);
env->SetMethod(target, "indexOfBuffer", IndexOfBuffer);
env->SetMethod(target, "indexOfNumber", IndexOfNumber);
env->SetMethod(target, "indexOfString", IndexOfString);
Expand Down
42 changes: 42 additions & 0 deletions test/parallel/test-buffer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1172,3 +1172,45 @@ Buffer.poolSize = ps;
assert.throws(function() {
Buffer(10).copy();
});


// https://github.com/nodejs/io.js/issues/1485
// C++ bindings fail assertions (die) when called on non-buffers.
assert.equal(Buffer.prototype.toString(), '[object Object]');

[null, undefined, {}, 0].forEach(function(v) {
assert.throws(function() {
Buffer.prototype.write.call(v);
});

assert.throws(function() {
Buffer.prototype.copy.call(v, new Buffer(0));
});
assert.throws(function() {
(new Buffer(0)).copy(v);
});

assert.throws(function() {
Buffer.prototype.equals.call(v, new Buffer('hi'));
});

assert.throws(function() {
Buffer.prototype.compare.call(v, new Buffer('hi'));
});

assert.throws(function() {
Buffer.prototype.fill.call(v);
});

assert.throws(function() {
Buffer.prototype.indexOf.call(v, 0);
});

assert.throws(function() {
Buffer.prototype.readDoubleLE.call(v, 0, false);
});

assert.throws(function() {
Buffer.prototype.writeDoubleLE.call(v, 0, 0, false);
});
});