Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Buffer.from(function) should probably fail #26741

Closed
ChALkeR opened this issue Mar 18, 2019 · 8 comments
Closed

Buffer.from(function) should probably fail #26741

ChALkeR opened this issue Mar 18, 2019 · 8 comments
Labels
buffer Issues and PRs related to the buffer subsystem.

Comments

@ChALkeR
Copy link
Member

ChALkeR commented Mar 18, 2019

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.

@ChALkeR ChALkeR added buffer Issues and PRs related to the buffer subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels Mar 18, 2019
@devsnek
Copy link
Member

devsnek commented Mar 18, 2019

sgtm

@cjihrig
Copy link
Contributor

cjihrig commented Mar 18, 2019

#26743

@cjihrig
Copy link
Contributor

cjihrig commented Mar 18, 2019

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
Copy link
Member

@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
Copy link
Member Author

ChALkeR commented Mar 18, 2019

@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
Copy link
Member Author

ChALkeR commented Mar 18, 2019

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

@BridgeAR
Copy link
Member

@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
Copy link
Member

./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

@targos targos closed this as completed in 75eaf25 Mar 30, 2019
BethGriggs pushed a commit that referenced this issue Apr 5, 2019
So far we did not throw an error for all types of invalid input.
Functions do not return a buffer anymore and `number` and `symbol`
validation is also improved.

PR-URL: #26825
Fixes: #26741
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
BethGriggs pushed a commit that referenced this issue Apr 9, 2019
So far we did not throw an error for all types of invalid input.
Functions do not return a buffer anymore and `number` and `symbol`
validation is also improved.

PR-URL: #26825
Fixes: #26741
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Signed-off-by: Beth Griggs <Bethany.Griggs@uk.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants