stream_wrap: cast input to Buffer

[indutny]

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]

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]

R=@bnoordhuis or @trevnorris

[indutny]

cc @nodejs/crypto

[cjihrig]

I think you can drop this line completely.

[indutny]

Why?

[cjihrig]

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]

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

[cjihrig]

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

[cjihrig]

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]

@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]

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

[indutny]

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

[trevnorris]

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

[indutny]

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

[cjihrig]

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

[indutny]

@cjihrig@trevnorris updated.

[indutny]

PTAL

[cjihrig]

This is unused.

[indutny]

Ack.

[cjihrig]

LGTM with comments.

[indutny]

@cjihrig fixed, thanks!

[indutny]

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

[indutny]

Unrelated CI failures, landing.

[indutny]

Landed in de2fd63, thank you!

[jasnell]

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

[indutny]

Actually, yes! Thank you @jasnell