Skip to content

Commit 57ed3da

Browse files
trevnorristrevnorris
committed
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]>
1 parent f2a78de commit 57ed3da

4 files changed

Lines changed: 91 additions & 53 deletions

File tree

‎lib/buffer.js‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -295,6 +295,29 @@ Buffer.prototype.compare = function compare(b) {
295295
};
296296

297297

298+
Buffer.prototype.fill = function fill(val, start, end) {
299+
start = start >> 0;
300+
end = (end === undefined) ? this.length : end >> 0;
301+
302+
if (start < 0 || end > this.length)
303+
throw new RangeError('out of range index');
304+
if (end <= start)
305+
return this;
306+
307+
if (typeof val !== 'string') {
308+
val = val >>> 0;
309+
} else if (val.length === 1) {
310+
var code = val.charCodeAt(0);
311+
if (code < 256)
312+
val = code;
313+
}
314+
315+
internal.fill(this, val, start, end);
316+
317+
return this;
318+
};
319+
320+
298321
// XXX remove in v0.13
299322
Buffer.prototype.get = util.deprecate(function get(offset) {
300323
offset = ~~offset;

‎src/env.h‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,6 @@ namespace node {
7272
V(buffer_string, "buffer") \
7373
V(bytes_string, "bytes") \
7474
V(bytes_parsed_string, "bytesParsed") \
75-
V(byte_length_string, "byteLength") \
7675
V(callback_string, "callback") \
7776
V(change_string, "change") \
7877
V(close_string, "close") \

‎src/node_buffer.cc‎

Lines changed: 18 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -338,37 +338,31 @@ void Copy(const FunctionCallbackInfo<Value> &args) {
338338
}
339339

340340

341-
// buffer.fill(value[, start][, end]);
342341
void Fill(const FunctionCallbackInfo<Value>& args) {
343-
Environment* env = Environment::GetCurrent(args.GetIsolate());
344-
HandleScope scope(env->isolate());
342+
ARGS_THIS(args[0].As<Object>())
345343

346-
ARGS_THIS(args.This())
347-
SLICE_START_END(args[1], args[2], obj_length)
348-
args.GetReturnValue().Set(args.This());
344+
size_t start = args[2]->Uint32Value();
345+
size_t end = args[3]->Uint32Value();
346+
size_t length = end - start;
347+
CHECK(length + start <= obj_length);
349348

350-
if (args[0]->IsNumber()) {
351-
int value = args[0]->Uint32Value() & 255;
349+
if (args[1]->IsNumber()) {
350+
int value = args[1]->Uint32Value() & 255;
352351
memset(obj_data + start, value, length);
353352
return;
354353
}
355354

356-
node::Utf8Value at(args[0]);
357-
size_t at_length = at.length();
355+
node::Utf8Value str(args[1]);
356+
size_t str_length = str.length();
357+
size_t in_there = str_length;
358+
char* ptr = obj_data + start + str_length;
358359

359-
// optimize single ascii character case
360-
if (at_length == 1) {
361-
int value = static_cast<int>((*at)[0]);
362-
memset(obj_data + start, value, length);
360+
if (str_length == 0)
363361
return;
364-
}
365362

366-
size_t in_there = at_length;
367-
char* ptr = obj_data + start + at_length;
363+
memcpy(obj_data + start, *str, MIN(str_length, length));
368364

369-
memcpy(obj_data + start, *at, MIN(at_length, length));
370-
371-
if (at_length >= length)
365+
if (str_length >= length)
372366
return;
373367

374368
while (in_there < length - in_there) {
@@ -660,7 +654,6 @@ void SetupBufferJS(const FunctionCallbackInfo<Value>& args) {
660654
NODE_SET_METHOD(proto, "writeFloatLE", WriteFloatLE);
661655

662656
NODE_SET_METHOD(proto, "copy", Copy);
663-
NODE_SET_METHOD(proto, "fill", Fill);
664657

665658
// for backwards compatibility
666659
proto->Set(env->offset_string(),
@@ -670,16 +663,11 @@ void SetupBufferJS(const FunctionCallbackInfo<Value>& args) {
670663
assert(args[1]->IsObject());
671664

672665
Local<Object> internal = args[1].As<Object>();
666+
ASSERT(internal->IsObject());
673667

674-
Local<Function> byte_length = FunctionTemplate::New(
675-
env->isolate(), ByteLength)->GetFunction();
676-
byte_length->SetName(env->byte_length_string());
677-
internal->Set(env->byte_length_string(), byte_length);
678-
679-
Local<Function> compare = FunctionTemplate::New(
680-
env->isolate(), Compare)->GetFunction();
681-
compare->SetName(env->compare_string());
682-
internal->Set(env->compare_string(), compare);
668+
NODE_SET_METHOD(internal, "byteLength", ByteLength);
669+
NODE_SET_METHOD(internal, "compare", Compare);
670+
NODE_SET_METHOD(internal, "fill", Fill);
683671
}
684672

685673

‎test/simple/test-buffer.js‎

Lines changed: 50 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,56 @@ var c = new Buffer(512);
4949
console.log('c.length == %d', c.length);
5050
assert.strictEqual(512, c.length);
5151

52+
// First check Buffer#fill() works as expected.
53+
54+
assert.throws(function() {
55+
Buffer(8).fill('a', -1);
56+
});
57+
58+
assert.throws(function() {
59+
Buffer(8).fill('a', 0, 9);
60+
});
61+
62+
// Make sure this doesn't hang indefinitely.
63+
Buffer(8).fill('');
64+
65+
var buf = new Buffer(64);
66+
buf.fill(10);
67+
for (var i = 0; i < buf.length; i++)
68+
assert.equal(buf[i], 10);
69+
70+
buf.fill(11, 0, buf.length >> 1);
71+
for (var i = 0; i < buf.length >> 1; i++)
72+
assert.equal(buf[i], 11);
73+
for (var i = (buf.length >> 1) + 1; i < buf.length; i++)
74+
assert.equal(buf[i], 10);
75+
76+
buf.fill('h');
77+
for (var i = 0; i < buf.length; i++)
78+
assert.equal('h'.charCodeAt(0), buf[i]);
79+
80+
buf.fill(0);
81+
for (var i = 0; i < buf.length; i++)
82+
assert.equal(0, buf[i]);
83+
84+
buf.fill(null);
85+
for (var i = 0; i < buf.length; i++)
86+
assert.equal(0, buf[i]);
87+
88+
buf.fill(1, 16, 32);
89+
for (var i = 0; i < 16; i++)
90+
assert.equal(0, buf[i]);
91+
for (; i < 32; i++)
92+
assert.equal(1, buf[i]);
93+
for (; i < buf.length; i++)
94+
assert.equal(0, buf[i]);
95+
96+
var buf = new Buffer(10);
97+
buf.fill('abc');
98+
assert.equal(buf.toString(), 'abcabcabca');
99+
buf.fill('է');
100+
assert.equal(buf.toString(), 'էէէէէ');
101+
52102
// copy 512 bytes, from 0 to 512.
53103
b.fill(++cntr);
54104
c.fill(++cntr);
@@ -642,28 +692,6 @@ assert.equal(0x6f, z[1]);
642692

643693
assert.equal(0, Buffer('hello').slice(0, 0).length);
644694

645-
b = new Buffer(50);
646-
b.fill('h');
647-
for (var i = 0; i < b.length; i++) {
648-
assert.equal('h'.charCodeAt(0), b[i]);
649-
}
650-
651-
b.fill(0);
652-
for (var i = 0; i < b.length; i++) {
653-
assert.equal(0, b[i]);
654-
}
655-
656-
b.fill(1, 16, 32);
657-
for (var i = 0; i < 16; i++) assert.equal(0, b[i]);
658-
for (; i < 32; i++) assert.equal(1, b[i]);
659-
for (; i < b.length; i++) assert.equal(0, b[i]);
660-
661-
var buf = new Buffer(10);
662-
buf.fill('abc');
663-
assert.equal(buf.toString(), 'abcabcabca');
664-
buf.fill('է');
665-
assert.equal(buf.toString(), 'էէէէէ');
666-
667695
['ucs2', 'ucs-2', 'utf16le', 'utf-16le'].forEach(function(encoding) {
668696
var b = new Buffer(10);
669697
b.write('あいうえお', encoding);

0 commit comments

Comments
 (0)