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: nodejs#8469
Signed-off-by: Trevor Norris <[email protected]>

Author: trevnorris

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;

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")                                                    \

src/node_buffer.cc

@@ -338,37 +338,31 @@ void Copy(const FunctionCallbackInfo<Value> &args) {
 }
 
 
-// buffer.fill(value[, start][, end]);
 void Fill(const FunctionCallbackInfo<Value>& args) {
-  Environment* env = Environment::GetCurrent(args.GetIsolate());
-  HandleScope scope(env->isolate());
+  ARGS_THIS(args[0].As<Object>())
 
-  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<int>((*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<Value>& 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<Value>& args) {
   assert(args[1]->IsObject());
 
   Local<Object> internal = args[1].As<Object>();
+  ASSERT(internal->IsObject());
 
-  Local<Function> byte_length = FunctionTemplate::New(
-                    env->isolate(), ByteLength)->GetFunction();
-  byte_length->SetName(env->byte_length_string());
-  internal->Set(env->byte_length_string(), byte_length);
-
-  Local<Function> 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);
 }
 
 

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);