From 49175e6ae293fdb564fa0e91fdb104a3a1f248f5 Mon Sep 17 00:00:00 2001
From: Trevor Norris <trev.norris@gmail.com>
Date: Tue, 22 Jan 2013 10:48:54 -0800
Subject: [PATCH] buffer: clean up copy() asserts and tests

Argument checks were simplified by setting all undefined/NaN or out of
bounds values equal to their defaults.

Also copy() tests had a flaw that each buffer had the same bit pattern at
the same offset. So even if the copy failed, the bit-by-bit comparison
would have still been true. This was fixed by filling each buffer with a
unique value before copy operations.
---
 doc/api/buffer.markdown    |   3 +
 lib/buffer.js              |  39 +++-----
 test/simple/test-buffer.js | 192 ++++++++++++++++++++++---------------
 3 files changed, 130 insertions(+), 104 deletions(-)

diff --git a/doc/api/buffer.markdown b/doc/api/buffer.markdown
index 34366262ab4..9d924490203 100644
--- a/doc/api/buffer.markdown
+++ b/doc/api/buffer.markdown
@@ -239,6 +239,9 @@ Does copy between buffers. The source and target regions can be overlapped.
 `targetStart` and `sourceStart` default to `0`.
 `sourceEnd` defaults to `buffer.length`.
 
+All values passed that are `undefined`/`NaN` or are out of bounds are set equal
+to their respective defaults.
+
 Example: build two Buffers, then copy `buf1` from byte 16 through byte 19
 into `buf2`, starting at the 8th byte in `buf2`.
 
diff --git a/lib/buffer.js b/lib/buffer.js
index 098406bc46d..f5d7c731d0c 100644
--- a/lib/buffer.js
+++ b/lib/buffer.js
@@ -541,37 +541,26 @@ Buffer.concat = function(list, length) {
 
 // copy(targetBuffer, targetStart=0, sourceStart=0, sourceEnd=buffer.length)
 Buffer.prototype.copy = function(target, target_start, start, end) {
-  var source = this;
-  start || (start = 0);
-  end || (end = this.length);
-  target_start || (target_start = 0);
-
-  if (end < start) throw new Error('sourceEnd < sourceStart');
+  // set undefined/NaN or out of bounds values equal to their default
+  if (!(target_start >= 0)) target_start = 0;
+  if (!(start >= 0)) start = 0;
+  if (!(end < this.length)) end = this.length;
 
   // Copy 0 bytes; we're done
-  if (end === start) return 0;
-  if (target.length == 0 || source.length == 0) return 0;
+  if (end === start ||
+      target.length === 0 ||
+      this.length === 0 ||
+      start > this.length)
+    return 0;
 
-  if (target_start < 0 || target_start >= target.length) {
-    throw new Error('targetStart out of bounds');
-  }
+  if (end < start)
+    throw new RangeError('sourceEnd < sourceStart');
 
-  if (start < 0 || start >= source.length) {
-    throw new Error('sourceStart out of bounds');
-  }
+  if (target_start >= target.length)
+    throw new RangeError('targetStart out of bounds');
 
-  if (end < 0 || end > source.length) {
-    throw new Error('sourceEnd out of bounds');
-  }
-
-  // Are we oob?
-  if (end > this.length) {
-    end = this.length;
-  }
-
-  if (target.length - target_start < end - start) {
+  if (target.length - target_start < end - start)
     end = target.length - target_start + start;
-  }
 
   return this.parent.copy(target.parent || target,
                           target_start + (target.offset || 0),
diff --git a/test/simple/test-buffer.js b/test/simple/test-buffer.js
index 387973d9366..c367c412e7d 100644
--- a/test/simple/test-buffer.js
+++ b/test/simple/test-buffer.js
@@ -25,6 +25,9 @@ var assert = require('assert');
 var SlowBuffer = require('buffer').SlowBuffer;
 var Buffer = require('buffer').Buffer;
 
+// counter to ensure unique value is always copied
+var cntr = 0;
+
 // Regression test for segfault introduced in commit e501ce4.
 ['base64','binary','ucs2','utf8','ascii'].forEach(function(encoding) {
   var buf = new SlowBuffer(0);
@@ -33,18 +36,18 @@ var Buffer = require('buffer').Buffer;
 
 var b = Buffer(1024); // safe constructor
 
-console.log('b.length == ' + b.length);
+console.log('b.length == %d', b.length);
 assert.strictEqual(1024, b.length);
 
 b[0] = -1;
-assert.equal(b[0], 255);
+assert.strictEqual(b[0], 255);
 
 for (var i = 0; i < 1024; i++) {
   b[i] = i % 256;
 }
 
 for (var i = 0; i < 1024; i++) {
-  assert.equal(i % 256, b[i]);
+  assert.strictEqual(i % 256, b[i]);
 }
 
 var c = new Buffer(512);
@@ -52,93 +55,134 @@ console.log('c.length == %d', c.length);
 assert.strictEqual(512, c.length);
 
 // copy 512 bytes, from 0 to 512.
+b.fill(++cntr);
+c.fill(++cntr);
 var copied = b.copy(c, 0, 0, 512);
-console.log('copied ' + copied + ' bytes from b into c');
-assert.equal(512, copied);
-for (var i = 0; i < c.length; i++) {
-  common.print('.');
-  assert.equal(i % 256, c[i]);
-}
-console.log('');
-
-// try to copy 513 bytes, and hope we don't overrun c, which is only 512 long
-var copied = b.copy(c, 0, 0, 513);
-console.log('copied ' + copied + ' bytes from b into c');
+console.log('copied %d bytes from b into c', copied);
 assert.strictEqual(512, copied);
 for (var i = 0; i < c.length; i++) {
-  assert.equal(i % 256, c[i]);
+  assert.strictEqual(b[i], c[i]);
 }
 
-// copy all of c back into b, without specifying sourceEnd
+// copy c into b, without specifying sourceEnd
+b.fill(++cntr);
+c.fill(++cntr);
 var copied = c.copy(b, 0, 0);
-console.log('copied ' + copied + ' bytes from c back into b');
-assert.strictEqual(512, copied);
-for (var i = 0; i < b.length; i++) {
-  assert.equal(i % 256, b[i]);
+console.log('copied %d bytes from c into b w/o sourceEnd', copied);
+assert.strictEqual(c.length, copied);
+for (var i = 0; i < c.length; i++) {
+  assert.strictEqual(c[i], b[i]);
+}
+
+// copy c into b, without specifying sourceStart
+b.fill(++cntr);
+c.fill(++cntr);
+var copied = c.copy(b, 0);
+console.log('copied %d bytes from c into b w/o sourceStart', copied);
+assert.strictEqual(c.length, copied);
+for (var i = 0; i < c.length; i++) {
+  assert.strictEqual(c[i], b[i]);
+}
+
+// copy longer buffer b to shorter c without targetStart
+b.fill(++cntr);
+c.fill(++cntr);
+var copied = b.copy(c);
+console.log('copied %d bytes from b into c w/o targetStart', copied);
+assert.strictEqual(c.length, copied);
+for (var i = 0; i < c.length; i++) {
+  assert.strictEqual(b[i], c[i]);
+}
+
+// copy starting near end of b to c
+b.fill(++cntr);
+c.fill(++cntr);
+var copied = b.copy(c, 0, b.length - Math.floor(c.length / 2));
+console.log('copied %d bytes from end of b into beginning of c', copied);
+assert.strictEqual(Math.floor(c.length / 2), copied);
+for (var i = 0; i < Math.floor(c.length / 2); i++) {
+  assert.strictEqual(b[b.length - Math.floor(c.length / 2) + i], c[i]);
+}
+for (var i = Math.floor(c.length /2) + 1; i < c.length; i++) {
+  assert.strictEqual(c[c.length-1], c[i]);
+}
+
+// try to copy 513 bytes, and check we don't overrun c
+b.fill(++cntr);
+c.fill(++cntr);
+var copied = b.copy(c, 0, 0, 513);
+console.log('copied %d bytes from b trying to overrun c', copied);
+assert.strictEqual(c.length, copied);
+for (var i = 0; i < c.length; i++) {
+  assert.strictEqual(b[i], c[i]);
 }
 
 // copy 768 bytes from b into b
+b.fill(++cntr);
+b.fill(++cntr, 256);
 var copied = b.copy(b, 0, 256, 1024);
-console.log('copied ' + copied + ' bytes from b into c');
+console.log('copied %d bytes from b into b', copied);
 assert.strictEqual(768, copied);
+for (var i = 0; i < b.length; i++) {
+  assert.strictEqual(cntr, b[i]);
+}
+
+// copy from b to c with negative targetStart
+b.fill(++cntr);
+c.fill(++cntr);
+var copied = b.copy(c, -1);
+console.log('copied %d bytes from b into c w/ negative targetStart', copied);
+assert.strictEqual(c.length, copied);
 for (var i = 0; i < c.length; i++) {
-  assert.equal(i % 256, c[i]);
+  assert.strictEqual(b[i], c[i]);
 }
 
-// copy from fast to slow buffer
+// copy from b to c with negative sourceStart
+b.fill(++cntr);
+c.fill(++cntr);
+var copied = b.copy(c, 0, -1);
+assert.strictEqual(c.length, copied);
+console.log('copied %d bytes from b into c w/ negative sourceStart', copied);
+for (var i = 0; i < c.length; i++) {
+  assert.strictEqual(b[i], c[i]);
+}
+
+// check sourceEnd resets to targetEnd if former is greater than the latter
+b.fill(++cntr);
+c.fill(++cntr);
+var copied = b.copy(c, 0, 0, 1025);
+console.log('copied %d bytes from b into c', copied);
+for (var i = 0; i < c.length; i++) {
+  assert.strictEqual(b[i], c[i]);
+}
+
+// copy from fast buffer to slow buffer without parameters
 var sb = new SlowBuffer(b.length);
+sb.fill(++cntr, 0, sb.length);
+b.fill(++cntr);
 var copied = b.copy(sb);
-console.log('copied %d bytes from b into sb');
-for (var i = 0; i < sb.length; i++) {
-  assert.strictEqual(sb[i], b[i]);
+console.log('copied %d bytes from fast buffer to slow buffer', copied);
+for (var i = 0 ; i < b.length; i++) {
+  assert.strictEqual(b[i], sb[i]);
 }
 
-var caught_error = null;
+// throw with negative sourceEnd
+console.log('test copy at negative sourceEnd');
+assert.throws(function() {
+  b.copy(c, 0, 0, -1);
+}, RangeError);
 
-// try to copy from before the beginning of b
-caught_error = null;
-try {
-  var copied = b.copy(c, 0, 100, 10);
-} catch (err) {
-  caught_error = err;
-}
-assert.strictEqual('sourceEnd < sourceStart', caught_error.message);
+// throw when sourceStart is greater than sourceEnd
+assert.throws(function() {
+  b.copy(c, 0, 100, 10);
+}, RangeError);
 
-// try to copy to before the beginning of c
-caught_error = null;
-try {
-  var copied = b.copy(c, -1, 0, 10);
-} catch (err) {
-  caught_error = err;
-}
-assert.strictEqual('targetStart out of bounds', caught_error.message);
+// throw attempting to copy after end of c
+assert.throws(function() {
+  b.copy(c, 512, 0, 10);
+}, RangeError);
 
-// try to copy to after the end of c
-caught_error = null;
-try {
-  var copied = b.copy(c, 512, 0, 10);
-} catch (err) {
-  caught_error = err;
-}
-assert.strictEqual('targetStart out of bounds', caught_error.message);
-
-// try to copy starting before the beginning of b
-caught_error = null;
-try {
-  var copied = b.copy(c, 0, -1, 1);
-} catch (err) {
-  caught_error = err;
-}
-assert.strictEqual('sourceStart out of bounds', caught_error.message);
-
-// try to copy starting after the end of b
-caught_error = null;
-try {
-  var copied = b.copy(c, 0, 1024, 1025);
-} catch (err) {
-  caught_error = err;
-}
-assert.strictEqual('sourceStart out of bounds', caught_error.message);
+var caught_error;
 
 // invalid encoding for Buffer.toString
 caught_error = null;
@@ -158,16 +202,6 @@ try {
 }
 assert.strictEqual('Unknown encoding: invalid', caught_error.message);
 
-// a too-low sourceEnd will get caught by earlier checks
-
-// try to copy ending after the end of b
-try {
-  var copied = b.copy(c, 0, 1023, 1025);
-} catch (err) {
-  caught_error = err;
-}
-assert.strictEqual('sourceEnd out of bounds', caught_error.message);
-
 // try to create 0-length buffers
 new Buffer('');
 new Buffer('', 'ascii');