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

v8.deserialize triggers Buffer constructor deprecation warning #21181

Closed
Conduitry opened this issue Jun 6, 2018 · 7 comments
Closed

v8.deserialize triggers Buffer constructor deprecation warning #21181

Conduitry opened this issue Jun 6, 2018 · 7 comments
Assignees
Labels
buffer Issues and PRs related to the buffer subsystem. confirmed-bug Issues with confirmed bugs.

Comments

@Conduitry
Copy link

Conduitry commented Jun 6, 2018

  • Version: Observed on 10.4.0, presumably occurs on all 10+
  • Platform: All

Deserializing a representation of an expression that contained a Buffer displays(node:15176) [DEP0005] DeprecationWarning: Buffer() is deprecated due to security and usability issues. Please use the Buffer.alloc(), Buffer.allocUnsafe(), or Buffer.from() methods instead.

Reproduce withv8.deserialize(v8.serialize(Buffer.alloc(0))).

It looks like DefaultDeserializer ._readHostObject() needs a special case for buffers.

@Trott Trott added buffer Issues and PRs related to the buffer subsystem. v8 engine Issues and PRs related to the V8 dependency. confirmed-bug Issues with confirmed bugs. labels Jun 7, 2018
@Trott
Copy link
Member

Trott commented Jun 7, 2018

@nodejs/v8 @nodejs/buffer

@ryzokuken
Copy link
Contributor

@Conduitry line to be blamed for this:

node/lib/v8.js

Line 189 in c9d9bf1

return new ctor(this.buffer.buffer,

/cc @nodejs/buffer @ChALkeR do we break character and hide the warning? I hope not. There's got to be a way to achieve the same functionality without using the Buffer constructor.

@addaleax
Copy link
Member

addaleax commented Jun 7, 2018

Like this? :)

diff --git a/lib/v8.js b/lib/v8.js
index ed93b094ca78..0d9ffc6033ce 100644
--- a/lib/v8.js
+++ b/lib/v8.js
@@ -144,5 +144,5 @@ const arrayBufferViewTypeToIndex = new Map();
 }
 
-const bufferConstructorIndex = arrayBufferViewTypes.push(Buffer) - 1;
+const bufferConstructorIndex = arrayBufferViewTypes.push(FastBuffer) - 1;
 
 class DefaultSerializer extends Serializer {

@ryzokuken
Copy link
Contributor

@addaleax we have a drop-in, secure replacement for Buffer? I didn't know. Will make a patch.

@ryzokuken ryzokuken removed the v8 engine Issues and PRs related to the V8 dependency. label Jun 7, 2018
@addaleax
Copy link
Member

addaleax commented Jun 7, 2018

@ryzokuken It’s not really drop-in because it only takes Uint8Array-style arguments, but that’s not an issue here, right?

@ryzokuken
Copy link
Contributor

Exactly. The arguments are literally being passed into an Uint8Array, so that's definitely not a problem.

ryzokuken added a commit to ryzokuken/node that referenced this issue Jun 7, 2018
Replace the Buffer constructor with a FastBuffer in v8.deserialize in
order to avoid calling the Buffer constructor and thus triggering a
deprecation warning from code inside the core.

Fixes: nodejs#21181
targos pushed a commit that referenced this issue Jun 14, 2018
Replace the Buffer constructor with a FastBuffer in v8.deserialize in
order to avoid calling the Buffer constructor and thus triggering a
deprecation warning from code inside the core.

Fixes: #21181

PR-URL: #21196
Fixes: #21181
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@Conduitry
Copy link
Author

This looks good now in 10.5.0 👍

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. confirmed-bug Issues with confirmed bugs.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants