sqlite: fix stack-use-after-scope with function callback

[ndossche]

The hasIt block has Local<Function>s, but it's capture in the lambda, yet the lambda is used after the locals go out of scope.

This can be triggered by just running the test suite under ASAN.

ASAN report:

==202109==ERROR: AddressSanitizer: stack-use-after-scope on address 0x71bcb8cc6b40 at pc 0x5e562e3d0bfe bp 0x7fff740e23e0 sp 0x7fff740e23d8
READ of size 8 at 0x71bcb8cc6b40 thread T0
    #0 0x5e562e3d0bfd in v8::Function* v8::api_internal::IndirectHandleBase::value<v8::Function, false>() const /work/node/out/../deps/v8/include/v8-handle-base.h:90:62
    #1 0x5e562e3c9ee2 in v8::Local<v8::Function>::operator->() const /work/node/out/../deps/v8/include/v8-local-handle.h:382:59
    #2 0x5e562f5603cd in node::sqlite::DatabaseSync::ApplyChangeset(v8::FunctionCallbackInfo<v8::Value> const&)::$_1::operator()(std::basic_string_view<char, std::char_traits<char>>) const /work/node/out/../src/node_sqlite.cc:1907:14
    #3 0x5e562f55fd42 in bool std::__invoke_impl<bool, node::sqlite::DatabaseSync::ApplyChangeset(v8::FunctionCallbackInfo<v8::Value> const&)::$_1&, std::basic_string_view<char, std::char_traits<char>>>(std::__invoke_other, node::sqlite::DatabaseSync::ApplyChangeset(v8::FunctionCallbackInfo<v8::Value> const&)::$_1&, std::basic_string_view<char, std::char_traits<char>>&&) /usr/lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/invoke.h:61:14
    #4 0x5e562f55fc3c in std::enable_if<is_invocable_r_v<bool, node::sqlite::DatabaseSync::ApplyChangeset(v8::FunctionCallbackInfo<v8::Value> const&)::$_1&, std::basic_string_view<char, std::char_traits<char>>>, bool>::type std::__invoke_r<bool, node::sqlite::DatabaseSync::ApplyChangeset(v8::FunctionCallbackInfo<v8::Value> const&)::$_1&, std::basic_string_view<char, std::char_traits<char>>>(node::sqlite::DatabaseSync::ApplyChangeset(v8::FunctionCallbackInfo<v8::Value> const&)::$_1&, std::basic_string_view<char, std::char_traits<char>>&&) /usr/lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/invoke.h:114:9
    #5 0x5e562f55f8b4 in std::_Function_handler<bool (std::basic_string_view<char, std::char_traits<char>>), node::sqlite::DatabaseSync::ApplyChangeset(v8::FunctionCallbackInfo<v8::Value> const&)::$_1>::_M_invoke(std::_Any_data const&, std::basic_string_view<char, std::char_traits<char>>&&) /usr/lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/std_function.h:290:9
    #6 0x5e562f57745b in std::function<bool (std::basic_string_view<char, std::char_traits<char>>)>::operator()(std::basic_string_view<char, std::char_traits<char>>) const /usr/lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/std_function.h:591:9
    #7 0x5e562f53c8bb in node::sqlite::xFilter(void*, char const*) /work/node/out/../src/node_sqlite.cc:1814:10
    #8 0x71bcbb3dbf3a  (/lib/x86_64-linux-gnu/libsqlite3.so.0+0xdaf3a) (BuildId: ac2bec9c45eec6feab0928b3b1373b467aa51339)
    #9 0x71bcbb3dc9f6 in sqlite3changeset_apply (/lib/x86_64-linux-gnu/libsqlite3.so.0+0xdb9f6) (BuildId: ac2bec9c45eec6feab0928b3b1373b467aa51339)
    #10 0x5e562f53c035 in node::sqlite::DatabaseSync::ApplyChangeset(v8::FunctionCallbackInfo<v8::Value> const&) /work/node/out/../src/node_sqlite.cc:1919:11
    #11 0x71bc99ad2f8c in Builtins_CallApiCallbackGeneric embedded.o

Address 0x71bcb8cc6b40 is located in stack of thread T0 at offset 832 in frame
    #0 0x5e562f53ad3f in node::sqlite::DatabaseSync::ApplyChangeset(v8::FunctionCallbackInfo<v8::Value> const&) /work/node/out/../src/node_sqlite.cc:1817

Note: this was found by a static-dynamic analyser I'm developing.

[nodejs-github-bot]

Review requested:

• @nodejs/sqlite

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.33%. Comparing base (40dc5a1) to head (48fc045).
⚠️ Report is 15 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #63640      +/-   ##
==========================================
+ Coverage   90.30%   90.33%   +0.03%     
==========================================
  Files         730      732       +2     
  Lines      234802   236457    +1655     
  Branches    43957    44525     +568     
==========================================
+ Hits       212041   213610    +1569     
- Misses      14485    14548      +63     
- Partials     8276     8299      +23     

Files with missing lines	Coverage Δ
src/node_sqlite.cc	80.63% <100.00%> (+0.01%)	⬆️

... and 49 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[addaleax]

The hasIt block has Local<Function>s, but it's capture in the lambda, yet the lambda is used after the locals go out of scope.

Sure, but they are copied by value, not by reference, so that shouldn't be inherently problematic.

The real issue here to me seems to be that we're placing Local<> objects in the function's capture list and storing it in a std::function, which is heap-allocated, while V8's API contract says that Local<> objects are only allowed on the stack.

We could pass either Local<>* pointers here (in which case, yes, the Local<>s would need to be placed where this PR currently places them) or Global<> objects instead.