Node: Buffer.from(function) should probably fail

[ChALkeR]

Current behavior of Buffer.from when a function is passed as the first argument:

$ nvm use 12
Now using node v12.0.0-nightly201902158e68dc53b3 (npm v6.7.0)
$ node
> Buffer.from(() => {})
<Buffer >
> Buffer.from((x,y,z) => {})
<Buffer 00 00 00>

That doesn't seem useful, so passing a function in is most likely a programmers error, so we could throw errors on that to indicate those early. Thoughts?

If there are no immediate objections, I'll make a draft PR.

[devsnek]

sgtm

[cjihrig]

https://github.com/nodejs/node/pull/26743

[cjihrig]

If there are no immediate objections, I'll make a draft PR.

Sorry @ChALkeR. I didn't read the entire issue after I saw how to reproduce it. I'll close my PR.

[BridgeAR]

@ChALkeR to me it seems like we should also throw on all primitives besides strings. That would also simplify the logic:

diff --git a/lib/buffer.js b/lib/buffer.js
index c5497e18aa..4c00652b50 100644
--- a/lib/buffer.js
+++ b/lib/buffer.js
@@ -192,33 +192,23 @@ Buffer.from = function from(value, encodingOrOffset, length) {
   if (typeof value === 'string')
     return fromString(value, encodingOrOffset);

-  if (isAnyArrayBuffer(value))
-    return fromArrayBuffer(value, encodingOrOffset, length);
+  if (typeof value === 'object' && value !== null) {
+    if (isAnyArrayBuffer(value))
+      return fromArrayBuffer(value, encodingOrOffset, length);

-  if (value === null || value === undefined) {
-    throw new ERR_INVALID_ARG_TYPE(
-      'first argument',
-      ['string', 'Buffer', 'ArrayBuffer', 'Array', 'Array-like Object'],
-      value
-    );
-  }
-
-  if (typeof value === 'number') {
-    throw new ERR_INVALID_ARG_TYPE('value', 'not number', value);
-  }
+    const valueOf = value.valueOf && value.valueOf();
+    if (valueOf !== null && valueOf !== undefined && valueOf !== value)
+      return Buffer.from(valueOf, encodingOrOffset, length);

-  const valueOf = value.valueOf && value.valueOf();
-  if (valueOf !== null && valueOf !== undefined && valueOf !== value)
-    return Buffer.from(valueOf, encodingOrOffset, length);
-
-  const b = fromObject(value);
-  if (b)
-    return b;
+    const b = fromObject(value);
+    if (b)
+      return b;

-  if (typeof value[Symbol.toPrimitive] === 'function') {
-    return Buffer.from(value[Symbol.toPrimitive]('string'),
-                       encodingOrOffset,
-                       length);
+    if (typeof value[Symbol.toPrimitive] === 'function') {
+      return Buffer.from(value[Symbol.toPrimitive]('string'),
+                         encodingOrOffset,
+                         length);
+    }
   }

   throw new ERR_INVALID_ARG_TYPE(

[ChALkeR]

@cjihrig Um, there was no reason to close your PR if you already drafted that! Less work for me =).

I have not yet started doing anything, just wanted to collect initial feedback, so if you already have a PR, it would make sense reopening it, I think?

The PR looks good at the first glance, I will review it later if you reopen it. 😉

[ChALkeR]

@BridgeAR could you give a list of examples which behavior get changed by that?

[BridgeAR]

@ChALkeR currently symbols cause an maximum call stack size error and the error message for numbers is different than from the other primitives (and less helpful).

For bigint and booleans it's only about executing less code. Currently we try to create something useful from those types and if that fails, we'll throw the error.

[BridgeAR]

./node -e 'Buffer.from(Symbol())'

RangeError: Maximum call stack size exceeded
    at Symbol.get [Symbol.toStringTag] (<anonymous>)
    at internal/util/types.js:11:32
    at isUint8Array (internal/util/types.js:29:10)
    at fromObject (buffer.js:390:7)
    at Function.from (buffer.js:217:13)
    at Function.from (buffer.js:222:19)
    at Function.from (buffer.js:222:19)

./node -e 'Buffer.from(5)'

TypeError [ERR_INVALID_ARG_TYPE]: The "value" argument must not be of type number. Received type number
    at Function.from (buffer.js:210:11)
    at [eval]:1:8
    at Script.runInThisContext (vm.js:124:20)
    at Object.runInThisContext (vm.js:314:38)
    at Object.<anonymous> ([eval]-wrapper:9:26)
    at Module._compile (internal/modules/cjs/loader.js:813:30)
    at evalScript (internal/process/execution.js:60:25)
    at internal/main/eval_string.js:16:1