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.

Sign up for GitHub

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

Jump to bottom

Please stop using v8::BackingStore::Reallocate #52234

Closed
syg opened this issue Mar 27, 2024 · 2 comments · Fixed by #52292
Closed

Please stop using v8::BackingStore::Reallocate #52234

syg opened this issue Mar 27, 2024 · 2 comments · Fixed by #52292
Labels
deprecations Issues and PRs related to deprecations. v8 platform Issues and PRs related to Node's v8::Platform implementation.

Comments

@syg
Copy link
Contributor

syg commented Mar 27, 2024

V8 is deprecating and removing v8::BackingStore::Reallocate for being a safety footgun: https://issues.chromium.org/u/1/issues/331326406

Node uses it currently AFAICT for some internal buffers. Note that since #43594, there is no performance benefit of using Reallocate. Node's override of Reallocate defers to the default implementation, which allocates a new block of memory and performs a copy. It would be better to explicitly allocate a new v8::BackingStore and explicitly memcpy for the current uses of Reallocate.

(If I have some time I'll try to make a PR. But if someone is inclined to change this, please do so!)
@kylo5aby
Copy link
Contributor

I would like to make a PR

@marco-ippolito marco-ippolito added deprecations Issues and PRs related to deprecations. v8 platform Issues and PRs related to Node's v8::Platform implementation. labels Mar 28, 2024
@joyeecheung
Copy link
Member

joyeecheung commented Mar 28, 2024 •
edited
Loading

Since the override was removed to be compatible with V8 sandboxed pointers for Electron, cc @nornagon @codebytere

targos added a commit to targos/node that referenced this issue Mar 31, 2024
@targos
src: stop using v8::BackingStore::Reallocate
27cc3a6 
It's being deprecated by V8.
Explicitly allocate a new ArrayBuffer and copy the data instead.

Fixes: nodejs#52234
@targos targos mentioned this issue Mar 31, 2024
Merged
targos added a commit that referenced this issue Mar 31, 2024
@targos
src: stop using v8::BackingStore::Reallocate
a4d65e0 
It's being deprecated by V8.
Explicitly allocate a new ArrayBuffer and copy the data instead.

Fixes: #52234
targos added a commit to targos/node that referenced this issue Mar 31, 2024
@targos @joyeecheung
src: stop using v8::BackingStore::Reallocate
ef7fff0 
It's being deprecated by V8.
Explicitly allocate a new ArrayBuffer and copy the data when needed
instead.

Fixes: nodejs#52234
Co-authored-by: Joyee Cheung <joyeec9h3@gmail.com>
nodejs-github-bot pushed a commit to nodejs/node-v8 that referenced this issue Mar 31, 2024
@targos @nodejs-github-bot
src: stop using v8::BackingStore::Reallocate
ca0bb3a 
It's being deprecated by V8.
Explicitly allocate a new ArrayBuffer and copy the data instead.

Fixes: nodejs/node#52234
targos added a commit to targos/node that referenced this issue Mar 31, 2024
@targos @joyeecheung
src: stop using v8::BackingStore::Reallocate
d5adde1 
It's being deprecated by V8.
Explicitly allocate a new ArrayBuffer and copy the data when needed
instead.

Fixes: nodejs#52234
Co-authored-by: Joyee Cheung <joyeec9h3@gmail.com>
nodejs-github-bot pushed a commit to nodejs/node-v8 that referenced this issue Apr 1, 2024
@targos @nodejs-github-bot
src: stop using v8::BackingStore::Reallocate
457e1bc 
It's being deprecated by V8.
Explicitly allocate a new ArrayBuffer and copy the data instead.

Fixes: nodejs/node#52234
nodejs-github-bot pushed a commit to nodejs/node-v8 that referenced this issue Apr 2, 2024
@targos @nodejs-github-bot
src: stop using v8::BackingStore::Reallocate
038bb97 
It's being deprecated by V8.
Explicitly allocate a new ArrayBuffer and copy the data instead.

Fixes: nodejs/node#52234
targos added a commit that referenced this issue Apr 3, 2024
@targos
src: stop using v8::BackingStore::Reallocate
af67dc1 
It's being deprecated by V8.
Explicitly allocate a new ArrayBuffer and copy the data instead.

Fixes: #52234
nodejs-github-bot pushed a commit to nodejs/node-v8 that referenced this issue Apr 3, 2024
@targos @nodejs-github-bot
src: stop using v8::BackingStore::Reallocate
65a54a3 
It's being deprecated by V8.
Explicitly allocate a new ArrayBuffer and copy the data instead.

Fixes: nodejs/node#52234
nodejs-github-bot pushed a commit to nodejs/node-v8 that referenced this issue Apr 4, 2024
@targos @nodejs-github-bot
src: stop using v8::BackingStore::Reallocate
5e6b013 
It's being deprecated by V8.
Explicitly allocate a new ArrayBuffer and copy the data instead.

Fixes: nodejs/node#52234
nodejs-github-bot pushed a commit to nodejs/node-v8 that referenced this issue Apr 5, 2024
@targos @nodejs-github-bot
src: stop using v8::BackingStore::Reallocate
d7e1b27 
It's being deprecated by V8.
Explicitly allocate a new ArrayBuffer and copy the data instead.

Fixes: nodejs/node#52234
nodejs-github-bot pushed a commit that referenced this issue Apr 5, 2024
@targos @joyeecheung
src: stop using v8::BackingStore::Reallocate
ec15504 
It's being deprecated by V8.
Explicitly allocate a new ArrayBuffer and copy the data when needed
instead.

Fixes: #52234
Co-authored-by: Joyee Cheung <joyeec9h3@gmail.com>
PR-URL: #52292
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
marco-ippolito pushed a commit that referenced this issue May 2, 2024
@targos @joyeecheung @marco-ippolito
src: stop using v8::BackingStore::Reallocate
9b239cd 
It's being deprecated by V8.
Explicitly allocate a new ArrayBuffer and copy the data when needed
instead.

Fixes: #52234
Co-authored-by: Joyee Cheung <joyeec9h3@gmail.com>
PR-URL: #52292
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
marco-ippolito pushed a commit that referenced this issue May 3, 2024
@targos @joyeecheung @marco-ippolito
src: stop using v8::BackingStore::Reallocate
7e35a16 
It's being deprecated by V8.
Explicitly allocate a new ArrayBuffer and copy the data when needed
instead.

Fixes: #52234
Co-authored-by: Joyee Cheung <joyeec9h3@gmail.com>
PR-URL: #52292
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@mildsunrise mildsunrise mentioned this issue Jun 23, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
deprecations Issues and PRs related to deprecations. v8 platform Issues and PRs related to Node's v8::Platform implementation.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

src: stop using v8::BackingStore::Reallocate targos/node
4 participants
@syg @joyeecheung @marco-ippolito @kylo5aby