stream_wrap: cast input to Buffer #4031
Conversation
|
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 ) |
|
R=@bnoordhuis or @trevnorris |
|
cc @nodejs/crypto |
| @@ -4,6 +4,7 @@ const assert = require('assert'); | |||
| const util = require('util'); | |||
| const Socket = require('net').Socket; | |||
| const JSStream = process.binding('js_stream').JSStream; | |||
| const Buffer = require('buffer').Buffer; | |||
There was a problem hiding this comment.
I think you can drop this line completely.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
It is available, but I thought that we decided to not use globals as much as we can.
There was a problem hiding this comment.
Oh, I didn't know that. Ignore my comment then :-)
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. |
|
@cjihrig I would say all of the encodings will make it fail depending on the particular input. I think it can emit |
|
@indutny Are you saying the data is converted from Buffer to String by |
|
Yeah, not sure if it is a right thing to do, but at least it does not crash... |
|
@indutny may have a problem since v8 automatically strips invalid utf8 characters. so not guaranteed to preserve the data in all cases. |
|
Ok, going to make it emit |
|
I think resetting the encoding after someone explicitly sets it to something else will lead to a lot of confusion. |
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
|
@cjihrig @trevnorris updated. |
|
PTAL |
| const StreamWrap = require('_stream_wrap'); | ||
| const Duplex = require('stream').Duplex; | ||
|
|
||
| var done = false; |
|
LGTM with comments. |
|
@cjihrig fixed, thanks! |
|
Unrelated CI failures, landing. |
|
Landed in de2fd63, thank you! |
|
@indutny ... should this go into v4 also? |
|
Actually, yes! Thank you @jasnell |
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>
When wrapping stream - cast its input data to
Bufferbefore using.Someone may have called
.setEncoding()on it, and we should not crashif the input is a string.
Fix: #3970