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

v8::internal::GlobalBackingStoreRegistry::Register: Check failed: result.second. #161

Closed
mischnic opened this issue Apr 22, 2022 · 5 comments

Comments

@mischnic
Copy link
Contributor

parcel-bundler/parcel#7979 has introduced another one of these errors:

#
# Fatal error in , line 0
# Check failed: result.second.
#
#
#
#FailureMessage Object: 0x7ff7b57cb950
 1: 0x10a841dca node::NodePlatform::GetStackTracePrinter()::$_3::__invoke() [/usr/local/Cellar/node@16/16.14.2/bin/node]
 2: 0x10b0434c1 V8_Fatal(char const*, ...) [/usr/local/Cellar/node@16/16.14.2/bin/node]
 3: 0x10ab2f244 v8::internal::GlobalBackingStoreRegistry::Register(std::__1::shared_ptr<v8::internal::BackingStore>) [/usr/local/Cellar/node@16/16.14.2/bin/node]
 4: 0x10a93076b v8::ArrayBuffer::GetBackingStore() [/usr/local/Cellar/node@16/16.14.2/bin/node]
 5: 0x10a7af3a1 napi_get_typedarray_info [/usr/local/Cellar/node@16/16.14.2/bin/node]
 6: 0x10d25b264 getViewAddress(napi_env__*, napi_callback_info__*) [/Users/niklas/Desktop/napi-backing-buffer/node_modules/lmdb-darwin-x64/node.napi.glibc.node]
 7: 0x10a7b0762 v8impl::(anonymous namespace)::FunctionCallbackWrapper::Invoke(v8::FunctionCallbackInfo<v8::Value> const&) [/usr/local/Cellar/node@16/16.14.2/bin/node]
 8: 0x10a96c1e3 v8::internal::FunctionCallbackArguments::Call(v8::internal::CallHandlerInfo) [/usr/local/Cellar/node@16/16.14.2/bin/node]
 9: 0x10a96bc14 v8::internal::MaybeHandle<v8::internal::Object> v8::internal::(anonymous namespace)::HandleApiCallHelper<false>(v8::internal::Isolate*, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::FunctionTemplateInfo>, v8::internal::Handle<v8::internal::Object>, v8::internal::BuiltinArguments) [/usr/local/Cellar/node@16/16.14.2/bin/node]
10: 0x10a96b3f3 v8::internal::Builtin_HandleApiCall(int, unsigned long*, v8::internal::Isolate*) [/usr/local/Cellar/node@16/16.14.2/bin/node]
11: 0x10af61c39 Builtins_CEntry_Return1_DontSaveFPRegs_ArgvOnStack_BuiltinExit [/usr/local/Cellar/node@16/16.14.2/bin/node]
error Command failed with signal "SIGTRAP".
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

Reproduction (haven't found one without Parcel yet):
index.js:

import "@babel/types/lib/ast-types/generated/index.js";
import "@babel/core/lib/config/cache-contexts.js";

package.json:

{
  "dependencies": {
    "@babel/core": "^7.17.9",
    "@babel/types": "^7.17.0",
    "parcel": "2.0.0-nightly.1067"
  },
  "scripts": {
    "build": "rm -rf .parcel-cache; PARCEL_WORKERS=0 parcel build index.js --no-cache"
  },
  "resolutions": {
    "lmdb": "2.3.5"
  }
}

run yarn build

kriszyp added a commit that referenced this issue Apr 22, 2022
…nal empty buffer situation that parcel often generates, #161
kriszyp added a commit that referenced this issue Apr 22, 2022
…nal empty buffer situation that parcel often generates, #161
@kriszyp
Copy link
Owner

kriszyp commented Apr 22, 2022

Thanks for the great test @mischnic . I think I have finally gotten to the bottom of this. I have long been puzzled by these errors since lmdb-js has plenty of unit tests for empty/zero-length buffers, but somehow parcel is able to generate some different/exotic variety of these that crashes node when getting address information for them. As it turns, the specific/minimum buffer that causes this issue of crashing node when attempting to get the address/size of a buffer is when there is more than one external buffer, with a zero length and that both point to the same non-null address and they are both accessed by getting their address/info.

Of course this goes a little deeper than just lmdb-js. I am not exactly sure of the (implicit) expectations/invariants of external buffer address/sizes. Generally from what I have seen, if you create empty buffers through V8 that are given a null pointer as an address. I don't know if this is considered a requirement though. And it would seem that something in parcel (or rust) is probably creating an external zero-length buffer with a non-null address. And until there are multiple with the same address where more than one have contents that are accessed, it can get away with it. On the other hand, if this is indeed a legitimate use of external buffers, node should probably have an extra check for zero length buffers before calling GetBackingStore in https://github.com/nodejs/node/blob/master/src/js_native_api_v8.cc#L2924. I am not entirely sure which side to blame though, although I do think that since V8 version 8.x that multiple buffers are not allowed to have the same address (regardless of length, and they have assertions to this effect).

Fortunately, the work around in lmdb-js is pretty trivial. I just check for zero length before getting the address. I have generally avoided this just to eliminate any extra overhead (and again, I don't think I should have to do this), but knowing this can only originate from externally allocated buffers, I can isolate this to a lesser-used branch.

Anyway, thanks again for the great test case. I'll run my CI tests, and try to get an updated patch out shortly.

@kriszyp
Copy link
Owner

kriszyp commented Apr 22, 2022

Ok, fix published in v2.3.6.

@mischnic
Copy link
Contributor Author

mischnic commented Apr 23, 2022

Thank you for the thorough investigation!

I don't think I should have to do this

I think so too.
We have run into this non-unique buffer address situation before: napi-rs/napi-rs#1124 (comment). The problematic buffers in my reproduction are coming from Rust via napi-rs.
Maybe napi-rs/napi-rs#1144 can fix this one and for all

@kriszyp
Copy link
Owner

kriszyp commented Apr 23, 2022

Wow, nice job tracking back to napi-rs and putting together a fix, that's awesome!

@mischnic
Copy link
Contributor Author

This should be fixed in Parcel with parcel-bundler/parcel#7995. And sorry for reporting this as an lmdb bug, when in reality Parcel simply passed you an invalid buffer 😄

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

No branches or pull requests

2 participants