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.

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: stricter Buffer.isBuffer #11961

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
39 changes: 39 additions & 0 deletions benchmark/buffers/buffer-isbuffer.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
'use strict';
const common = require('../common');
const { Buffer } = require('buffer');

function FakeBuffer() {}
FakeBuffer.prototype = Object.create(Buffer.prototype);

// Refs: https://github.com/nodejs/node/issues/9531#issuecomment-265611061
class ExtensibleBuffer extends Buffer {
constructor() {
super(new ArrayBuffer(0), 0, 0);
Object.setPrototypeOf(this, new.target.prototype);
}
}

const inputs = {
primitive: false,
object: {},
fake: new FakeBuffer(),
subclassed: new ExtensibleBuffer(),
fastbuffer: Buffer.alloc(1),
slowbuffer: Buffer.allocUnsafeSlow(0),
uint8array: new Uint8Array(1)
};

const bench = common.createBenchmark(main, {
type: Object.keys(inputs),
n: [1e7]
});

function main(conf) {
const n = conf.n | 0;
const input = inputs[conf.type];

bench.start();
for (var i = 0; i < n; i++)
Buffer.isBuffer(input);
bench.end(n);
}
4 changes: 2 additions & 2 deletions lib/_http_outgoing.js
Original file line number Diff line number Diff line change
Expand Up @@ -645,7 +645,7 @@ function write_(msg, chunk, encoding, callback, fromEnd) {
return true;
}

if (!fromEnd && typeof chunk !== 'string' && !(chunk instanceof Buffer)) {
if (!fromEnd && typeof chunk !== 'string' && !Buffer.isBuffer(chunk)) {
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'first argument',
['string', 'buffer']);
}
Expand Down Expand Up @@ -743,7 +743,7 @@ OutgoingMessage.prototype.end = function end(chunk, encoding, callback) {

var uncork;
if (chunk) {
if (typeof chunk !== 'string' && !(chunk instanceof Buffer)) {
if (typeof chunk !== 'string' && !Buffer.isBuffer(chunk)) {
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'first argument',
['string', 'buffer']);
}
Expand Down
13 changes: 10 additions & 3 deletions lib/buffer.js
Original file line number Diff line number Diff line change
Expand Up @@ -420,7 +420,14 @@ function fromObject(obj) {
// Static methods

Buffer.isBuffer = function isBuffer(b) {
return b instanceof Buffer;
if (!isUint8Array(b))
return false;
// Subclassing Buffer is not officially supported currently, and actually
// subclassing it requires some awkward code that users are unlikely to do.
// Therefore prioritize the fast case over the full `instanceof`.
// Refs: https://github.com/nodejs/node/issues/9531#issuecomment-265611061
const proto = Object.getPrototypeOf(b);
return proto === Buffer.prototype || proto instanceof Buffer.prototype;
};


Expand Down Expand Up @@ -567,15 +574,15 @@ Buffer.byteLength = byteLength;
Object.defineProperty(Buffer.prototype, 'parent', {
enumerable: true,
get() {
if (!(this instanceof Buffer))
if (!Buffer.isBuffer(this))
return undefined;
return this.buffer;
}
});
Object.defineProperty(Buffer.prototype, 'offset', {
enumerable: true,
get() {
if (!(this instanceof Buffer))
if (!Buffer.isBuffer(this))
return undefined;
return this.byteOffset;
}
Expand Down
2 changes: 1 addition & 1 deletion lib/fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -2198,7 +2198,7 @@ WriteStream.prototype.open = function() {


WriteStream.prototype._write = function(data, encoding, cb) {
if (!(data instanceof Buffer))
if (!Buffer.isBuffer(data))
return this.emit('error', new Error('Invalid data'));

if (typeof this.fd !== 'number') {
Expand Down
8 changes: 4 additions & 4 deletions lib/net.js
Original file line number Diff line number Diff line change
Expand Up @@ -694,7 +694,7 @@ protoGetter('localPort', function localPort() {


Socket.prototype.write = function(chunk, encoding, cb) {
if (typeof chunk !== 'string' && !(chunk instanceof Buffer)) {
if (typeof chunk !== 'string' && !Buffer.isBuffer(chunk)) {
throw new TypeError(
'Invalid data, chunk must be a string or buffer, not ' + typeof chunk);
}
Expand Down Expand Up @@ -752,7 +752,7 @@ Socket.prototype._writeGeneric = function(writev, data, encoding, cb) {
if (err === 0) req._chunks = chunks;
} else {
var enc;
if (data instanceof Buffer) {
if (Buffer.isBuffer(data)) {
enc = 'buffer';
} else {
enc = encoding;
Expand Down Expand Up @@ -821,7 +821,7 @@ protoGetter('bytesWritten', function bytesWritten() {
return undefined;

state.getBuffer().forEach(function(el) {
if (el.chunk instanceof Buffer)
if (Buffer.isBuffer(el.chunk))
bytes += el.chunk.length;
else
bytes += Buffer.byteLength(el.chunk, el.encoding);
Expand All @@ -832,7 +832,7 @@ protoGetter('bytesWritten', function bytesWritten() {
for (var i = 0; i < data.length; i++) {
const chunk = data[i];

if (data.allBuffers || chunk instanceof Buffer)
if (data.allBuffers || Buffer.isBuffer(chunk))
bytes += chunk.length;
else
bytes += Buffer.byteLength(chunk.chunk, chunk.encoding);
Expand Down
2 changes: 1 addition & 1 deletion lib/readline.js
Original file line number Diff line number Diff line change
Expand Up @@ -973,7 +973,7 @@ Interface.prototype._ttyWrite = function(s, key) {
// falls through

default:
if (s instanceof Buffer)
if (Buffer.isBuffer(s))
s = s.toString('utf-8');

if (s) {
Expand Down
31 changes: 31 additions & 0 deletions test/parallel/test-buffer-isbuffer.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
'use strict';
require('../common');
const assert = require('assert');
const { Buffer } = require('buffer');
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to require this, right?

Copy link
Member

Choose a reason for hiding this comment

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

@cjihrig Our linter forbids using Buffer via the global

Copy link
Member

@Trott Trott Mar 22, 2017

Choose a reason for hiding this comment

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

Our linter forbids using Buffer via the global

That's only in lib. Tests like this (as well as benchmarks and tools) can use the Buffer global and the linter won't complain.


const t = (val, exp) => assert.strictEqual(Buffer.isBuffer(val), exp);

function FakeBuffer() {}
FakeBuffer.prototype = Object.create(Buffer.prototype);

class ExtensibleBuffer extends Buffer {
constructor() {
super(new ArrayBuffer(0), 0, 0);
Object.setPrototypeOf(this, new.target.prototype);
}
}

t(0, false);
t(true, false);
t('foo', false);
t(Symbol(), false);
t(null, false);
t(undefined, false);
t(() => {}, false);

Copy link
Contributor

Choose a reason for hiding this comment

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

Why the blank line here?

t({}, false);
t(new Uint8Array(2), false);
t(new FakeBuffer(), false);
t(new ExtensibleBuffer(), true);
t(Buffer.from('foo'), true);
t(Buffer.allocUnsafeSlow(2), true);
1 change: 1 addition & 0 deletions test/parallel/test-util.js
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ assert.strictEqual(true, util.isPrimitive(NaN));
assert.strictEqual(true, util.isPrimitive(Symbol('symbol')));

// isBuffer
assert.strictEqual(util.isBuffer, Buffer.isBuffer);
assert.strictEqual(false, util.isBuffer('foo'));
assert.strictEqual(true, util.isBuffer(Buffer.from('foo')));

Expand Down