Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

buffer: validate list elements in Buffer.concat #4951

Merged
merged 1 commit into from
Jan 31, 2016

Conversation

targos
Copy link
Member

@targos targos commented Jan 29, 2016

buffer: validate list elements in Buffer.concat

Without this change, if any of the elements in the list to be concatenated
is not a Buffer instance, the method fails with "buf.copy is not a function".
Make an instanceof check before using the copy method so that we can throw
a with a better message.

Fixes: #4949

@targos targos added the buffer Issues and PRs related to the buffer subsystem. label Jan 29, 2016
@@ -241,6 +241,8 @@ Buffer.concat = function(list, length) {
var pos = 0;
for (let i = 0; i < list.length; i++) {
var buf = list[i];
if (!(buf instanceof Buffer))
Copy link
Contributor

Choose a reason for hiding this comment

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

Or Buffer.isBuffer?

Copy link
Member Author

Choose a reason for hiding this comment

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

I chose instanceof for consistency (Buffer.isBuffer is never used in this file).

Copy link
Contributor

Choose a reason for hiding this comment

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

I was suggesting that because, if we change how we detect Buffers (which will be implemented in isBuffer) later on, then we don't have to change this line.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I changed it.

@cjihrig
Copy link
Contributor

cjihrig commented Jan 29, 2016

LGTM with one comment.

@thefourtheye
Copy link
Contributor

LGTM with cjihrig's comment

@targos
Copy link
Member Author

targos commented Jan 29, 2016

@targos targos added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jan 29, 2016
@targos
Copy link
Member Author

targos commented Jan 29, 2016

Marking semver-major because of the error message change.

@cjihrig
Copy link
Contributor

cjihrig commented Jan 29, 2016

I'm not sure I would classify this as semver-major. This isn't a change to an existing error message. Every bug fix is technically a behavior change.

@targos
Copy link
Member Author

targos commented Jan 29, 2016

I agree with you. Then it would be a candidate for LTS ?

@thefourtheye
Copy link
Contributor

Overall, This looks to me as just an error message change. Does this qualify for backporting? @jasnell @thealphanerd

@cjihrig
Copy link
Contributor

cjihrig commented Jan 29, 2016

The existing code was already checking for an array, and throwing this same error. It was a bug on our part that we didn't do adequate checking.

@targos targos added lts-watch-v4.x and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels Jan 29, 2016
@r-52
Copy link
Contributor

r-52 commented Jan 30, 2016

LGTM

Without this change, if any of the elements in the list to be concatenated is
not a Buffer instance, the method fails with "buf.copy is not a function".
Make an isBuffer check before using the copy method so that we can throw with
a better message.

Fixes: nodejs#4949
PR-URL: nodejs#4951
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Roman Klauke <romaaan.git@gmail.com>
@targos targos merged commit d2dc234 into nodejs:master Jan 31, 2016
@targos targos deleted the node-4949 branch January 31, 2016 08:36
@rvagg
Copy link
Member

rvagg commented Feb 8, 2016

@nodejs/lts this commit adds a new error string "list" argument must be an Array of Buffers, in addition to an existing list argument must be an Array of Buffers on everything but master due to a semver-major change to error messages. To remain consistent with the existing error message and to not introduce a new one that's different to the existing by two " characters, this commit should be changed to remove the "'s from lib/buffer.js as well as test/parallel/test-buffer-concat.js when cherry-picking.

rvagg pushed a commit that referenced this pull request Feb 8, 2016
Without this change, if any of the elements in the list to be concatenated is
not a Buffer instance, the method fails with "buf.copy is not a function".
Make an isBuffer check before using the copy method so that we can throw with
a better message.

Fixes: #4949
PR-URL: #4951
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Roman Klauke <romaaan.git@gmail.com>
@trevnorris
Copy link
Contributor

@rvagg in that case shouldn't it just be changed in master as well? seems strange to only change it on cherry-pick.

@rvagg
Copy link
Member

rvagg commented Feb 9, 2016

@trevnorris no because master has a change that unifies all error messages, giving them consistent formatting. So what's in this is correct and nice and consistent with the rest of the codebase but that's not true of v5.x and v4.x where we need to be internally consistent with this particular file and this particular function.

@rvagg
Copy link
Member

rvagg commented Feb 9, 2016

#3374 thar it is, I think there may be more work ontop of that as well

@rvagg
Copy link
Member

rvagg commented Feb 11, 2016

Removed lts-watch-v4.x, this is breaking in userland, see rvagg/bl#26, moscajs/mosca#412

@targos
Copy link
Member Author

targos commented Feb 12, 2016

Should we label it as dont-land-on-v4.x then ?

@rvagg
Copy link
Member

rvagg commented Feb 15, 2016

sure, done, but tbh I'm not sure if that's even used, @thealphanerd can you clarify how we should be using lts-watch-v4.x and/or dont-land-on-v4.x?

@MylesBorins
Copy link
Contributor

@rvagg we definitely use dont-land-on-v4.x

lts-watch is applied when a PR needs to be reviewed
land-on is applied when a commit is in staging but not in a release
landed is applied when a commit has landed in a release

dont-land is added when we know a commit will not be backported. dont-land is particularly useful when doing audits of master to make sure we didn't miss anything

scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
Without this change, if any of the elements in the list to be concatenated is
not a Buffer instance, the method fails with "buf.copy is not a function".
Make an isBuffer check before using the copy method so that we can throw with
a better message.

Fixes: nodejs#4949
PR-URL: nodejs#4951
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Roman Klauke <romaaan.git@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants