Repository navigation
Weird and inexplicable crashes when using TypedArrayContents #922
Description
I'm getting really weird crashes with v8 array buffers / typed arrays
Base:
size_t numEntries = 2 * TOTAL_NUM_BANDS;
size_t numBytes = sizeof(float) * numEntries;
v8::Local<v8::ArrayBuffer> dataBuffer = v8::ArrayBuffer::New(v8::Isolate::GetCurrent(), numBytes);
v8::Local<v8::Float32Array> dataArr = v8::Float32Array::New(minMaxDataBuffer, 0, numEntries);
works:
auto dataPtr = static_cast<float*>(dataBuffer->GetContents().Data());
segfaults:
auto test = Nan::TypedArrayContents<float>(dataArr);
...but it doesn't segfault immediately, but rather a couple of seconds later, probably when some GC happens.
I don't even need to do anything with the pointer. It is enough to construct a (temporary) Nan::TypedArrayContents
I have digged through the nan_typedarray_contents.h header
once I comment out
data = static_cast<char*>(buffer->GetBackingStore()->Data()) + byte_offset;
there is no crash.
I tried to find out what this is, but now it's getting really weird:
auto dataPtr2 = static_cast<float*>(dataBuffer->GetBackingStore()->Data());
//auto dataPtr3 = static_cast<float*>(dataBuffer->GetBackingStore()->Data());
works.
Where as
auto dataPtr2 = static_cast<float*>(dataBuffer->GetBackingStore()->Data());
auto dataPtr3 = static_cast<float*>(dataBuffer->GetBackingStore()->Data());
(just the first call repeated)
segfaults immediately... I suppose this is a node bug?
I'm compiling against electron 8.0.0, node version should be 12.13.0
I'll try with a later node / electron version now
Activity
kkoopa commentedon Oct 20, 2021
Based on
auto dataPtr = static_cast<float*>(dataBuffer->GetContents().Data());working and it not crashing when you comment out https://github.com/nodejs/nan/blob/main/nan_typedarray_contents.h#L37, I suspect that something is off in a mismatch between the version of V8 being linked against and the V8 headers.https://github.com/nodejs/nan/blob/main/nan_typedarray_contents.h#L36-L40 identifies whether to dereference a class pointer or reference based on the V8 version indicated by the headers.
robinchrist commentedon Oct 20, 2021
Hmm, but that doesn't explain why a single call
static_cast<float*>(dataBuffer->GetBackingStore()->Data());works, but doing it twice crashesaddaleax commentedon Oct 20, 2021
If this is the case, my guess would be that this is a
std::shared_ptr<>ABI mismatch (for the temporarystd::shared_ptr<v8::BackingStore>). We’ve had problems with that on at least arm64 and Windows.robinchrist commentedon Oct 20, 2021
AMD64 and Linux... Yeah, an ABI mismatch is a possibility, but I don't know how I could prove or disprove this...
addaleax commentedon Oct 20, 2021
@robinchrist Can you maybe share the exact compiler version and flags used when building the addon?
kkoopa commentedon Oct 20, 2021
I missed that doing it twice causes a problem. However,
auto test = Nan::TypedArrayContents<float>(dataArr);seems a bit off,Nan::TypedArrayContentsdoes not permit assigning, copying or even moving.robinchrist commentedon Oct 20, 2021
Well that'd give a compile time error, so I'd say your "do not permit assigning" macro doesn't work as intended.
See godbolt example here: https://godbolt.org/z/j8584bYWj
kkoopa commentedon Oct 20, 2021
Strange, I did get a compile time error with Apple clang version 12.0.0 (clang-1200.0.32.29)
Target: x86_64-apple-darwin19.6.0 when trying it.
kkoopa commentedon Oct 20, 2021
Ah, problem is C++17.
kkoopa commentedon Oct 20, 2021
Trying to do a std::move() fails as expected. Perhaps C++17 allows this assignment expression because it is guaranteed to be identical to directly using the constructor as
foo(arg);? Either way, moving it should probably be fine, non-movability is just what we default to for all objects of this nature.robinchrist commentedon Oct 21, 2021
I've got a stacktrace with a debug version of Electron (9.0.0, but same for all other versions)
The assertion / check is raised in
backing-store.ccin theDCHECKLine (662)robinchrist commentedon Oct 21, 2021
@addaleax
Compile Flags for Electron 9 (that happens to be the version I have a debug build of...)
kkoopa commentedon Oct 22, 2021
Based on what @addaleax mentioned, the
std::shared_ptr<v8::BackingStore>in the backtrace does stick out.robinchrist commentedon Oct 22, 2021
Yeah it definitely sticks out... But I'm not sure where the ABI mismatch could be.
I'm definitely building against the right node version (otherwise the addon wouldn't be able to be loaded by node because the node ABI version is embedded in the path where the actual shared library is located)
kkoopa commentedon Oct 22, 2021
Are there threads involved somehow?
robinchrist commentedon Oct 22, 2021
Yes, in the original version. But this definitely happens inside the main loop (in an async worker).
I have prepared another minimal example (don't know whether I'll be able to push it today, it's 3AM), without any threads or whatever involved.
The issue depends on the size of the buffer!
Renderer crashes for
245761, but works for245760If you remove the
Float32Arrayand only return the Buffer, everything works fine.Yes, I have actually bisected the issue
robinchrist commentedon Oct 22, 2021
kkoopa commentedon Oct 23, 2021
I tried this and did not see a crash. Could this problem be specific to Electron or is it exposing a deeper flaw?
robinchrist commentedon Oct 25, 2021
Update:
Only in the release version does the renderer crash depend on the array size
For the debug version, the renderer crash happens regardless of the array size
devyetii commentedon Nov 21, 2021
I think this is related to what happens here : #892
robinchrist commentedon Nov 21, 2021
Oh yeah, I forgot to follow up.
TL;DR
I'm almost certain that this is an ABI issue
Electron and Node are built against libc++ whereas the default on Linux is still libstdc++
Everything was fine until "now" (meaning the point when I upgraded electron and thus node). The native node API was very C-ish, so the ABI issue which was there all the time didn't matter.
But at some point, the node guys decided that it would be a clever thing to change the API and now pass a non-trivial object, aka
shared_ptracross an ABI boundary.Spoiler: It is not a good idea.
While I understand the reason (more intuitive and idiomatic code within node), it is a huge issue for everyone using the native node API.
They do offer an alternative, N-API, but it may be a whole lot of work to port your addon from native node API to N-API.
So for me, the only viable solution was to port the entire addon (more than 10k lines) to N-API / node-addon-api.
I am still a bit mad about that decision from the node devs, but I have successfully ported the addon to node-addon-api / N-API and everything is working fine.
My personal recommendation:
Stop using nan and switch to node-addon-api. There is not a single feature I am missing (and I use quite a lot).
node-addon-api / N-API avoids subtle ABI issues and inherently solves the context awareness issue.
nan should be abandoned / archived, its use officially be discouraged and superseded by node-addon-api`
If you want more bloody details, here's a story from a guy on medium which almost applys 1:1
https://nornagon.medium.com/a-libc-odyssey-973e51649063
addaleax commentedon Nov 23, 2021
Not to deflect, but unfortunately we weren’t given a choice here – this came from V8/Chromium. They don’t prioritize situations like the Node.js one, where the V8 “embedder” doesn’t consist of a monolithic codebase and instead potentially includes sub-embedders like Electron and users who write addons against the V8 API.
👍 👍 👍
robinchrist commentedon Nov 23, 2021
Maybe this should be added to the "Known issues" section? And it would probably be good to place a warning at the very top
like
Opinions on this?
robinchrist commentedon Nov 23, 2021
Well, in that case I'm mad at the Chromium / V8 devs now 😁
kkoopa commentedon Nov 23, 2021
addaleax commentedon Nov 23, 2021
Right – to be clear, there’s also nothing wrong with NAN here. It’s Node.js that does more or less promise to expose a stable ABI through its module version number, and fails to do so here.