Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Fix overflow issues on writing/reading integers #11

Merged
merged 1 commit into from

2 participants

Tõnis Tiigi Romain Beauxis
Tõnis Tiigi

buffer-browserify does not handle writing and reading out of bounds correctly.

  • If you write value out-of-bounds and read it back you always get the full value back. Nothing is cut off.
  • If you write value out-of-bounds it can leak to the next buffer in SlowBuffer.

This PR only fixes Integers. Floats probably have same issues.

Romain Beauxis
Owner

Hi and thanks for you work!

Is the overflow present in node's original module? Even though it is a bug, I want to stick to node's implementation as much as possible so that there's no surprise when running code on browserify that has been written for node..

Tõnis Tiigi

Yes, this is the way node handles overflow. I also added a testcase that compares results from node to this module.

The implementation is different because in node its based on fixed length typed-array and there is no way to write over the length of the buffer. In buffer-browserify it just directly accesses slowbuffer and length has no special meaning.

Romain Beauxis
Owner

Oh, my bad I did not see the test. Merging, thanks!

Romain Beauxis toots merged commit 0ab6b0e into from
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 27, 2013
  1. Tõnis Tiigi
This page is out of date. Refresh to see the latest.
Showing with 70 additions and 25 deletions.
  1. +37 −25 index.js
  2. +33 −0 test/buffer.js
62 index.js
View
@@ -689,6 +689,8 @@ Buffer.prototype.readUInt8 = function(offset, noAssert) {
'Trying to read beyond buffer length');
}
+ if (offset >= buffer.length) return;
+
return buffer.parent[buffer.offset + offset];
};
@@ -707,12 +709,18 @@ function readUInt16(buffer, offset, isBigEndian, noAssert) {
'Trying to read beyond buffer length');
}
+ if (offset >= buffer.length) return 0;
+
if (isBigEndian) {
val = buffer.parent[buffer.offset + offset] << 8;
- val |= buffer.parent[buffer.offset + offset + 1];
+ if (offset + 1 < buffer.length) {
+ val |= buffer.parent[buffer.offset + offset + 1];
+ }
} else {
val = buffer.parent[buffer.offset + offset];
- val |= buffer.parent[buffer.offset + offset + 1] << 8;
+ if (offset + 1 < buffer.length) {
+ val |= buffer.parent[buffer.offset + offset + 1] << 8;
+ }
}
return val;
@@ -740,16 +748,24 @@ function readUInt32(buffer, offset, isBigEndian, noAssert) {
'Trying to read beyond buffer length');
}
+ if (offset >= buffer.length) return 0;
+
if (isBigEndian) {
- val = buffer.parent[buffer.offset + offset + 1] << 16;
- val |= buffer.parent[buffer.offset + offset + 2] << 8;
- val |= buffer.parent[buffer.offset + offset + 3];
+ if (offset + 1 < buffer.length)
+ val = buffer.parent[buffer.offset + offset + 1] << 16;
+ if (offset + 2 < buffer.length)
+ val |= buffer.parent[buffer.offset + offset + 2] << 8;
+ if (offset + 3 < buffer.length)
+ val |= buffer.parent[buffer.offset + offset + 3];
val = val + (buffer.parent[buffer.offset + offset] << 24 >>> 0);
} else {
- val = buffer.parent[buffer.offset + offset + 2] << 16;
- val |= buffer.parent[buffer.offset + offset + 1] << 8;
+ if (offset + 2 < buffer.length)
+ val = buffer.parent[buffer.offset + offset + 2] << 16;
+ if (offset + 1 < buffer.length)
+ val |= buffer.parent[buffer.offset + offset + 1] << 8;
val |= buffer.parent[buffer.offset + offset];
- val = val + (buffer.parent[buffer.offset + offset + 3] << 24 >>> 0);
+ if (offset + 3 < buffer.length)
+ val = val + (buffer.parent[buffer.offset + offset + 3] << 24 >>> 0);
}
return val;
@@ -821,6 +837,8 @@ Buffer.prototype.readInt8 = function(offset, noAssert) {
'Trying to read beyond buffer length');
}
+ if (offset >= buffer.length) return;
+
neg = buffer.parent[buffer.offset + offset] & 0x80;
if (!neg) {
return (buffer.parent[buffer.offset + offset]);
@@ -971,7 +989,9 @@ Buffer.prototype.writeUInt8 = function(value, offset, noAssert) {
verifuint(value, 0xff);
}
- buffer.parent[buffer.offset + offset] = value;
+ if (offset < buffer.length) {
+ buffer.parent[buffer.offset + offset] = value;
+ }
};
function writeUInt16(buffer, value, offset, isBigEndian, noAssert) {
@@ -991,13 +1011,12 @@ function writeUInt16(buffer, value, offset, isBigEndian, noAssert) {
verifuint(value, 0xffff);
}
- if (isBigEndian) {
- buffer.parent[buffer.offset + offset] = (value & 0xff00) >>> 8;
- buffer.parent[buffer.offset + offset + 1] = value & 0x00ff;
- } else {
- buffer.parent[buffer.offset + offset + 1] = (value & 0xff00) >>> 8;
- buffer.parent[buffer.offset + offset] = value & 0x00ff;
+ for (var i = 0; i < Math.min(buffer.length - offset, 2); i++) {
+ buffer.parent[buffer.offset + offset + i] =
+ (value & (0xff << (8 * (isBigEndian ? 1 - i : i)))) >>>
+ (isBigEndian ? 1 - i : i) * 8;
}
+
}
Buffer.prototype.writeUInt16LE = function(value, offset, noAssert) {
@@ -1025,16 +1044,9 @@ function writeUInt32(buffer, value, offset, isBigEndian, noAssert) {
verifuint(value, 0xffffffff);
}
- if (isBigEndian) {
- buffer.parent[buffer.offset + offset] = (value >>> 24) & 0xff;
- buffer.parent[buffer.offset + offset + 1] = (value >>> 16) & 0xff;
- buffer.parent[buffer.offset + offset + 2] = (value >>> 8) & 0xff;
- buffer.parent[buffer.offset + offset + 3] = value & 0xff;
- } else {
- buffer.parent[buffer.offset + offset + 3] = (value >>> 24) & 0xff;
- buffer.parent[buffer.offset + offset + 2] = (value >>> 16) & 0xff;
- buffer.parent[buffer.offset + offset + 1] = (value >>> 8) & 0xff;
- buffer.parent[buffer.offset + offset] = value & 0xff;
+ for (var i = 0; i < Math.min(buffer.length - offset, 4); i++) {
+ buffer.parent[buffer.offset + offset + i] =
+ (value >>> (isBigEndian ? 3 - i : i) * 8) & 0xff;
}
}
33 test/buffer.js
View
@@ -100,6 +100,39 @@ test("hex of write{Uint,Int}{8,16,32}{LE,BE}", function (t) {
t.end();
});
+test("hex of write{Uint,Int}{8,16,32}{LE,BE} with overflow", function (t) {
+ t.plan(3*(2*2*2+2));
+ ["UInt","Int"].forEach(function(x){
+ [8,16,32].forEach(function(y){
+ var endianesses = (y === 8) ? [""] : ["LE","BE"];
+ endianesses.forEach(function(z){
+ var v1 = new buffer.Buffer(y / 8 - 1);
+ var v2 = new Buffer(y / 8 - 1);
+ var next = new buffer.Buffer(4);
+ next.writeUInt32BE(0, 0);
+ var writefn = "write" + x + y + z;
+ var val = (x === "Int") ? -3 : 3;
+ v1[writefn](val, 0, true);
+ v2[writefn](val, 0, true);
+ t.equal(
+ v1.toString("hex"),
+ v2.toString("hex")
+ );
+ // check that nothing leaked to next buffer.
+ t.equal(next.readUInt32BE(0), 0);
+ // check that no bytes are read from next buffer.
+ next.writeInt32BE(~0, 0);
+ var readfn = "read" + x + y + z;
+ t.equal(
+ v1[readfn](0, true),
+ v2[readfn](0, true)
+ );
+ });
+ });
+ });
+ t.end();
+});
+
test("concat() a varying number of buffers", function (t) {
t.plan(5);
var zero = [];
Something went wrong with that request. Please try again.