Better-sqlite3: Unofficial fork: help with segmentation fault (core dumped)

[Prinzhorn]

This just randomly came up while I was working on tests for #367 (cc @missinglink). All I did was make slight changes to the missinglink:unsafe_mode branch. I haven't dug deeper but I assume a segfault is always bad and my changes kind of did a fuzzing? Now the question is: did I unravel an existing bug or can unsafe actually cause a segfault? Which would be a heckin bold UNSAFE if I've ever seen one :smile:

Steps to reproduce:

1. git clone --single-branch --branch segfault [email protected]:Prinzhorn/better-sqlite3.git
2. cd better-sqlite3
3. npm install
4. npm test

  integrity checks
    Database#prepare()
      1) while iterating (allowed)
      2) while busy (blocked)
      3) while closed (blocked)
    Database#exec()
      4) while iterating (blocked)
      5) while busy (blocked)
      6) while closed (blocked)
    Database#pragma()
      7) while iterating (blocked)
      8) while busy (blocked)
      9) while closed (blocked)
    Database#checkpoint()
      10) while iterating (blocked)
      11) while busy (blocked)
      12) while closed (blocked)
    Database#backup()
      13) while iterating (allowed)
(node:13917) UnhandledPromiseRejectionWarning: TypeError: The database connection is not open
    at Database.backup (/home/alex/Documents/Projects/Bandsalat/forks/better-sqlite3/lib/backup.js:31:33)
(node:13917) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 15)
(node:13917) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
      14) while busy (allowed)
(node:13917) UnhandledPromiseRejectionWarning: TypeError: The database connection is not open
    at Database.backup (/home/alex/Documents/Projects/Bandsalat/forks/better-sqlite3/lib/backup.js:31:33)
(node:13917) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 17)
      15) while closed (blocked)
(node:13917) UnhandledPromiseRejectionWarning: TypeError: The database connection is not open
    at Database.backup (/home/alex/Documents/Projects/Bandsalat/forks/better-sqlite3/lib/backup.js:31:33)
(node:13917) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 19)
    Database#function()
      16) while iterating (blocked)
      17) while busy (blocked)
      18) while closed (blocked)
    Database#aggregate()
      19) while iterating (blocked)
      20) while busy (blocked)
      21) while closed (blocked)
    Database#loadExtension()
      22) while iterating (blocked)
      23) while busy (blocked)
      24) while closed (blocked)
    Database#close()
Segmentation fault (core dumped)
npm ERR! Test failed.  See above for more details.

[missinglink]

I can confirm that your code causes a SEGFAULT.
I think you might have overstepped the 'potentially unsafe' territory into the 'completely unsafe' ;)

[missinglink]

The following command returns a different error message:

npm run rebuild-debug && npm t

...

  integrity checks
    Database#prepare()
      1) while iterating (allowed)
Assertion failed: (!db->GetState()->busy), function Statement, file ./src/objects/statement.lzz, line 70.
[1]    32448 abort      npm t

[missinglink]

I think maybe a BDD approach is good for this feature where we add the failing test case first and then rework the code to make it pass, rather than making loads of changes to the code and hoping it has the desired effect without breaking anything else.

As it stands right now the unsafe mode solves my specific problem (reading from a table using an iterator and writing to the same table while the iterator is running), so for me it already 'just works', so I'm reluctant to add anything to unsafe mode which would make either my usecase fail or the connection unreliable in any way.

What functionality are you trying to achieve?

[Prinzhorn]

What functionality are you trying to achieve?

Reading and writing from within the implementation of a user-defined function (#338) which seems to be allowed https://github.com/JoshuaWise/better-sqlite3/issues/338#issuecomment-592035102

Here's a new idea: what if we move unsafe from Database to Statement? Usually there are only a very limited number of cases where you need unsafe in a project. In all other places you still want safe-mode to keep you from accidentally shooting yourself in the foot. So enabling unsafe for one use-case should not affect unrelated portions of the same project.

From an API point of view two versions come to mind:

1. Statement#unsafe

Similar to pluck(true|false) you can enable unsafe(true|false) and unsafe-mode will be enabled for the duration of the query. So in my use-case executing a statement from inside a user-defined function would be no problem. However, iterators are not that trivial I guess. They don't have an "end" that better-sqlite3 controls.

1. Database#unsafe

Thanks to everything being synchronous we can declare an unsafe block

// You cannot do nasty things here

db.unsafe(() => {
    // You can do nasty things here
});

// You cannot do nasty things here

If we implement this we need to make sure that nested unsafe blocks work as expected. So instead of having a flag that is enabled on entering and disabled when the block exits we need to have a counter (or a stack) to keep track.

Both solutions by itself don't solve the segfault though. But I think they're the right way nonetheless.

[Prinzhorn]

Here's a separate comment regarding the segfault: I guess the assertion just needs a || db->unsafe_mode. So the test actually caught a bug in my changes. In contrast to db->iterators the db->busy flag is checked in more places, some of them assertions. So I can't just update the code analogous to yours.

[Prinzhorn]

I can't get more useful errors even with npm run rebuild-debug && npm test. I'm still getting the exact same output. Not sure what I'm doing wrong.

Does anything change for you when you update the line?

assert(!db->GetState()->busy || db->GetState()->unsafe_mode);

[missinglink]

I just added a test for my use-case in https://github.com/JoshuaWise/better-sqlite3/commit/7ad0b199822af7d24b48d65a2f8673bd0977364b, could you please provide something similar so I can understand what you're doing and reproduce it easily?

[missinglink]

When I pull you code and run npm run rebuild-release my copy of src/better_sqlite3.cpp is updated.

Are you sure you're generating the .cpp files from the .lzz files correctly and have .lzz installed?

➜  better-sqlite3 git:(segfault) git log -1
commit 7911e35801510055e880dea38efdbe28216ecdb0 (HEAD -> segfault, origin/segfault)
Author: Alexander Prinzhorn <[email protected]>
Date:   Wed Mar 25 10:22:11 2020 +0100

    segfault

➜  better-sqlite3 git:(segfault) git status
On branch segfault
Your branch is up to date with 'origin/segfault'.

nothing to commit, working tree clean

➜  better-sqlite3 git:(segfault) npm run rebuild-release

> [email protected] rebuild-release /private/tmp/better-sqlite3
> npm run lzz && npm run build-release


> [email protected] lzz /private/tmp/better-sqlite3
> lzz -hx hpp -sx cpp -k BETTER_SQLITE3 -d -hl -sl -e ./src/better_sqlite3.lzz


> [email protected] build-release /private/tmp/better-sqlite3
> node-gyp rebuild --release

  TOUCH b857c92884e9598d609f6be182a2595df7a8e00f.intermediate
  ACTION deps_sqlite3_gyp_locate_sqlite3_target_extract_sqlite3 b857c92884e9598d609f6be182a2595df7a8e00f.intermediate
  TOUCH Release/obj.target/deps/locate_sqlite3.stamp
  CC(target) Release/obj.target/sqlite3/gen/sqlite3/sqlite3.o
warning: unknown warning option '-Wno-cast-function-type'; did you mean
      '-Wno-bad-function-cast'? [-Wunknown-warning-option]
1 warning generated.
  LIBTOOL-STATIC Release/sqlite3.a
  CXX(target) Release/obj.target/better_sqlite3/src/better_sqlite3.o
./src/objects/database.lzz:131:23: warning: 'AtExit' is deprecated: Use the
      three-argument variant of AtExit() or AddEnvironmentCleanupHook()
      [-Wdeprecated-declarations]
                node::AtExit(Database::AtExit);
                      ^
/Users/peter/Library/Caches/node-gyp/12.16.1/include/node/node.h:693:1: note: 'AtExit'
      has been explicitly marked deprecated here
NODE_DEPRECATED(
^
/Users/peter/Library/Caches/node-gyp/12.16.1/include/node/node.h:93:20: note: expanded
      from macro 'NODE_DEPRECATED'
    __attribute__((deprecated(message))) declarator
                   ^
1 warning generated.
  SOLINK_MODULE(target) Release/better_sqlite3.node
  CC(target) Release/obj.target/test_extension/deps/test_extension.o
  SOLINK_MODULE(target) Release/test_extension.node
rm b857c92884e9598d609f6be182a2595df7a8e00f.intermediate

➜  better-sqlite3 git:(segfault) ✗ git status
On branch segfault
Your branch is up to date with 'origin/segfault'.

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
        modified:   src/better_sqlite3.cpp

no changes added to commit (use "git add" and/or "git commit -a")

[missinglink]

If I remove that line as such then the test output is the same as the one you first posted:

➜  better-sqlite3 git:(segfault) ✗ git diff src/objects/statement.lzz
diff --git a/src/objects/statement.lzz b/src/objects/statement.lzz
index 163d069..f0a65c0 100644
--- a/src/objects/statement.lzz
+++ b/src/objects/statement.lzz
@@ -67,7 +67,6 @@ private:
                assert(db != NULL);
                assert(handle != NULL);
                assert(db->GetState()->open);
-               assert(!db->GetState()->busy);
                db->AddStatement(this);
        }

...
(node:37425) UnhandledPromiseRejectionWarning: TypeError: The database connection is not open
    at Database.backup (/private/tmp/better-sqlite3/lib/backup.js:31:33)
(node:37425) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 19)
    Database#function()
      16) while iterating (blocked)
      17) while busy (blocked)
      18) while closed (blocked)
    Database#aggregate()
      19) while iterating (blocked)
      20) while busy (blocked)
      21) while closed (blocked)
    Database#loadExtension()
      22) while iterating (blocked)
      23) while busy (blocked)
      24) while closed (blocked)
    Database#close()
[1]    37424 segmentation fault  npm test

[Prinzhorn]

I think I've caught a cold and I'm lacking big brain energy right now. I'll get back to you when I can. Maybe you can put some thought into the unsafe-block I've suggested above.

[JoshuaWise]

Changed the title so thousands of people who use better-sqlite3 don't freak out and start thinking better-sqlite3 has a segfault. I'll also think about this when I have a bit more time (the company I work for is having a big launch today).

[missinglink]

Heya, so I had a bit of time to think about whether unsafe-mode is set during instantiation for the whole connection or variable per-statement and the conclusion I came to is... yes, function-scoped enable/disable sounds preferable 😄

The unsafe mode is intended for use-cases such as mine and yours, so naturally the semantics of it should allow for both methods.

I think the cons for your approach are fairly minor:

• We need to figure out how that would work and write reliable code for it (my version is dead simple), this has nesting to worry about.
• Users would need to modify their application code in order to nest under db.unsafe(() => { (positive/negative?)
• It's not always clear which block of code to wrap, and the read/write code may appear in different files, making it potentially difficult to wrap (possibly not an issue in practice)

And the pros:

• Nesting code under an explicit db.unsafe(() => would be positive for readability and debug-ability. I am concerned that Joshua is going to have a bunch of error reports due to people doing silly things under unsafe mode and so this change would actually be positive in that regard as it's clear whats going on when people post bug reports so he can respond appropriately.
• The last 'con' is also a pro (which block of code to wrap), meaning that it's now clear which part of the code is unsafe which again increases readability and makes group maintenance of the application code by different developers easier.

[missinglink]

In all honesty I will probably not find the time to rewrite unsafe mode to change how it operates but I'm happy to assist others from my own limited experience with the code 😸

[missinglink]

meh, I had a crack at it https://github.com/JoshuaWise/better-sqlite3/compare/master...missinglink:unsafe_statement
I don't find it very elegant to pass a boolean as the second argument to db.prepare but it works 🤷‍♂

[Prinzhorn]

@missinglink Nice! I'm not a fan of the second argument either, because it is much less explicit (and flexible?) than the approach in your PR or the unsafe-block. I'd even accept calling the block db.dangerouslyUnsafe (I like how React made it very explicit by calling their prop dangerouslySetInnerHTML). Or db.warantyVoidIfUsed() or db.lookMaNoHands() :smile:

Anyway, your two approaches get us closer to the solution. I haven't done any native Node.js stuff yet, so I will give it a shot as well. If things go well I'll open a PR today and we have something to discuss.

Changed the title so thousands of people who use better-sqlite3 don't freak out and start thinking better-sqlite3 has a segfault.

@JoshuaWise thanks, that didn't cross my mind

[missinglink]

Yeah, I diverged from the plan once I got into the code, I think the semantics you suggested are better but tacking it on to prepare() was the easiest code to write.

I actually have zero experience with c++ so I'm just making it all up as I go along :man_shrugging:

Another option is a 'namespace', so db.unsafe.prepare() if the db.unsafe(() => method is too hard to achieve.

[missinglink]

Regarding the naming, I was going to write up something about what is/isn't going to be accepted for inclusion into unsafe mode.

The tl;dr is that anything that can SEGFAULT or corrupt your database will never be included but things that result in undefined behaviour may be included.

So based off that we need to pick a word that indicates caution but not as far as to make people thing it can corrupt the database or crash their application.

[Prinzhorn]

The tl;dr is that anything that can SEGFAULT or corrupt your database will never be included but things that result in undefined behaviour may be included.

So based off that we need to pick a word that indicates caution but not as far as to make people thing it can corrupt the database or crash their application.

:100:

[missinglink]

One problem with that definition is that I believe my use-case will memory leak in certain situations and crash with OOM errors, so there's that..

[Prinzhorn]

One problem with that definition is that I believe my use-case will memory leak in certain situations and crash with OOM errors, so there's that..

My mantra is "first make it work, then make it great". So let's first get the API working and then we'll look into the details and edge cases.

[JoshuaWise]

Closing now that unsafe mode is available in v7.0.0