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

bl.append(number) appends a buffer with uninitialized memory #22

Closed
wants to merge 2 commits into from
Closed

bl.append(number) appends a buffer with uninitialized memory #22

wants to merge 2 commits into from

Conversation

feross
Copy link
Contributor

@feross feross commented Jan 19, 2016

This PR changes the behavior of bl.append(number) so that it no longer appends a buffer with uninitialized memory of the specified size.

The issue is fixed by coercing the argument to a string. (An alternate solution would be to throw an exception in this case.)

This played a role in this issue in request: request/request#2018

Further reading:

@@ -52,6 +52,7 @@ BufferList.prototype.append = function (buf) {
var isBuffer = Buffer.isBuffer(buf) ||
buf instanceof BufferList

if (typeof buf === 'number') buf = buf.toString()
Copy link
Owner

Choose a reason for hiding this comment

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

newline, == instead of === please

Copy link
Owner

Choose a reason for hiding this comment

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

comment would also be good

@rvagg
Copy link
Owner

rvagg commented Jan 19, 2016

lgtm sans syntax issues

bl.append(number) no longer appends a buffer with uninitialized memory
@feross
Copy link
Contributor Author

feross commented Jan 19, 2016

@rvagg thanks, fixed as requested :)

@rvagg
Copy link
Owner

rvagg commented Jan 19, 2016

Published as v1.0.1, backported and released as v0.9.5 too, but tbh I have no idea how much that's used. I would have done v0.8 but it doesn't apply cleanly for reasons that suggest that you probably shouldn't be using v0.8 anyway.

Thanks @feross.

@rvagg rvagg closed this Jan 19, 2016
@feross
Copy link
Contributor Author

feross commented Jan 19, 2016

Nice work. Thanks for going above-and-beyond by backporting to pre-1.0.

@mcollina
Copy link
Collaborator

👍, nice!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants