Skip to content

use v8 copy constructor for v8 global#6312

Merged
anonrig merged 1 commit intomainfrom
yagiz/fix-v8-global-rust
Mar 13, 2026
Merged

use v8 copy constructor for v8 global#6312
anonrig merged 1 commit intomainfrom
yagiz/fix-v8-global-rust

Conversation

@anonrig
Copy link
Member

@anonrig anonrig commented Mar 12, 2026

Create a proper independent persistent handle via V8's copy constructor. The previous implementation would cause a double-free when both the original and clone were dropped.

@anonrig anonrig requested review from a team as code owners March 12, 2026 18:48
@anonrig anonrig requested a review from mikea March 12, 2026 18:49
Copy link
Contributor

@ask-bonk ask-bonk bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm Bonk, and I've done a quick review of your PR.

This fixes a double-free bug in Global<T> cloning by replacing a bitwise pointer copy with V8's proper v8::Global<T>(Isolate*, const Global&) copy constructor, and removes the Clone trait impl in favor of an inherent method that takes &mut Lock.

The fix is correct — the old implementation created two v8::Global handles that shared the same persistent handle slot, so dropping both would double-free. One observation:

  1. Low: No regression test — This was a serious memory safety bug (double-free). It would be good to add a test in src/rust/jsg-test/ that exercises Global<T>::clone() and verifies both original and clone can be independently dropped and dereferenced. Not blocking, but worth tracking.

@ask-bonk
Copy link
Contributor

ask-bonk bot commented Mar 12, 2026

I'm Bonk, and I've done a quick review of your PR.

This fixes a double-free bug in Global<T> cloning by replacing a bitwise pointer copy with V8's proper v8::Global<T>(Isolate*, const Global&) copy constructor, and removes the Clone trait impl in favor of an inherent method that takes &mut Lock.

The fix is correct. The C++ side properly uses global_as_ref_from_ffi to get a const reference to the original v8::Global<v8::Value> and constructs a new independent persistent handle via v8::Global<v8::Value>(isolate, original). The Rust side correctly removes the Clone trait (which couldn't provide an isolate) and replaces it with an inherent clone(&self, lock: &mut Lock) method. The #[must_use] annotation and the explanatory comment are good additions.

One low-severity observation posted: no regression test for the double-free fix. Not blocking.

github run

@anonrig anonrig enabled auto-merge March 12, 2026 18:53
@anonrig anonrig force-pushed the yagiz/fix-v8-global-rust branch from afe997f to 1d9be5a Compare March 12, 2026 20:28
@anonrig anonrig force-pushed the yagiz/fix-v8-global-rust branch from 1d9be5a to 3204893 Compare March 12, 2026 22:57
@anonrig anonrig merged commit 6f80709 into main Mar 13, 2026
43 of 47 checks passed
@anonrig anonrig deleted the yagiz/fix-v8-global-rust branch March 13, 2026 00:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants