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
Prev Previous commit
!fixup remove checks besides toString() 
also add comments
  • Loading branch information
@brendanashworth
brendanashworth committed Jun 9, 2015
commit ece2b4e651a7f5c1d694ea1cba769d509764ca6b
38 changes: 6 additions & 32 deletions lib/buffer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
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 @@ -336,12 +335,12 @@ 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.
// .toString() can be called by JS on prototype (Buffer.prototype + '').
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);
throw new TypeError('this must be instance of Buffer');
}

var loweredCase = false;
Expand Down Expand Up @@ -389,7 +388,8 @@ Buffer.prototype.toString = function(encoding, start, end) {


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

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


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

if (this === b)
return 0;
Expand All @@ -422,8 +423,6 @@ 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 @@ -442,8 +441,6 @@ 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 @@ -466,13 +463,6 @@ 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 @@ -797,8 +787,6 @@ 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 @@ -807,8 +795,6 @@ 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 @@ -817,8 +803,6 @@ 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 @@ -827,8 +811,6 @@ 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 @@ -1051,8 +1033,6 @@ 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 @@ -1063,8 +1043,6 @@ 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 @@ -1075,8 +1053,6 @@ 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 @@ -1087,8 +1063,6 @@ 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: 10 additions & 6 deletions src/node_buffer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,19 +303,22 @@ void Base64Slice(const FunctionCallbackInfo<Value>& args) {
void Copy(const FunctionCallbackInfo<Value> &args) {
Environment* env = Environment::GetCurrent(args);

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

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

ARGS_THIS(args.This())
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[2], 0, &target_start));
CHECK_NOT_OOB(ParseArrayIndex(args[3], 0, &source_start));
CHECK_NOT_OOB(ParseArrayIndex(args[4], obj_length, &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));

// Copy 0 bytes; we're done
if (target_start >= target_length || source_start >= source_end)
Expand Down Expand Up @@ -720,6 +723,8 @@ 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 @@ -737,7 +742,6 @@ 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
38 changes: 0 additions & 38 deletions test/parallel/test-buffer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1173,44 +1173,6 @@ 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);
});
});