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 segfaults when this is not a Buffer. #1486

Closed
wants to merge 2 commits into from
Closed

buffer: fix segfaults when this is not a Buffer. #1486

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9f61af1
move all prototype initialization to JS
monsanto Apr 21, 2015
00da460
validate buffer instance consistently
monsanto Apr 25, 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
152 changes: 121 additions & 31 deletions lib/buffer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,8 +300,28 @@ function byteLength(string, encoding) {

Buffer.byteLength = byteLength;


Buffer.prototype.copy = function(target, targetStart, sourceStart, sourceEnd) {
if (!(this instanceof Buffer))
throw new TypeError('this is not an instance of Buffer');

if (!(target instanceof Buffer))
throw new TypeError('target is not an instance of Buffer');

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

// toString(encoding, start=0, end=buffer.length)
Buffer.prototype.toString = function(encoding, start, end) {
if (!(this instanceof Buffer)) {
// If no arguments, then this is probably an implicit coercion, and we
// shouldn't throw.
if (arguments.length === 0)
return Object.prototype.toString.call(this);
else
throw new TypeError('this is not an instance of Buffer');
Copy link
Contributor

Choose a reason for hiding this comment

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

Even though this case is insane, it inevitably will break someone in the wild. Because of that this is a sem-minor patch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

which part will break? if this is not a Buffer the process will crash

Copy link
Contributor

Choose a reason for hiding this comment

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

hehe. okay true enough. in that case I'd consider this a bug fix patch. :)

}
Copy link
Contributor

Choose a reason for hiding this comment

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

style: no need for the { }.


var loweredCase = false;

start = start >>> 0;
Expand All @@ -315,26 +335,26 @@ Buffer.prototype.toString = function(encoding, start, end) {
while (true) {
switch (encoding) {
case 'hex':
return this.hexSlice(start, end);
return binding.hexSlice(this, start, end);

case 'utf8':
case 'utf-8':
return this.utf8Slice(start, end);
return binding.utf8Slice(this, start, end);

case 'ascii':
return this.asciiSlice(start, end);
return binding.asciiSlice(this, start, end);

case 'binary':
return this.binarySlice(start, end);
return binding.binarySlice(this, start, end);

case 'base64':
return this.base64Slice(start, end);
return binding.base64Slice(this, start, end);

case 'ucs2':
case 'ucs-2':
case 'utf16le':
case 'utf-16le':
return this.ucs2Slice(start, end);
return binding.ucs2Slice(this, start, end);

default:
if (loweredCase)
Expand All @@ -345,6 +365,30 @@ Buffer.prototype.toString = function(encoding, start, end) {
}
};

Buffer.prototype.asciiSlice = function(start, end) {
return this.toString('ascii', start, end);
};

Buffer.prototype.base64Slice = function(start, end) {
return this.toString('base64', start, end);
};

Buffer.prototype.binarySlice = function(start, end) {
return this.toString('binary', start, end);
};

Buffer.prototype.hexSlice = function(start, end) {
return this.toString('hex', start, end);
};

Buffer.prototype.ucs2Slice = function(start, end) {
return this.toString('ucs2', start, end);
};

Buffer.prototype.utf8Slice = function(start, end) {
return this.toString('utf8', start, end);
Copy link
Contributor

Choose a reason for hiding this comment

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

this makes my heart hurt. i used to use these as a more performant way of doing operations. though they are also no loner documented (though IIRC they once were). guess I'll have to switch to using process.binding('buffer').

};


Buffer.prototype.equals = function equals(b) {
if (!(b instanceof Buffer))
Expand All @@ -371,6 +415,9 @@ Buffer.prototype.inspect = function inspect() {


Buffer.prototype.compare = function compare(b) {
if (!(this instanceof Buffer))
throw new TypeError('this is not an instance of Buffer');

if (!(b instanceof Buffer))
throw new TypeError('Argument must be a Buffer');

Expand All @@ -382,6 +429,9 @@ Buffer.prototype.compare = function compare(b) {


Buffer.prototype.indexOf = function indexOf(val, byteOffset) {
if (!(this instanceof Buffer))
throw new TypeError('this is not an instance of Buffer');

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


Buffer.prototype.fill = function fill(val, start, end) {
if (!(this instanceof Buffer))
throw new TypeError('this is not an instance of Buffer');

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

Expand Down Expand Up @@ -446,6 +499,12 @@ var writeWarned = false;
const writeMsg = '.write(string, encoding, offset, length) is deprecated.' +
' Use write(string[, offset[, length]][, encoding]) instead.';
Buffer.prototype.write = function(string, offset, length, encoding) {
if (!(this instanceof Buffer))
throw new TypeError('this is not an instance of Buffer');

if (typeof string !== 'string')
throw new TypeError('Argument must be a string');
Copy link
Contributor

Choose a reason for hiding this comment

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

don't agree with this. primitives like strings should always be coerced, IMO.

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 is a throw ported from C++, but of course there is no problem with coercing

Copy link
Contributor

Choose a reason for hiding this comment

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

gah! hate that we have such inconsistency in how we handle types. thanks for clarifying.


// Buffer#write(string);
if (offset === undefined) {
encoding = 'utf8';
Expand Down Expand Up @@ -502,27 +561,29 @@ Buffer.prototype.write = function(string, offset, length, encoding) {
for (;;) {
switch (encoding) {
case 'hex':
return this.hexWrite(string, offset, length);
if (string.length % 2 !== 0)
throw new TypeError('Invalid hex string');
Copy link
Contributor

Choose a reason for hiding this comment

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

all this extra throwing needs to be documented, if it's not already.

Copy link
Contributor

Choose a reason for hiding this comment

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

okay, maybe not all of it (like checking instanceof Buffer), but cases like this should.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that is not an extra throw, it is an existing throw ported from C++

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks.

return binding.hexWrite(this, string, offset, length);

case 'utf8':
case 'utf-8':
return this.utf8Write(string, offset, length);
return binding.utf8Write(this, string, offset, length);

case 'ascii':
return this.asciiWrite(string, offset, length);
return binding.asciiWrite(this, string, offset, length);

case 'binary':
return this.binaryWrite(string, offset, length);
return binding.binaryWrite(this, string, offset, length);

case 'base64':
// Warning: maxLength not taken into account in base64Write
return this.base64Write(string, offset, length);
return binding.base64Write(this, string, offset, length);

case 'ucs2':
case 'ucs-2':
case 'utf16le':
case 'utf-16le':
return this.ucs2Write(string, offset, length);
return binding.ucs2Write(this, string, offset, length);

default:
if (loweredCase)
Expand All @@ -533,6 +594,30 @@ Buffer.prototype.write = function(string, offset, length, encoding) {
}
};

Buffer.prototype.asciiWrite = function(string, offset, length) {
return this.write(string, offset, length, 'ascii');
};

Buffer.prototype.base64Write = function(string, offset, length) {
return this.write(string, offset, length, 'base64');
};

Buffer.prototype.binaryWrite = function(string, offset, length) {
return this.write(string, offset, length, 'binary');
};

Buffer.prototype.hexWrite = function(string, offset, length) {
return this.write(string, offset, length, 'hex');
};

Buffer.prototype.ucs2Write = function(string, offset, length) {
return this.write(string, offset, length, 'ucs2');
};

Buffer.prototype.utf8Write = function(string, offset, length) {
return this.write(string, offset, length, 'utf8');
Copy link
Contributor

Choose a reason for hiding this comment

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

we might as well deprecate these calls if they're just being sent back to .write(). (not this PR though, just thinking out loud)

Copy link
Contributor

Choose a reason for hiding this comment

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

ditto on deprecating the .read() calls.

};


Buffer.prototype.toJSON = function() {
return {
Expand All @@ -545,6 +630,9 @@ Buffer.prototype.toJSON = function() {
// TODO(trevnorris): currently works like Array.prototype.slice(), which
// doesn't follow the new standard for throwing on out of range indexes.
Buffer.prototype.slice = function(start, end) {
if (!(this instanceof Buffer))
throw new TypeError('this is not an instance of Buffer');

var len = this.length;
start = ~~start;
end = end === undefined ? len : ~~end;
Expand Down Expand Up @@ -578,7 +666,9 @@ Buffer.prototype.slice = function(start, end) {
};


function checkOffset(offset, ext, length) {
function checkOffset(buffer, offset, ext, length) {
if (!(buffer instanceof Buffer))
throw new TypeError('this is not an instance of Buffer');
if (offset + ext > length)
throw new RangeError('index out of range');
}
Expand All @@ -588,7 +678,7 @@ Buffer.prototype.readUIntLE = function(offset, byteLength, noAssert) {
offset = offset >>> 0;
byteLength = byteLength >>> 0;
if (!noAssert)
checkOffset(offset, byteLength, this.length);
checkOffset(this, offset, byteLength, this.length);

var val = this[offset];
var mul = 1;
Expand All @@ -604,7 +694,7 @@ Buffer.prototype.readUIntBE = function(offset, byteLength, noAssert) {
offset = offset >>> 0;
byteLength = byteLength >>> 0;
if (!noAssert)
checkOffset(offset, byteLength, this.length);
checkOffset(this, offset, byteLength, this.length);

var val = this[offset + --byteLength];
var mul = 1;
Expand All @@ -618,31 +708,31 @@ Buffer.prototype.readUIntBE = function(offset, byteLength, noAssert) {
Buffer.prototype.readUInt8 = function(offset, noAssert) {
offset = offset >>> 0;
if (!noAssert)
checkOffset(offset, 1, this.length);
checkOffset(this, offset, 1, this.length);
return this[offset];
};


Buffer.prototype.readUInt16LE = function(offset, noAssert) {
offset = offset >>> 0;
if (!noAssert)
checkOffset(offset, 2, this.length);
checkOffset(this, offset, 2, this.length);
return this[offset] | (this[offset + 1] << 8);
};


Buffer.prototype.readUInt16BE = function(offset, noAssert) {
offset = offset >>> 0;
if (!noAssert)
checkOffset(offset, 2, this.length);
checkOffset(this, offset, 2, this.length);
return (this[offset] << 8) | this[offset + 1];
};


Buffer.prototype.readUInt32LE = function(offset, noAssert) {
offset = offset >>> 0;
if (!noAssert)
checkOffset(offset, 4, this.length);
checkOffset(this, offset, 4, this.length);

return ((this[offset]) |
(this[offset + 1] << 8) |
Expand All @@ -654,7 +744,7 @@ Buffer.prototype.readUInt32LE = function(offset, noAssert) {
Buffer.prototype.readUInt32BE = function(offset, noAssert) {
offset = offset >>> 0;
if (!noAssert)
checkOffset(offset, 4, this.length);
checkOffset(this, offset, 4, this.length);

return (this[offset] * 0x1000000) +
((this[offset + 1] << 16) |
Expand All @@ -667,7 +757,7 @@ Buffer.prototype.readIntLE = function(offset, byteLength, noAssert) {
offset = offset >>> 0;
byteLength = byteLength >>> 0;
if (!noAssert)
checkOffset(offset, byteLength, this.length);
checkOffset(this, offset, byteLength, this.length);

var val = this[offset];
var mul = 1;
Expand All @@ -687,7 +777,7 @@ Buffer.prototype.readIntBE = function(offset, byteLength, noAssert) {
offset = offset >>> 0;
byteLength = byteLength >>> 0;
if (!noAssert)
checkOffset(offset, byteLength, this.length);
checkOffset(this, offset, byteLength, this.length);

var i = byteLength;
var mul = 1;
Expand All @@ -706,7 +796,7 @@ Buffer.prototype.readIntBE = function(offset, byteLength, noAssert) {
Buffer.prototype.readInt8 = function(offset, noAssert) {
offset = offset >>> 0;
if (!noAssert)
checkOffset(offset, 1, this.length);
checkOffset(this, offset, 1, this.length);
var val = this[offset];
return !(val & 0x80) ? val : (0xff - val + 1) * -1;
};
Expand All @@ -715,7 +805,7 @@ Buffer.prototype.readInt8 = function(offset, noAssert) {
Buffer.prototype.readInt16LE = function(offset, noAssert) {
offset = offset >>> 0;
if (!noAssert)
checkOffset(offset, 2, this.length);
checkOffset(this, offset, 2, this.length);
var val = this[offset] | (this[offset + 1] << 8);
return (val & 0x8000) ? val | 0xFFFF0000 : val;
};
Expand All @@ -724,7 +814,7 @@ Buffer.prototype.readInt16LE = function(offset, noAssert) {
Buffer.prototype.readInt16BE = function(offset, noAssert) {
offset = offset >>> 0;
if (!noAssert)
checkOffset(offset, 2, this.length);
checkOffset(this, offset, 2, this.length);
var val = this[offset + 1] | (this[offset] << 8);
return (val & 0x8000) ? val | 0xFFFF0000 : val;
};
Expand All @@ -733,7 +823,7 @@ Buffer.prototype.readInt16BE = function(offset, noAssert) {
Buffer.prototype.readInt32LE = function(offset, noAssert) {
offset = offset >>> 0;
if (!noAssert)
checkOffset(offset, 4, this.length);
checkOffset(this, offset, 4, this.length);

return (this[offset]) |
(this[offset + 1] << 8) |
Expand All @@ -745,7 +835,7 @@ Buffer.prototype.readInt32LE = function(offset, noAssert) {
Buffer.prototype.readInt32BE = function(offset, noAssert) {
offset = offset >>> 0;
if (!noAssert)
checkOffset(offset, 4, this.length);
checkOffset(this, offset, 4, this.length);

return (this[offset] << 24) |
(this[offset + 1] << 16) |
Expand All @@ -757,31 +847,31 @@ Buffer.prototype.readInt32BE = function(offset, noAssert) {
Buffer.prototype.readFloatLE = function readFloatLE(offset, noAssert) {
offset = offset >>> 0;
if (!noAssert)
checkOffset(offset, 4, this.length);
checkOffset(this, offset, 4, this.length);
return binding.readFloatLE(this, offset);
};


Buffer.prototype.readFloatBE = function readFloatBE(offset, noAssert) {
offset = offset >>> 0;
if (!noAssert)
checkOffset(offset, 4, this.length);
checkOffset(this, offset, 4, this.length);
return binding.readFloatBE(this, offset);
};


Buffer.prototype.readDoubleLE = function readDoubleLE(offset, noAssert) {
offset = offset >>> 0;
if (!noAssert)
checkOffset(offset, 8, this.length);
checkOffset(this, offset, 8, this.length);
return binding.readDoubleLE(this, offset);
};


Buffer.prototype.readDoubleBE = function readDoubleBE(offset, noAssert) {
offset = offset >>> 0;
if (!noAssert)
checkOffset(offset, 8, this.length);
checkOffset(this, offset, 8, this.length);
return binding.readDoubleBE(this, offset);
};

Expand Down
Loading