diff options
| author | Nathan Whitaker <17734409+nathanwhit@users.noreply.github.com> | 2024-06-13 15:31:42 -0700 |
|---|---|---|
| committer | GitHub <noreply@github.com> | 2024-06-13 22:31:42 +0000 |
| commit | 368eb9073bff776b8bb49480b98ca4628ebdc7cd (patch) | |
| tree | 50e76947b934312d0ebf6a6e95d8e2a48cc10f3b /cli/napi/js_native_api.rs | |
| parent | 4ec9250c409fc0734e192d6571b0cad3cbc8a7ee (diff) | |
fix(napi): Read reference ownership before calling finalizer to avoid crash (#24203)
Fixes #23493.
What was happening here was that napi-rs was freeing the napi reference
([here](https://github.com/napi-rs/napi-rs/blob/19e3488efcbc601afa1f11a979372eb6c5ea6130/crates/napi/src/bindgen_runtime/mod.rs#L62))
during its finalize callback (which we call
[here](https://github.com/denoland/deno/blob/fb31eaa9ca59f6daaee0210d5cd206185c7041b9/cli/napi/js_native_api.rs#L132)).
We then were [reading the `ownership`
field](https://github.com/denoland/deno/blob/fb31eaa9ca59f6daaee0210d5cd206185c7041b9/cli/napi/js_native_api.rs#L136)
of that freed reference.
For some reason on arm macs the freed memory gets zeroed, so the value
of `ownership` was `0` when we read it (i.e. it was
`ReferenceOwnership::Runtime`). We then freed it again (since we thought
we owned it), causing the segfault.
Diffstat (limited to 'cli/napi/js_native_api.rs')
| -rw-r--r-- | cli/napi/js_native_api.rs | 6 |
1 files changed, 4 insertions, 2 deletions
diff --git a/cli/napi/js_native_api.rs b/cli/napi/js_native_api.rs index fad13ba62..fe6535446 100644 --- a/cli/napi/js_native_api.rs +++ b/cli/napi/js_native_api.rs @@ -127,13 +127,16 @@ impl Reference { let finalize_hint = reference.finalize_hint; reference.reset(); + // copy this value before the finalize callback, since + // it might free the reference (which would be a UAF) + let ownership = reference.ownership; if let Some(finalize_cb) = finalize_cb { unsafe { finalize_cb(reference.env as _, finalize_data, finalize_hint); } } - if reference.ownership == ReferenceOwnership::Runtime { + if ownership == ReferenceOwnership::Runtime { unsafe { drop(Reference::from_raw(reference)) } } } @@ -3440,7 +3443,6 @@ fn napi_add_finalizer( } else { ReferenceOwnership::Userland }; - let reference = Reference::new( env, value.into(), |