Skip to content

Commit

Permalink
buffer: fix case of one buffer passed to concat
Browse files Browse the repository at this point in the history
Fix Buffer.concat() so a copy is always returned regardless of the
number of buffers that were passed.

Previously if the array length was one then the same same buffer was
returned. This created a special case for the user where there was a
chance mutating the buffer returned by .concat() could mutate the buffer
passed in.

Also fixes an inconsistency when throwing if an array member was not a
Buffer instance. For example:

    Buffer.concat([42]);      // Returns 42
    Buffer.concat([42, 1]);  // Throws a TypeError

Now .concat() will always throw if an array member is not a Buffer
instance.

See: #1891
PR-URL: #1937
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
  • Loading branch information
thefourtheye authored and trevnorris committed Jun 10, 2015
1 parent 6020d2a commit f4f16bf
Show file tree
Hide file tree
Showing 4 changed files with 7 additions and 10 deletions.
5 changes: 0 additions & 5 deletions doc/api/buffer.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -130,11 +130,6 @@ the list together.
If the list has no items, or if the totalLength is 0, then it returns a
zero-length buffer.

If the list has exactly one item, then the first item of the list is
returned.

If the list has more than one item, then a new Buffer is created.

If totalLength is not provided, it is read from the buffers in the list.
However, this adds an additional loop to the function, so it is faster
to provide the length explicitly.
Expand Down
2 changes: 0 additions & 2 deletions lib/internal/buffer_new.js
Original file line number Diff line number Diff line change
Expand Up @@ -183,8 +183,6 @@ Buffer.concat = function(list, length) {

if (list.length === 0)
return new Buffer(0);
else if (list.length === 1)
return list[0];

if (length === undefined) {
length = 0;
Expand Down
2 changes: 0 additions & 2 deletions lib/internal/buffer_old.js
Original file line number Diff line number Diff line change
Expand Up @@ -249,8 +249,6 @@ Buffer.concat = function(list, length) {

if (list.length === 0)
return new Buffer(0);
else if (list.length === 1)
return list[0];

if (length === undefined) {
length = 0;
Expand Down
8 changes: 7 additions & 1 deletion test/parallel/test-buffer-concat.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,14 @@ var flatLongLen = Buffer.concat(long, 40);

assert(flatZero.length === 0);
assert(flatOne.toString() === 'asdf');
assert(flatOne === one[0]);
// A special case where concat used to return the first item,
// if the length is one. This check is to make sure that we don't do that.
assert(flatOne !== one[0]);
assert(flatLong.toString() === (new Array(10 + 1).join('asdf')));
assert(flatLongLen.toString() === (new Array(10 + 1).join('asdf')));

assert.throws(function() {
Buffer.concat([42]);
}, TypeError);

console.log('ok');

2 comments on commit f4f16bf

@dcousens
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a breaking change no? I'm +1, totally for it, but, it might potentially break users applications if they relied on the old (documented) behaviour?

@cjihrig
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It landed on the next branch, which means it will go in a semver major release.

Please sign in to comment.