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

fix(napi): Read reference ownership before calling finalizer to avoid crash #24203

Merged
merged 3 commits into from
Jun 13, 2024

Conversation

nathanwhit
Copy link
Member

Fixes #23493.

What was happening here was that napi-rs was freeing the napi reference (here) during its finalize callback (which we call here). We then were reading the ownership field 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.

@nathanwhit nathanwhit requested a review from devsnek June 13, 2024 22:00
@nathanwhit nathanwhit enabled auto-merge (squash) June 13, 2024 22:05
@nathanwhit nathanwhit merged commit 368eb90 into denoland:main Jun 13, 2024
17 checks passed
@nathanwhit nathanwhit deleted the napi-segfault branch June 13, 2024 22:38
bartlomieju pushed a commit that referenced this pull request Jun 18, 2024
… 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.
zebreus pushed a commit to zebreus/deno that referenced this pull request Jul 8, 2024
… crash (denoland#24203)

Fixes denoland#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.
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.

Panic in NAPI when using pl.format() with expressions in Polars Node.js
2 participants