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

Jump to bottom

buffer: prevent abort on bad proto #2012

Closed
wants to merge 1 commit into from
Closed

buffer: prevent abort on bad proto #2012

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
2 changes: 0 additions & 2 deletions lib/buffer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -380,8 +380,6 @@ function slowToString(encoding, start, end) {

Buffer.prototype.toString = function() {
const length = this.length | 0;
Copy link
Member

Choose a reason for hiding this comment

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

length is only used when arguments.length === 0 now.

if (length === 0)
return '';
if (arguments.length === 0)
return this.utf8Slice(0, length);
return slowToString.apply(this, arguments);
Expand Down
36 changes: 32 additions & 4 deletions src/node_buffer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,12 @@
if (!(r)) return env->ThrowRangeError("out of range index"); \
} while (0)

#define THROW_AND_RETURN_UNLESS_BUFFER(env, obj) \
do { \
if (!HasInstance(obj)) \
return env->ThrowTypeError("argument should be a Buffer"); \
} while (0)

#define ARGS_THIS(argT) \
Local<Object> obj = argT; \
size_t obj_length = obj->GetIndexedPropertiesExternalArrayDataLength(); \
Expand Down Expand Up @@ -223,7 +229,12 @@ template <encoding encoding>
void StringSlice(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);

THROW_AND_RETURN_UNLESS_BUFFER(env, args.This());
ARGS_THIS(args.This())

if (obj_length == 0)
return args.GetReturnValue().SetEmptyString();

SLICE_START_END(args[0], args[1], obj_length)

args.GetReturnValue().Set(
Expand All @@ -235,7 +246,12 @@ template <>
void StringSlice<UCS2>(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);

THROW_AND_RETURN_UNLESS_BUFFER(env, args.This());
ARGS_THIS(args.This())

if (obj_length == 0)
return args.GetReturnValue().SetEmptyString();

SLICE_START_END(args[0], args[1], obj_length)
length /= 2;

Expand Down Expand Up @@ -306,8 +322,9 @@ void Copy(const FunctionCallbackInfo<Value> &args) {
if (!HasInstance(args[0]))
return env->ThrowTypeError("first arg should be a Buffer");

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

THROW_AND_RETURN_UNLESS_BUFFER(env, args.This());
ARGS_THIS(args.This())
size_t target_length = target->GetIndexedPropertiesExternalArrayDataLength();
char* target_data = static_cast<char*>(
Expand Down Expand Up @@ -340,6 +357,7 @@ void Copy(const FunctionCallbackInfo<Value> &args) {


void Fill(const FunctionCallbackInfo<Value>& args) {
THROW_AND_RETURN_UNLESS_BUFFER(Environment::GetCurrent(args), args[0]);
ARGS_THIS(args[0].As<Object>())

size_t start = args[2]->Uint32Value();
Expand Down Expand Up @@ -383,6 +401,7 @@ template <encoding encoding>
void StringWrite(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);

THROW_AND_RETURN_UNLESS_BUFFER(env, args.This());
ARGS_THIS(args.This())

if (!args[0]->IsString())
Expand Down Expand Up @@ -459,6 +478,7 @@ static inline void Swizzle(char* start, unsigned int len) {

template <typename T, enum Endianness endianness>
void ReadFloatGeneric(const FunctionCallbackInfo<Value>& args) {
THROW_AND_RETURN_UNLESS_BUFFER(Environment::GetCurrent(args), args[0]);
ARGS_THIS(args[0].As<Object>());

uint32_t offset = args[1]->Uint32Value();
Expand Down Expand Up @@ -522,21 +542,25 @@ uint32_t WriteFloatGeneric(const FunctionCallbackInfo<Value>& args) {


void WriteFloatLE(const FunctionCallbackInfo<Value>& args) {
THROW_AND_RETURN_UNLESS_BUFFER(Environment::GetCurrent(args), args[0]);
args.GetReturnValue().Set(WriteFloatGeneric<float, kLittleEndian>(args));
}


void WriteFloatBE(const FunctionCallbackInfo<Value>& args) {
THROW_AND_RETURN_UNLESS_BUFFER(Environment::GetCurrent(args), args[0]);
args.GetReturnValue().Set(WriteFloatGeneric<float, kBigEndian>(args));
}


void WriteDoubleLE(const FunctionCallbackInfo<Value>& args) {
THROW_AND_RETURN_UNLESS_BUFFER(Environment::GetCurrent(args), args[0]);
args.GetReturnValue().Set(WriteFloatGeneric<double, kLittleEndian>(args));
}


void WriteDoubleBE(const FunctionCallbackInfo<Value>& args) {
THROW_AND_RETURN_UNLESS_BUFFER(Environment::GetCurrent(args), args[0]);
args.GetReturnValue().Set(WriteFloatGeneric<double, kBigEndian>(args));
}

Expand All @@ -550,6 +574,10 @@ void ByteLengthUtf8(const FunctionCallbackInfo<Value> &args) {


void Compare(const FunctionCallbackInfo<Value> &args) {
Environment* env = Environment::GetCurrent(args);
THROW_AND_RETURN_UNLESS_BUFFER(env, args[0]);
THROW_AND_RETURN_UNLESS_BUFFER(env, args[1]);

Local<Object> obj_a = args[0].As<Object>();
char* obj_a_data =
static_cast<char*>(obj_a->GetIndexedPropertiesExternalArrayData());
Expand Down Expand Up @@ -599,10 +627,10 @@ int32_t IndexOf(const char* haystack,


void IndexOfString(const FunctionCallbackInfo<Value>& args) {
ASSERT(args[0]->IsObject());
ASSERT(args[1]->IsString());
ASSERT(args[2]->IsNumber());

THROW_AND_RETURN_UNLESS_BUFFER(Environment::GetCurrent(args), args[0]);
ARGS_THIS(args[0].As<Object>());
node::Utf8Value str(args.GetIsolate(), args[1]);
int32_t offset_i32 = args[2]->Int32Value();
Expand Down Expand Up @@ -630,10 +658,10 @@ void IndexOfString(const FunctionCallbackInfo<Value>& args) {


void IndexOfBuffer(const FunctionCallbackInfo<Value>& args) {
ASSERT(args[0]->IsObject());
ASSERT(args[1]->IsObject());
ASSERT(args[2]->IsNumber());

THROW_AND_RETURN_UNLESS_BUFFER(Environment::GetCurrent(args), args[0]);
ARGS_THIS(args[0].As<Object>());
Local<Object> buf = args[1].As<Object>();
int32_t offset_i32 = args[2]->Int32Value();
Expand Down Expand Up @@ -667,10 +695,10 @@ void IndexOfBuffer(const FunctionCallbackInfo<Value>& args) {


void IndexOfNumber(const FunctionCallbackInfo<Value>& args) {
ASSERT(args[0]->IsObject());
ASSERT(args[1]->IsNumber());
ASSERT(args[2]->IsNumber());

THROW_AND_RETURN_UNLESS_BUFFER(Environment::GetCurrent(args), args[0]);
ARGS_THIS(args[0].As<Object>());
uint32_t needle = args[1]->Uint32Value();
int32_t offset_i32 = args[2]->Int32Value();
Expand Down
56 changes: 56 additions & 0 deletions test/parallel/test-buffer-fakes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
'use strict';

const common = require('../common');
const assert = require('assert');
const Buffer = require('buffer').Buffer;
const Bp = Buffer.prototype;

function FakeBuffer() { }
FakeBuffer.__proto__ = Buffer;
FakeBuffer.prototype.__proto__ = Buffer.prototype;

const fb = new FakeBuffer();

assert.throws(function() {
new Buffer(fb);
}, TypeError);

assert.throws(function() {
+Buffer.prototype;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this should fail, but rather coerce as usual with Object.prototype.toString.call(Buffer.prototype). Thoughts?

> '' + net.Socket.prototype
'[object Object]'
> '' + Buffer.prototype
TypeError: blah blah blah

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This operation calls the toString method on the object. Which is a custom implementation. It seems standard to throw in such cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

I disagree and think that regardless of how the operation works, we shouldn't throw an error on simple coercion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

V8 throws the 'incompatible_method_receiver' TypeError for any custom defined toString. For example:

> Symbol.prototype.toString.call({})
TypeError: Method Symbol.prototype.toString called on incompatible receiver #<Object>

This type of behavior is defined by the ES spec: https://people.mozilla.org/~jorendorff/es6-draft.html#sec-symbol.prototype.tostring

I think it would be smart for us to follow the same pattern.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wasn't aware of that before, thank you, failing with an error now makes sense to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Neither was I before this PR. Previously I would have agreed with you on just coercing the call.

}, TypeError);

assert.throws(function() {
Buffer.compare(fb, new Buffer(0));
}, TypeError);

assert.throws(function() {
fb.write('foo');
}, TypeError);

assert.throws(function() {
Buffer.concat([fb, fb]);
}, TypeError);

assert.throws(function() {
fb.toString();
}, TypeError);

assert.throws(function() {
fb.equals(new Buffer(0));
}, TypeError);

assert.throws(function() {
fb.indexOf(5);
}, TypeError);

assert.throws(function() {
fb.readFloatLE(0);
}, TypeError);

assert.throws(function() {
fb.writeFloatLE(0);
}, TypeError);

assert.throws(function() {
fb.fill(0);
}, TypeError);