Node: Converting String-object to Buffer fails to copy data

[mvduin]

• Version: v8.1.0

Creating a buffer from a String object results in a zero-filled buffer (of length equal to the string length in UTF-16 code-units):

> Buffer.from(new String("Hello world"))
<Buffer 00 00 00 00 00 00 00 00 00 00 00>
> Buffer.from(new String("\u{1F4A9}"))
<Buffer 00 00>

[bnoordhuis]

It's because there is no special treatment for string objects, they are interpreted as array-like objects with their elements coerced to numeric values. That is, these are all equal:

Buffer.from(new String('x'));
Buffer.from({ length: 1, [0]: 'x' });
Buffer.from({ length: 1, [0]: +'x' });  // +'x' -> Nan -> 0

Node has no special treatment for string objects elsewhere either so it's a bit of an open question if we should do it here.

[mvduin]

Ah... eww... okay.

I had hoped to use a String subclass to mark certain strings as having some particular semantic value, but somewhere down the road they get converted to buffer in third party code.... oh well, it briefly seemed like a good idea.

With the introduction of things like String subclasses, Symbol.toPrimitive, etc it would definitely have been nice if they had also included an authoritive way code can figure out how it's _supposed_ to treat objects... "are you trying to be an array? a string? a number maybe?"

[benjamingr]

@mvduin just to be clear - what would you expect the behavior to be in this case noting:

> var s = new String()
> s[Symbol.toPrimitive](); // uncaught type error s[Symbol.toPrimitive] is not a function

?

[mvduin]

That's not what I meant, but never mind - you can ignore that bit of my comment.

[jasnell]

@mvduin ... the easiest thing to do would be just:

Buffer.from(new String('test').valueOf());
// or
Buffer.from(new String('test').toString());

That would allow you to do the String subclassing.

That said, I can see a case being made to allow Buffer.from() to accept objects that support Symbol.toPrimitive(). Will have to give that one some thought.

[bnoordhuis]

I think a reasonable case can be made that the current behavior is confusing and possibly insecure in some circumstances. Consider how a string of digits is parsed differently based on whether the value is a primitive string or a string object:

> Buffer.from(String('0123456789')).toString()
'0123456789'

Vs.

> Buffer.from(new String('0123456789')).toString()
'\u0000\u0001\u0002\u0003\u0004\u0005\u0006\u0007\b\t'

You wouldn't write such code directly but if you pass in a variable x without knowing if x is primitive or not, you'll get confusing results.

[mvduin]

Exactly, the issue didn't happen directly in my code but in third party code that got my non-primitive strings somewhere down the road and transmitted them over a socket

[joyeecheung]

@mvduin I think in this case there is a somewhat related symbol, it's Symbol.iterator instead of Symbol.toPrimitive(minus the fact that boxed types doesn't have Symbol.toPrimitive defined, but I got your point), because it does not really make sense to do Buffer.from(new Number(1)).

I think it's an open question how Buffer.from(boxedString) should be interpreted, but if an encoding is passed as the second argument, it makes sense to the convert the first argument to a string value using toString().

[ryzokuken]

Does this still need to stay open?

/cc @jasnell

[ryzokuken]

This seems to be resolved in https://github.com/nodejs/node/pull/13725, but I may be wrong, in which case, please feel free to reopen this.

[mvduin]

Looks good to me, thanks!