FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

buffer: validate list elements in Buffer.concat by targos · Pull Request #4951 · nodejs/node · GitHub

/ node Public

buffer: validate list elements in Buffer.concat - #4951

Merged
targos merged 1 commit into
nodejs:masterfrom
targos:node-4949
Jan 31, 2016
ConversationCommits (1)ChecksFiles changed
Merged

buffer: validate list elements in Buffer.concat#4951
targos merged 1 commit into
nodejs:masterfrom
targos:node-4949

Conversation

targos commented Jan 29, 2016

Copy link
Copy Markdown
Member

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 added the buffer Issues and PRs related to the buffer subsystem. label Jan 29, 2016
thefourtheye reviewed Jan 29, 2016
View reviewed changes
Comment thread lib/buffer.js Outdated

thefourtheye Jan 29, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Or Buffer.isBuffer?

targos Jan 29, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

thefourtheye Jan 29, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

targos Jan 29, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

OK, I changed it.

cjihrig commented Jan 29, 2016

Copy link
Copy Markdown
Contributor

LGTM with one comment.

thefourtheye commented Jan 29, 2016

Copy link
Copy Markdown
Contributor

LGTM with cjihrig's comment

targos commented Jan 29, 2016

Copy link
Copy Markdown
Member Author

CI: https://ci.nodejs.org/job/node-test-commit/1978/

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

targos commented Jan 29, 2016

Copy link
Copy Markdown
Member Author

Marking semver-major because of the error message change.

cjihrig commented Jan 29, 2016

Copy link
Copy Markdown
Contributor

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 commented Jan 29, 2016

Copy link
Copy Markdown
Member Author

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

thefourtheye commented Jan 29, 2016

Copy link
Copy Markdown
Contributor

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

cjihrig commented Jan 29, 2016

Copy link
Copy Markdown
Contributor

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 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 commented Jan 30, 2016

Copy link
Copy Markdown
Contributor

LGTM

buffer: validate list elements in Buffer.concat
d2dc234 
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 merged commit d2dc234 into nodejs:master Jan 31, 2016
targos deleted the node-4949 branch January 31, 2016 08:36

rvagg commented Feb 8, 2016

Copy link
Copy Markdown
Member

@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
buffer: validate list elements in Buffer.concat
58d67e2 
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>
rvagg mentioned this pull request Feb 8, 2016
Closed

trevnorris commented Feb 9, 2016

Copy link
Copy Markdown
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 commented Feb 9, 2016

Copy link
Copy Markdown
Member

@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 commented Feb 9, 2016

Copy link
Copy Markdown
Member

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

Nibbler999 mentioned this pull request Feb 10, 2016
Closed
This was referenced Feb 10, 2016
Closed
Closed

rvagg commented Feb 11, 2016

Copy link
Copy Markdown
Member

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

targos commented Feb 12, 2016

Copy link
Copy Markdown
Member Author

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

rvagg commented Feb 15, 2016

Copy link
Copy Markdown
Member

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 commented Feb 15, 2016

Copy link
Copy Markdown
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
buffer: validate list elements in Buffer.concat
951e3aa 
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

buffer Issues and PRs related to the buffer subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL