From a2f70da4c9bf14a6e0c4a17cac11a5ab555ce90f Mon Sep 17 00:00:00 2001 From: Matt Ranney Date: Thu, 24 Jun 2010 12:26:23 -0700 Subject: [PATCH] Buffer.copy should copy through sourceEnd, as specified. Improve test-buffer.js to cover all copy error cases. Fix off by one error in string_decoder. --- doc/api.markdown | 2 +- lib/string_decoder.js | 4 +- src/node_buffer.cc | 25 +++++++---- test/simple/test-buffer.js | 90 ++++++++++++++++++++++++++++++++++++-- 4 files changed, 106 insertions(+), 15 deletions(-) diff --git a/doc/api.markdown b/doc/api.markdown index b412cf997e985e..b3947da8f32d8d 100644 --- a/doc/api.markdown +++ b/doc/api.markdown @@ -177,7 +177,7 @@ into `buf2`, starting at the 8th byte in `buf2`. buf1.copy(buf2, 8, 16, 20); console.log(buf2.toString('ascii', 0, 25)); - // !!!!!!!!qrst!!!!!!!!!!!!! + // !!!!!!!!qrstu!!!!!!!!!!!! ### buffer.slice(start, end) diff --git a/lib/string_decoder.js b/lib/string_decoder.js index 2c7ace0ae5bd51..7bae2fc47bb3a3 100644 --- a/lib/string_decoder.js +++ b/lib/string_decoder.js @@ -25,7 +25,7 @@ StringDecoder.prototype.write = function (buffer) { : buffer.length; // add the new bytes to the char buffer - buffer.copy(this.charBuffer, this.charReceived, 0, i); + buffer.copy(this.charBuffer, this.charReceived, 0, (i - 1)); this.charReceived += i; if (this.charReceived < this.charLength) { @@ -79,7 +79,7 @@ StringDecoder.prototype.write = function (buffer) { } // buffer the incomplete character bytes we got - buffer.copy(this.charBuffer, 0, buffer.length - i, buffer.length); + buffer.copy(this.charBuffer, 0, buffer.length - i, (buffer.length - 1)); this.charReceived = i; if (buffer.length - i > 0) { diff --git a/src/node_buffer.cc b/src/node_buffer.cc index 5682f93791b6d1..9a13b1ef64f554 100644 --- a/src/node_buffer.cc +++ b/src/node_buffer.cc @@ -298,34 +298,41 @@ Handle Buffer::Copy(const Arguments &args) { ssize_t target_start = args[1]->Int32Value(); ssize_t source_start = args[2]->Int32Value(); ssize_t source_end = args[3]->IsInt32() ? args[3]->Int32Value() - : source->length(); + : (source->length() - 1); if (source_end < source_start) { return ThrowException(Exception::Error(String::New( "sourceEnd < sourceStart"))); } - if (target_start < 0 || target_start > target->length()) { + if (target_start < 0 || target_start >= target->length()) { return ThrowException(Exception::Error(String::New( "targetStart out of bounds"))); } - if (source_start < 0 || source_start > source->length()) { + if (source_start < 0 || source_start >= source->length()) { return ThrowException(Exception::Error(String::New( "sourceStart out of bounds"))); } - if (source_end < 0 || source_end > source->length()) { + if (source_end < 0 || source_end >= source->length()) { return ThrowException(Exception::Error(String::New( "sourceEnd out of bounds"))); } - ssize_t to_copy = MIN(source_end - source_start, - target->length() - target_start); + ssize_t to_copy = MIN( (source_end - source_start + 1), + (target->length() - target_start) ); - memcpy((void*)(target->data() + target_start), - (const void*)(source->data() + source_start), - to_copy); + if (source->handle_->StrictEquals(target->handle_)) { + // need to use slightly slower memmove is the ranges might overlap + memmove((void*)(target->data() + target_start), + (const void*)(source->data() + source_start), + to_copy); + } else { + memcpy((void*)(target->data() + target_start), + (const void*)(source->data() + source_start), + to_copy); + } return scope.Close(Integer::New(to_copy)); } diff --git a/test/simple/test-buffer.js b/test/simple/test-buffer.js index 949b5b844ea981..455efa6d3cf5d9 100644 --- a/test/simple/test-buffer.js +++ b/test/simple/test-buffer.js @@ -6,7 +6,7 @@ var Buffer = require('buffer').Buffer; var b = new Buffer(1024); console.log("b.length == " + b.length); -assert.equal(1024, b.length); +assert.strictEqual(1024, b.length); for (var i = 0; i < 1024; i++) { assert.ok(b[i] >= 0); @@ -19,12 +19,96 @@ for (var i = 0; i < 1024; i++) { var c = new Buffer(512); -var copied = b.copy(c, 0, 0, 512); -assert.equal(512, copied); +// copy 512 bytes, from 0 to 511. +var copied = b.copy(c, 0, 0, 511); +console.log("copied " + copied + " bytes from b into c"); +assert.strictEqual(512, copied); for (var i = 0; i < c.length; i++) { 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, 512); +console.log("copied " + copied + " bytes from b into c"); +assert.strictEqual(512, copied); +for (var i = 0; i < c.length; i++) { + assert.equal(i % 256, c[i]); +} + +// copy all of c back into b, without specifying sourceEnd +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]); +} + +// copy 768 bytes from b into b +var copied = b.copy(b, 0, 256, 1023); +console.log("copied " + copied + " bytes from b into c"); +assert.strictEqual(768, copied); +for (var i = 0; i < c.length; i++) { + assert.equal(i % 256, c[i]); +} + +var caught_error = null; + +// 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); + +// 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); + +// 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, 1024); +} catch (err) { + caught_error = err; +} +assert.strictEqual('sourceStart out of bounds', 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, 1024); +} catch (err) { + caught_error = err; +} +assert.strictEqual('sourceEnd out of bounds', caught_error.message); var asciiString = "hello world";