Fix issues surrounding type checking for Uint8Arrays and Buffers (#346)

Author: chjj

index.js

@@ -317,6 +317,7 @@ function fromArrayBuffer (array, byteOffset, length) {
 
 function fromObject (obj) {
   if (Buffer.isBuffer(obj)) {
+    // Note: Probably not necessary anymore.
     const len = checked(obj.length) | 0
     const buf = createBuffer(len)
 
@@ -363,9 +364,7 @@ Buffer.isBuffer = function isBuffer (b) {
 }
 
 Buffer.compare = function compare (a, b) {
-  if (isInstance(a, Uint8Array)) a = Buffer.from(a, a.offset, a.byteLength)
-  if (isInstance(b, Uint8Array)) b = Buffer.from(b, b.offset, b.byteLength)
-  if (!Buffer.isBuffer(a) || !Buffer.isBuffer(b)) {
+  if (!isInstance(a, Uint8Array) || !isInstance(b, Uint8Array)) {
     throw new TypeError(
       'The "buf1", "buf2" arguments must be one of type Buffer or Uint8Array'
     )
@@ -428,37 +427,28 @@ Buffer.concat = function concat (list, length) {
   const buffer = Buffer.allocUnsafe(length)
   let pos = 0
   for (i = 0; i < list.length; ++i) {
-    let buf = list[i]
-    if (isInstance(buf, Uint8Array)) {
-      if (pos + buf.length > buffer.length) {
-        if (!Buffer.isBuffer(buf)) {
-          buf = Buffer.from(buf.buffer, buf.byteOffset, buf.byteLength)
-        }
-        buf.copy(buffer, pos)
-      } else {
-        Uint8Array.prototype.set.call(
-          buffer,
-          buf,
-          pos
-        )
-      }
-    } else if (!Buffer.isBuffer(buf)) {
+    const buf = list[i]
+    if (!isInstance(buf, Uint8Array)) {
       throw new TypeError('"list" argument must be an Array of Buffers')
-    } else {
-      buf.copy(buffer, pos)
     }
+    if (pos + buf.length > buffer.length) {
+      buffer.set(buf.subarray(0, buffer.length - pos), pos)
+      break
+    }
+    buffer.set(buf, pos)
     pos += buf.length
   }
   return buffer
 }
 
 function byteLength (string, encoding) {
-  if (Buffer.isBuffer(string)) {
-    return string.length
-  }
   if (ArrayBuffer.isView(string) || isInstance(string, ArrayBuffer)) {
     return string.byteLength
   }
+  if (typeof SharedArrayBuffer !== 'undefined' &&
+      isInstance(string, SharedArrayBuffer)) {
+    return string.byteLength
+  }
   if (typeof string !== 'string') {
     throw new TypeError(
       'The "string" argument must be one of type string, Buffer, or ArrayBuffer. ' +
@@ -632,7 +622,6 @@ Buffer.prototype.toString = function toString () {
 Buffer.prototype.toLocaleString = Buffer.prototype.toString
 
 Buffer.prototype.equals = function equals (b) {
-  if (!Buffer.isBuffer(b)) throw new TypeError('Argument must be a Buffer')
   if (this === b) return true
   return Buffer.compare(this, b) === 0
 }
@@ -649,10 +638,7 @@ if (customInspectSymbol) {
 }
 
 Buffer.prototype.compare = function compare (target, start, end, thisStart, thisEnd) {
-  if (isInstance(target, Uint8Array)) {
-    target = Buffer.from(target, target.offset, target.byteLength)
-  }
-  if (!Buffer.isBuffer(target)) {
+  if (!isInstance(target, Uint8Array)) {
     throw new TypeError(
       'The "target" argument must be one of type Buffer or Uint8Array. ' +
       'Received type ' + (typeof target)
@@ -697,13 +683,10 @@ Buffer.prototype.compare = function compare (target, start, end, thisStart, this
   let y = end - start
   const len = Math.min(x, y)
 
-  const thisCopy = this.slice(thisStart, thisEnd)
-  const targetCopy = target.slice(start, end)
-
   for (let i = 0; i < len; ++i) {
-    if (thisCopy[i] !== targetCopy[i]) {
-      x = thisCopy[i]
-      y = targetCopy[i]
+    if (this[thisStart + i] !== target[start + i]) {
+      x = this[thisStart + i]
+      y = target[start + i]
       break
     }
   }
@@ -1719,7 +1702,7 @@ Buffer.prototype.writeDoubleBE = function writeDoubleBE (value, offset, noAssert
 
 // copy(targetBuffer, targetStart=0, sourceStart=0, sourceEnd=buffer.length)
 Buffer.prototype.copy = function copy (target, targetStart, start, end) {
-  if (!Buffer.isBuffer(target)) throw new TypeError('argument should be a Buffer')
+  if (!isInstance(target, Uint8Array)) throw new TypeError('argument should be a Buffer')
   if (!start) start = 0
   if (!end && end !== 0) end = this.length
   if (targetStart >= target.length) targetStart = target.length
@@ -1814,7 +1797,7 @@ Buffer.prototype.fill = function fill (val, start, end, encoding) {
       this[i] = val
     }
   } else {
-    const bytes = Buffer.isBuffer(val)
+    const bytes = isInstance(val, Uint8Array)
       ? val
       : Buffer.from(val, encoding)
     const len = bytes.length
@@ -2100,7 +2083,8 @@ function blitBuffer (src, dst, offset, length) {
 function isInstance (obj, type) {
   return obj instanceof type ||
     (obj != null && obj.constructor != null && obj.constructor.name != null &&
-      obj.constructor.name === type.name)
+      obj.constructor.name === type.name) ||
+    (type === Uint8Array && Buffer.isBuffer(obj))
 }
 function numberIsNaN (obj) {
   // For IE11 support

test/typing.js

@@ -0,0 +1,179 @@
+'use strict'
+
+const Buffer = require('../').Buffer
+const test = require('tape')
+const vm = require('vm')
+
+// Get a Uint8Array and Buffer constructor from another context.
+const code = `
+  'use strict'
+  function Buffer (...args) {
+    const buf = new Uint8Array(...args)
+    Object.setPrototypeOf(buf, Buffer.prototype)
+    return buf
+  }
+  Object.setPrototypeOf(Buffer.prototype, Uint8Array.prototype)
+  Object.setPrototypeOf(Buffer, Uint8Array)
+  Buffer.prototype._isBuffer = true
+  exports.Uint8Array = Uint8Array
+  exports.Buffer = Buffer
+`
+
+const context = {}
+
+// Should work in browserify.
+vm.runInNewContext(code, { exports: context })
+
+const arrays = [context.Uint8Array, context.Buffer]
+
+// Extracted from the index.js code for testing purposes.
+function isInstance (obj, type) {
+  return (obj instanceof type) ||
+         (obj != null &&
+          obj.constructor != null &&
+          obj.constructor.name != null &&
+          obj.constructor.name === type.name) ||
+         (type === Uint8Array && Buffer.isBuffer(obj))
+}
+
+test('Uint8Arrays and Buffers from other contexts', (t) => {
+  // Our buffer is considered a view.
+  t.ok(ArrayBuffer.isView(Buffer.alloc(0)))
+
+  for (const ForeignArray of arrays) {
+    const buf = new ForeignArray(1)
+
+    buf[0] = 1
+
+    // Prove that ArrayBuffer.isView and isInstance
+    // return true for objects from other contexts.
+    t.ok(!(buf instanceof Object))
+    t.ok(!(buf instanceof Uint8Array))
+    t.ok(!(buf instanceof Buffer))
+    t.ok(ArrayBuffer.isView(buf))
+
+    // Now returns true even for Buffers from other contexts:
+    t.ok(isInstance(buf, Uint8Array))
+
+    if (ForeignArray === context.Uint8Array) {
+      t.ok(!Buffer.isBuffer(buf))
+    } else {
+      t.ok(Buffer.isBuffer(buf))
+    }
+
+    // They even behave the same!
+    const copy = new Uint8Array(buf)
+
+    t.ok(copy instanceof Object)
+    t.ok(copy instanceof Uint8Array)
+    t.ok(ArrayBuffer.isView(copy))
+    t.equal(copy[0], 1)
+  }
+
+  t.end()
+})
+
+test('should instantiate from foreign arrays', (t) => {
+  for (const ForeignArray of arrays) {
+    const arr = new ForeignArray(2)
+
+    arr[0] = 1
+    arr[1] = 2
+
+    const buf = Buffer.from(arr)
+
+    t.equal(buf.toString('hex'), '0102')
+  }
+
+  t.end()
+})
+
+test('should do comparisons with foreign arrays', (t) => {
+  const a = Buffer.from([1, 2, 3])
+  const b = new context.Uint8Array(a)
+  const c = new context.Buffer(a)
+
+  t.equal(Buffer.byteLength(a), 3)
+  t.equal(Buffer.byteLength(b), 3)
+  t.equal(Buffer.byteLength(c), 3)
+  t.equal(b[0], 1)
+  t.equal(c[0], 1)
+
+  t.ok(a.equals(b))
+  t.ok(a.equals(c))
+  t.ok(a.compare(b) === 0)
+  t.ok(a.compare(c) === 0)
+  t.ok(Buffer.compare(a, b) === 0)
+  t.ok(Buffer.compare(a, c) === 0)
+  t.ok(Buffer.compare(b, c) === 0)
+  t.ok(Buffer.compare(c, b) === 0)
+
+  a[0] = 0
+
+  t.ok(!a.equals(b))
+  t.ok(!a.equals(c))
+  t.ok(a.compare(b) < 0)
+  t.ok(a.compare(c) < 0)
+  t.ok(Buffer.compare(a, b) < 0)
+  t.ok(Buffer.compare(a, c) < 0)
+
+  b[0] = 0
+
+  t.ok(Buffer.compare(b, c) < 0)
+  t.ok(Buffer.compare(c, b) > 0)
+
+  t.end()
+})
+
+test('should fill with foreign arrays', (t) => {
+  for (const ForeignArray of arrays) {
+    const buf = Buffer.alloc(4)
+    const arr = new ForeignArray(2)
+
+    arr[0] = 1
+    arr[1] = 2
+
+    buf.fill(arr)
+
+    t.equal(buf.toString('hex'), '01020102')
+  }
+
+  t.end()
+})
+
+test('should do concatenation with foreign arrays', (t) => {
+  for (const ForeignArray of arrays) {
+    const a = new ForeignArray(2)
+
+    a[0] = 1
+    a[1] = 2
+
+    const b = new ForeignArray(a)
+
+    {
+      const buf = Buffer.concat([a, b])
+      t.equal(buf.toString('hex'), '01020102')
+    }
+
+    {
+      const buf = Buffer.concat([a, b], 3)
+      t.equal(buf.toString('hex'), '010201')
+    }
+  }
+
+  t.end()
+})
+
+test('should copy on to foreign arrays', (t) => {
+  for (const ForeignArray of arrays) {
+    const a = Buffer.from([1, 2])
+    const b = new ForeignArray(2)
+
+    a.copy(b)
+
+    t.equal(b[0], 1)
+    t.equal(b[1], 2)
+  }
+
+  t.end()
+})