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

napi_detach_arraybuffer aborts in Node 12.16.2 with an external arraybuffer #33022

Closed
mafintosh opened this issue Apr 23, 2020 · 9 comments
Closed
Labels
node-api Issues and PRs related to the Node-API.

Comments

@mafintosh
Copy link
Member

  • Version: 12.6.2
  • Platform: mac
  • Subsystem: n-api

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.

@emilbayes
Copy link

From the above repo:

$ node example
ArrayBuffer {
  [Uint8Contents]: <00 00 00 00 00 00 00 00 ba 02>,
  byteLength: 10
}
ArrayBuffer { (detached), byteLength: 0 }
FATAL ERROR: v8::ArrayBuffer::Detach Only detachable ArrayBuffers can be detached
 1: 0x1010248bd node::Abort() (.cold.1) [/..../.nvm/versions/node/v12.16.2/bin/node]
 2: 0x100084c4d node::FatalError(char const*, char const*) [/..../.nvm/versions/node/v12.16.2/bin/node]
 3: 0x100084d8e node::OnFatalError(char const*, char const*) [/..../.nvm/versions/node/v12.16.2/bin/node]
 4: 0x1001afde1 v8::ArrayBuffer::Detach() [/..../.nvm/versions/node/v12.16.2/bin/node]
 5: 0x10004f7a0 v8impl::(anonymous namespace)::ArrayBufferReference::Finalize(bool) [/..../.nvm/versions/node/v12.16.2/bin/node]
 6: 0x1000643d2 napi_env__::~napi_env__() [/..../.nvm/versions/node/v12.16.2/bin/node]
 7: 0x1000642de napi_env__::~napi_env__() [/..../.nvm/versions/node/v12.16.2/bin/node]
 8: 0x10003d7f7 node::Environment::RunCleanup() [/..../.nvm/versions/node/v12.16.2/bin/node]
 9: 0x1000ba305 node::NodeMainInstance::Run() [/..../.nvm/versions/node/v12.16.2/bin/node]
10: 0x100061d41 node::Start(int, char**) [/..../.nvm/versions/node/v12.16.2/bin/node]
11: 0x7fff728ce3d5 start [/usr/lib/system/libdyld.dylib]
12: 0x2 
Abort trap: 6

@himself65 himself65 added the node-api Issues and PRs related to the Node-API. label Apr 23, 2020
@sam-github
Copy link
Contributor

cc: @nodejs/n-api

@legendecas
Copy link
Member

legendecas commented Apr 24, 2020

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.

@mafintosh
Copy link
Member Author

@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+)

@legendecas
Copy link
Member

legendecas commented Apr 24, 2020

@mafintosh: 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.

@mafintosh
Copy link
Member Author

mafintosh commented Apr 24, 2020

@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

@addaleax
Copy link
Member

@emilbayes Thanks for including a stack trace here!

This should be relatively straightforward to fix.

addaleax added a commit to addaleax/node that referenced this issue Apr 24, 2020
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
@addaleax
Copy link
Member

#33039 should be enough to address this.

@emilbayes
Copy link

Yay! Thanks :)

@targos targos closed this as completed in 36993c0 Apr 27, 2020
targos pushed a commit that referenced this issue Apr 27, 2020
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 <jasnell@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
BethGriggs pushed a commit that referenced this issue Apr 27, 2020
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 <jasnell@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
BridgeAR pushed a commit that referenced this issue Apr 28, 2020
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 <jasnell@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
BridgeAR pushed a commit that referenced this issue Apr 28, 2020
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 <jasnell@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
BethGriggs pushed a commit that referenced this issue Apr 28, 2020
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 <jasnell@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
node-api Issues and PRs related to the Node-API.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants