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

stream_wrap: cast input to Buffer by indutny · Pull Request #4031 · nodejs/node · GitHub

/ node Public

stream_wrap: cast input to Buffer - #4031

Closed
indutny wants to merge 2 commits into
nodejs:masterfrom
indutny:fix/gh-3970
ConversationCommits (2)ChecksFiles changed
Closed

stream_wrap: cast input to Buffer#4031
indutny wants to merge 2 commits into
nodejs:masterfrom
indutny:fix/gh-3970

Conversation

indutny commented Nov 25, 2015

Copy link
Copy Markdown
Member

When wrapping stream - cast its input data to Buffer before using.
Someone may have called .setEncoding() on it, and we should not crash
if the input is a string.

Fix: #3970

indutny commented Nov 25, 2015

Copy link
Copy Markdown
Member Author

It is an open question though, what should be a proper behavior here. Should it just throw instead, or should it try to reset the encoding on stream? (There is no such API method atm, cc @nodejs/streams )

indutny commented Nov 25, 2015

Copy link
Copy Markdown
Member Author

R=@bnoordhuis or @trevnorris

indutny commented Nov 25, 2015

Copy link
Copy Markdown
Member Author

cc @nodejs/crypto

mscdex added the stream Issues and PRs related to the stream subsystem. label Nov 25, 2015
cjihrig reviewed Nov 30, 2015
View reviewed changes
Comment thread lib/_stream_wrap.js

cjihrig Nov 30, 2015

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 think you can drop this line completely.

indutny Nov 30, 2015

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

Why?

cjihrig Nov 30, 2015

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

Isn't Buffer available everywhere without requiring it? I was able to apply your changes and run it without the require() and it worked fine for me.

indutny Nov 30, 2015

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

It is available, but I thought that we decided to not use globals as much as we can.

cjihrig Nov 30, 2015

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

Oh, I didn't know that. Ignore my comment then :-)

cjihrig commented Nov 30, 2015

Copy link
Copy Markdown
Contributor

Should it just throw instead

Are all string encodings guaranteed to make this fail? And, if you throw, can it be done so that the error can be caught? If not, maybe emit an error instead.

indutny commented Nov 30, 2015

Copy link
Copy Markdown
Member Author

@cjihrig I would say all of the encodings will make it fail depending on the particular input. I think it can emit error instead.

trevnorris commented Nov 30, 2015

Copy link
Copy Markdown
Contributor

@indutny Are you saying the data is converted from Buffer to String by setEncoding() and this converts it back to a Buffer?

indutny commented Nov 30, 2015

Copy link
Copy Markdown
Member Author

Yeah, not sure if it is a right thing to do, but at least it does not crash...

trevnorris commented Nov 30, 2015

Copy link
Copy Markdown
Contributor

@indutny may have a problem since v8 automatically strips invalid utf8 characters. so not guaranteed to preserve the data in all cases.

indutny commented Nov 30, 2015

Copy link
Copy Markdown
Member Author

Ok, going to make it emit error then. What about resetting the encoding, are we interested in APIs like this?

cjihrig commented Nov 30, 2015

Copy link
Copy Markdown
Contributor

I think resetting the encoding after someone explicitly sets it to something else will lead to a lot of confusion.

stream_wrap: error if stream has StringDecoder
4237127 
If `.setEncoding` was called on input stream - all emitted `data` will
be `String`s instances, not `Buffer`s. This is unacceptable for
`StreamWrap`, and should not lead to the crash.

Fix: nodejs#3970

indutny commented Dec 5, 2015

Copy link
Copy Markdown
Member Author

@cjihrig @trevnorris updated.

indutny commented Dec 5, 2015

Copy link
Copy Markdown
Member Author

PTAL

cjihrig reviewed Dec 7, 2015
View reviewed changes
Comment thread test/parallel/test-stream-wrap-encoding.js Outdated

cjihrig Dec 7, 2015

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

This is unused.

indutny Dec 7, 2015

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

Ack.

cjihrig commented Dec 7, 2015

Copy link
Copy Markdown
Contributor

LGTM with comments.

indutny commented Dec 7, 2015

Copy link
Copy Markdown
Member Author

@cjihrig fixed, thanks!

indutny commented Dec 7, 2015

Copy link
Copy Markdown
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/942/

indutny commented Dec 7, 2015

Copy link
Copy Markdown
Member Author

Unrelated CI failures, landing.

indutny commented Dec 7, 2015

Copy link
Copy Markdown
Member Author

Landed in de2fd63, thank you!

indutny closed this Dec 7, 2015
indutny deleted the fix/gh-3970 branch December 7, 2015 02:56
indutny added a commit that referenced this pull request Dec 7, 2015
stream_wrap: error if stream has StringDecoder
de2fd63 
If `.setEncoding` was called on input stream - all emitted `data` will
be `String`s instances, not `Buffer`s. This is unacceptable for
`StreamWrap`, and should not lead to the crash.

Fix: #3970
PR-URL: #4031
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
indutny added a commit that referenced this pull request Dec 8, 2015
stream_wrap: error if stream has StringDecoder
8f845ba 
If `.setEncoding` was called on input stream - all emitted `data` will
be `String`s instances, not `Buffer`s. This is unacceptable for
`StreamWrap`, and should not lead to the crash.

Fix: #3970
PR-URL: #4031
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
rvagg mentioned this pull request Dec 8, 2015
Closed
rvagg mentioned this pull request Dec 8, 2015
Merged

jasnell commented Dec 11, 2015

Copy link
Copy Markdown
Member

@indutny ... should this go into v4 also?

indutny commented Dec 11, 2015

Copy link
Copy Markdown
Member Author

Actually, yes! Thank you @jasnell

indutny added a commit that referenced this pull request Dec 15, 2015
stream_wrap: error if stream has StringDecoder
535855c 
If `.setEncoding` was called on input stream - all emitted `data` will
be `String`s instances, not `Buffer`s. This is unacceptable for
`StreamWrap`, and should not lead to the crash.

Fix: #3970
PR-URL: #4031
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
jasnell mentioned this pull request Dec 17, 2015
Closed
indutny added a commit that referenced this pull request Dec 17, 2015
stream_wrap: error if stream has StringDecoder
957c1f2 
If `.setEncoding` was called on input stream - all emitted `data` will
be `String`s instances, not `Buffer`s. This is unacceptable for
`StreamWrap`, and should not lead to the crash.

Fix: #3970
PR-URL: #4031
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
indutny added a commit that referenced this pull request Dec 23, 2015
stream_wrap: error if stream has StringDecoder
6513a5a 
If `.setEncoding` was called on input stream - all emitted `data` will
be `String`s instances, not `Buffer`s. This is unacceptable for
`StreamWrap`, and should not lead to the crash.

Fix: #3970
PR-URL: #4031
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
stream_wrap: error if stream has StringDecoder
c7c1577 
If `.setEncoding` was called on input stream - all emitted `data` will
be `String`s instances, not `Buffer`s. This is unacceptable for
`StreamWrap`, and should not lead to the crash.

Fix: nodejs#3970
PR-URL: nodejs#4031
Reviewed-By: Colin Ihrig <cjihrig@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

stream Issues and PRs related to the stream subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL