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

bug: segfault due to Bind() failure #1449

Closed
kewde opened this issue Mar 9, 2021 · 1 comment · Fixed by #1450
Closed

bug: segfault due to Bind() failure #1449

kewde opened this issue Mar 9, 2021 · 1 comment · Fixed by #1450

Comments

@kewde
Copy link
Collaborator

kewde commented Mar 9, 2021

@mohd-akram could you take a look at this please?

reproduce:

// tested with Node.js v14.15.0
var sqlite3 = require('sqlite3').verbose();
var db = new sqlite3.Database(':memory:');
db.serialize(function() {
  //db.run("CREATE TABLE lorem (info TEXT)");
  try {
    db.run("INSERT INTO lorem VALUES (?)", [{toString: 23}])
  } catch(e) {
      console.log("caught exception")
  }
});
db.close();

It seems like the V8 engine is erroring because it mistinterprets toString as a function.
Having looked at this deeper, many of the typecasts in statement.cc:BindParameters could technically fail, therefore it seems better to catch errors at a higher level. I've attempted to catch all errors of Bind but it doesn't seem to be throwing a C++ exception.

I've narrowed this specific case down to Statement:Bind() -> Statement:BindParameter() ->

    else if (source.IsObject()) {
        std::string val = source.ToString().Utf8Value();
        return new Values::Text(pos, val.length(), val.c_str());
    }

But I assume more calls in BindParameter could throw an exception.

The function Napi::String::Utf8Value raises an error, oddly enough it is part of the N-API code itself, so it should setup the error information for Napi::Error:New which throws a fatal abort because it can't retrieve the error information from napi_get_last_error_info.

stack trace:

> node index.js

[?]  Binding parameters to the RunBaton
FATAL ERROR: Error::New napi_get_last_error_info
 1: 0xa295b0 node::Abort() [node]
 2: 0x9782df node::FatalError(char const*, char const*) [node]
 3: 0x9782e8  [node]
 4: 0x9fd33b napi_fatal_error [node]
 5: 0x7a1caca7c1b6  [/home/user/projects/mapbox/node-sample/node_modules/sqlite3/lib/binding/napi-v3-linux-x64/node_sqlite3.node]
 6: 0x7a1caca7c3e7 Napi::Error::New(napi_env__*) [/home/user/projects/mapbox/node-sample/node_modules/sqlite3/lib/binding/napi-v3-linux-x64/node_sqlite3.node]
 7: 0x7a1caca7cf31 Napi::String::Utf8Value[abi:cxx11]() const [/home/user/projects/mapbox/node-sample/node_modules/sqlite3/lib/binding/napi-v3-linux-x64/node_sqlite3.node]
 8: 0x7a1caca9a781 node_sqlite3::Values::Field* node_sqlite3::Statement::BindParameter<int>(Napi::Value, int) [/home/user/projects/mapbox/node-sample/node_modules/sqlite3/lib/binding/napi-v3-linux-x64/node_sqlite3.node]
 9: 0x7a1caca9be4c node_sqlite3::Statement::RunBaton* node_sqlite3::Statement::Bind<node_sqlite3::Statement::RunBaton>(Napi::CallbackInfo const&, int, int) [/home/user/projects/mapbox/node-sample/node_modules/sqlite3/lib/binding/napi-v3-linux-x64/node_sqlite3.node]
10: 0x7a1caca9680a node_sqlite3::Statement::Run(Napi::CallbackInfo const&) [/home/user/projects/mapbox/node-sample/node_modules/sqlite3/lib/binding/napi-v3-linux-x64/node_sqlite3.node]
11: 0x7a1caca976d8 Napi::InstanceWrap<node_sqlite3::Statement>::InstanceMethodCallbackWrapper(napi_env__*, napi_callback_info__*) [/home/user/projects/mapbox/node-sample/node_modules/sqlite3/lib/binding/napi-v3-linux-x64/node_sqlite3.node]
12: 0x9e2f0f  [node]
13: 0xc0251b  [node]
14: 0xc03ac6  [node]
15: 0xc04146 v8::internal::Builtin_HandleApiCall(int, unsigned long*, v8::internal::Isolate*) [node]
16: 0x13a5919  [node]
Aborted (core dumped)
@mohd-akram
Copy link
Contributor

I think we can enable C++ exceptions now after #1368 by removing the NAPI_DISABLE_CPP_EXCEPTIONS=1 define in binding.gyp. This will cause exceptions to bubble up properly and allow them to be handled.

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 a pull request may close this issue.

2 participants