buffer: introduce Buffer.encode() method

[JacksonTian]

Buffer constructor has too much behaviour.

it is usual to convert string to Buffer in the form of new
Buffer(str).

If don't check type of first argument, new Buffer(number) can lead
Buffer vulnerability. For example:

• https://github.com/websockets/ws/releases/tag/1.0.1
• RangeError: Attempt to allocate Buffer larger than maximum size: 0x3fffffff bytes qiniu/nodejs-sdk#123

Introduce Buffer.encode() method to be explicit about encoding.

[mscdex]

Why not simply do new Buffer(val.toString(), encoding) or if val can be undefined/null, new Buffer((val || '').toString(), encoding)? I don't see why an extra method like this needs to be added to core.

[JacksonTian]

In fact, too much guys use new Buffer() but forget to validate/convert input.

[mscdex]

IMHO that seems more like a userland problem. Either way, any such userland code would need to be changed, so you might as well just do new Buffer((val || '').toString(), encoding).

[ronkorving]

A userland problem that is promoted by the design of Buffer. I think this is well illustrated by the fact that your solution of val || '' would turn the number 0 into empty string, not the string '0'. That's not converting the number to a string. IMHO, the proper userland solution would be new Buffer(String(val));.

But that aside, doesn't this illustrate that there may indeed be a bit of an issue here?

[cjihrig]

IMO, a function like this falls into the same discussion as a parameter to the Buffer constructor to zero fill the allocated memory. Both of these have trivial existing solutions (Buffer(val + '') and fill(0)). If people aren't using the existing solutions, I'm not really convinced that they would use these new approaches either. I think we just need to increase and improve messaging about this.

[mscdex]

@ronkorving That was merely an example. My point was this can be easily done in userland and IMHO it does not require the addition of a new Buffer method.

[trevnorris]

It needs to be accepted that this is not a vulnerability of the Buffer API. How it will operate is well documented, and there aren't any surprises when using Buffer as documented (if there are then we do have a bug, or documentation fail).

Specifically in the ws case it's even stated in the release notes that they "didn't do any checks for the type of data [they] were sending."[1] This is akin to not sanitizing an incoming query and allowing a SQL injection attack, except in the SQL case it's written off as poor programming practice from the implementer.

So in any case the most we'd be doing is making the API more "developer-friendly". As for this PR, allowing the argument to be coerced doesn't follow its own documentation. If anything will be accepted then it should state {Value} and that any non string argument will be coerced. Or it should throw if the argument is not a string.

Anyway, I'm open to discussion on the topic but want to make sure we really understand why this or something similar would be added.

[1] https://github.com/websockets/ws/releases/tag/1.0.1

[JacksonTian]

Yes, Make the API more "developer-friendly" is my point.

[jasnell]

This can be closed now that the new buffer api's have landed.

[JacksonTian]

Thanks @jasnell