From 57ed3daebfe4700b14cd551f50240f1a634dbd64 Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Mon, 29 Sep 2014 17:29:59 -0700 Subject: [PATCH] buffer: fix and cleanup fill() Running fill() with an empty string would cause Node to hang indefinitely. Now it will return without having operated on the buffer. User facing function has been pulled into JS to perform all initial value checks and coercions. The C++ method has been placed on the "internal" object. Coerced non-string values to numbers to match v0.10 support. Simplified logic and changed a couple variable names. Added tests for fill() and moved them all to the beginning of buffer-test.js since many other tests depend on fill() working properly. Fixes: https://github.com/joyent/node/issues/8469 Signed-off-by: Trevor Norris --- lib/buffer.js | 23 ++++++++++++ src/env.h | 1 - src/node_buffer.cc | 48 ++++++++++--------------- test/simple/test-buffer.js | 72 ++++++++++++++++++++++++++------------ 4 files changed, 91 insertions(+), 53 deletions(-) diff --git a/lib/buffer.js b/lib/buffer.js index ed15915841..7fd297559b 100644 --- a/lib/buffer.js +++ b/lib/buffer.js @@ -295,6 +295,29 @@ Buffer.prototype.compare = function compare(b) { }; +Buffer.prototype.fill = function fill(val, start, end) { + start = start >> 0; + end = (end === undefined) ? this.length : end >> 0; + + if (start < 0 || end > this.length) + throw new RangeError('out of range index'); + if (end <= start) + return this; + + if (typeof val !== 'string') { + val = val >>> 0; + } else if (val.length === 1) { + var code = val.charCodeAt(0); + if (code < 256) + val = code; + } + + internal.fill(this, val, start, end); + + return this; +}; + + // XXX remove in v0.13 Buffer.prototype.get = util.deprecate(function get(offset) { offset = ~~offset; diff --git a/src/env.h b/src/env.h index 7fe132ce69..015dcd26e8 100644 --- a/src/env.h +++ b/src/env.h @@ -72,7 +72,6 @@ namespace node { V(buffer_string, "buffer") \ V(bytes_string, "bytes") \ V(bytes_parsed_string, "bytesParsed") \ - V(byte_length_string, "byteLength") \ V(callback_string, "callback") \ V(change_string, "change") \ V(close_string, "close") \ diff --git a/src/node_buffer.cc b/src/node_buffer.cc index dada1002ba..8be551d9b0 100644 --- a/src/node_buffer.cc +++ b/src/node_buffer.cc @@ -338,37 +338,31 @@ void Copy(const FunctionCallbackInfo &args) { } -// buffer.fill(value[, start][, end]); void Fill(const FunctionCallbackInfo& args) { - Environment* env = Environment::GetCurrent(args.GetIsolate()); - HandleScope scope(env->isolate()); + ARGS_THIS(args[0].As()) - ARGS_THIS(args.This()) - SLICE_START_END(args[1], args[2], obj_length) - args.GetReturnValue().Set(args.This()); + size_t start = args[2]->Uint32Value(); + size_t end = args[3]->Uint32Value(); + size_t length = end - start; + CHECK(length + start <= obj_length); - if (args[0]->IsNumber()) { - int value = args[0]->Uint32Value() & 255; + if (args[1]->IsNumber()) { + int value = args[1]->Uint32Value() & 255; memset(obj_data + start, value, length); return; } - node::Utf8Value at(args[0]); - size_t at_length = at.length(); + node::Utf8Value str(args[1]); + size_t str_length = str.length(); + size_t in_there = str_length; + char* ptr = obj_data + start + str_length; - // optimize single ascii character case - if (at_length == 1) { - int value = static_cast((*at)[0]); - memset(obj_data + start, value, length); + if (str_length == 0) return; - } - size_t in_there = at_length; - char* ptr = obj_data + start + at_length; + memcpy(obj_data + start, *str, MIN(str_length, length)); - memcpy(obj_data + start, *at, MIN(at_length, length)); - - if (at_length >= length) + if (str_length >= length) return; while (in_there < length - in_there) { @@ -660,7 +654,6 @@ void SetupBufferJS(const FunctionCallbackInfo& args) { NODE_SET_METHOD(proto, "writeFloatLE", WriteFloatLE); NODE_SET_METHOD(proto, "copy", Copy); - NODE_SET_METHOD(proto, "fill", Fill); // for backwards compatibility proto->Set(env->offset_string(), @@ -670,16 +663,11 @@ void SetupBufferJS(const FunctionCallbackInfo& args) { assert(args[1]->IsObject()); Local internal = args[1].As(); + ASSERT(internal->IsObject()); - Local byte_length = FunctionTemplate::New( - env->isolate(), ByteLength)->GetFunction(); - byte_length->SetName(env->byte_length_string()); - internal->Set(env->byte_length_string(), byte_length); - - Local compare = FunctionTemplate::New( - env->isolate(), Compare)->GetFunction(); - compare->SetName(env->compare_string()); - internal->Set(env->compare_string(), compare); + NODE_SET_METHOD(internal, "byteLength", ByteLength); + NODE_SET_METHOD(internal, "compare", Compare); + NODE_SET_METHOD(internal, "fill", Fill); } diff --git a/test/simple/test-buffer.js b/test/simple/test-buffer.js index 70cc5908af..4219a15be5 100644 --- a/test/simple/test-buffer.js +++ b/test/simple/test-buffer.js @@ -49,6 +49,56 @@ var c = new Buffer(512); console.log('c.length == %d', c.length); assert.strictEqual(512, c.length); +// First check Buffer#fill() works as expected. + +assert.throws(function() { + Buffer(8).fill('a', -1); +}); + +assert.throws(function() { + Buffer(8).fill('a', 0, 9); +}); + +// Make sure this doesn't hang indefinitely. +Buffer(8).fill(''); + +var buf = new Buffer(64); +buf.fill(10); +for (var i = 0; i < buf.length; i++) + assert.equal(buf[i], 10); + +buf.fill(11, 0, buf.length >> 1); +for (var i = 0; i < buf.length >> 1; i++) + assert.equal(buf[i], 11); +for (var i = (buf.length >> 1) + 1; i < buf.length; i++) + assert.equal(buf[i], 10); + +buf.fill('h'); +for (var i = 0; i < buf.length; i++) + assert.equal('h'.charCodeAt(0), buf[i]); + +buf.fill(0); +for (var i = 0; i < buf.length; i++) + assert.equal(0, buf[i]); + +buf.fill(null); +for (var i = 0; i < buf.length; i++) + assert.equal(0, buf[i]); + +buf.fill(1, 16, 32); +for (var i = 0; i < 16; i++) + assert.equal(0, buf[i]); +for (; i < 32; i++) + assert.equal(1, buf[i]); +for (; i < buf.length; i++) + assert.equal(0, buf[i]); + +var buf = new Buffer(10); +buf.fill('abc'); +assert.equal(buf.toString(), 'abcabcabca'); +buf.fill('է'); +assert.equal(buf.toString(), 'էէէէէ'); + // copy 512 bytes, from 0 to 512. b.fill(++cntr); c.fill(++cntr); @@ -642,28 +692,6 @@ assert.equal(0x6f, z[1]); assert.equal(0, Buffer('hello').slice(0, 0).length); -b = new Buffer(50); -b.fill('h'); -for (var i = 0; i < b.length; i++) { - assert.equal('h'.charCodeAt(0), b[i]); -} - -b.fill(0); -for (var i = 0; i < b.length; i++) { - assert.equal(0, b[i]); -} - -b.fill(1, 16, 32); -for (var i = 0; i < 16; i++) assert.equal(0, b[i]); -for (; i < 32; i++) assert.equal(1, b[i]); -for (; i < b.length; i++) assert.equal(0, b[i]); - -var buf = new Buffer(10); -buf.fill('abc'); -assert.equal(buf.toString(), 'abcabcabca'); -buf.fill('է'); -assert.equal(buf.toString(), 'էէէէէ'); - ['ucs2', 'ucs-2', 'utf16le', 'utf-16le'].forEach(function(encoding) { var b = new Buffer(10); b.write('あいうえお', encoding);