-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
napi_detach_arraybuffer aborts in Node 12.16.2 with an external arraybuffer #33022
Comments
From the above repo:
|
cc: @nodejs/n-api |
This is caused by double detach rooted in #30551. /cc @addaleax In v13.x and later V8 added a guard to silenced the double detach error https://github.com/nodejs/node/blob/master/deps/v8/src/objects/js-array-buffer.cc#L76, so this fatal error will not be reproducible on v13 and later. |
@legendecas nice find! could this be related to some crashes I’m seeing in Node 12 that involves console.log’ing a buffer with a detached arraybuffer as well? (Works in 13+) |
Not sure if there is no further info. Notably, this issue is not related to console.log, but on the finalization of the detached array buffers. If you can get backtrace similar to the one above, then this might be the cause. |
@legendecas i can only reproduce that one with a pointer allocated with libsodiums malloc in a very simple detach and print case. Let me see if I can get more info |
@emilbayes Thanks for including a stack trace here! This should be relatively straightforward to fix. |
These calls could fail if the `ArrayBuffer` had already been explicitly detached at some point in the past. The necessary test changes already came with 4f523c2 and could be ported back to v12.x with a backport of this PR. Fixes: nodejs#33022 Refs: nodejs#30551
#33039 should be enough to address this. |
Yay! Thanks :) |
These calls could fail if the `ArrayBuffer` had already been explicitly detached at some point in the past. The necessary test changes already came with 4f523c2 and could be ported back to v12.x with a backport of this PR. Fixes: #33022 Refs: #30551 PR-URL: #33039 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]>
These calls could fail if the `ArrayBuffer` had already been explicitly detached at some point in the past. The necessary test changes already came with 4f523c2 and could be ported back to v12.x with a backport of this PR. Fixes: #33022 Refs: #30551 PR-URL: #33039 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]>
These calls could fail if the `ArrayBuffer` had already been explicitly detached at some point in the past. The necessary test changes already came with 4f523c2 and could be ported back to v12.x with a backport of this PR. Fixes: #33022 Refs: #30551 PR-URL: #33039 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]>
These calls could fail if the `ArrayBuffer` had already been explicitly detached at some point in the past. The necessary test changes already came with 4f523c2 and could be ported back to v12.x with a backport of this PR. Fixes: #33022 Refs: #30551 PR-URL: #33039 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]>
These calls could fail if the `ArrayBuffer` had already been explicitly detached at some point in the past. The necessary test changes already came with 4f523c2 and could be ported back to v12.x with a backport of this PR. Fixes: #33022 Refs: #30551 PR-URL: #33039 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]>
What steps will reproduce the bug?
When using the napi_detach_arraybuffer api in Node.js 12.16.2, with an arraybuffer created with napi_create_external_arraybuffer that has a finalise method Node aborts with a V8 error.
(Additionally a warning is produced at compile with missing headers for napi_detach_arraybuffer)
I managed to make a simple test case for it here, https://github.com/mafintosh/failing-napi-detach-with-finalise
If you install and run
example.js
on latest Node.js 12, it aborts. The example simply allocates and detaches an externally created arraybuffer with a dummy finaliser.If I do the same thing without the finaliser it seems to work.
On Node.js 13+ it always seem to work in both cases.
The text was updated successfully, but these errors were encountered: