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

segmentation fault / EXC_BAD_ACCESS / KERN_INVALID_ADDRESS when closing DB when a query is in progress #1323

Closed
jjhbw opened this issue May 6, 2020 · 6 comments · Fixed by #1368

Comments

@jjhbw
Copy link

jjhbw commented May 6, 2020

Hi all, thanks for this fantastic library. I have used it with great success in a few projects.

I encountered a crash (Fatal Error: EXC_BAD_ACCESS / KERN_INVALID_ADDRESS) when closing the DB while a query is still in progress. I'm running in Electron 8.2.5. No exception was thrown, just a complete crash.

Versions used:
Sqlite3: 4.2.0
Electron: 8.2.5
Mac OS X: 10.14.6

I've attached the crash dump as recorded by Sentry
90104e4407e84e8793326bc199d03b4e-symbolicated.crash.zip

AFAIU sqlite3 should wait until all queries are finished before closing the database, right?

Let me know if you want me to build a repro case. This requires a basic Electron setup, as I couldn't get it to reproduce in Node 14x. I don't have the time for that atm, so i figured i'd make this issue first.

I'm not super familiar with Node native extensions, but let me know if you think i can help in any way.

@kewde
Copy link
Collaborator

kewde commented May 8, 2020

@jjhbw Thank you for reporting this issue, it's appreciated!

Please PR a test case that reproduces this issue if you have the time, thank you.
I'd suggest a little test case here.

AFAIU sqlite3 should wait until all queries are finished before closing the database, right?
Correct, closing is split into three stages:

  • Before Close: sets a variable that the database will be closing, no work is allowed on the queue after this anymore.
  • Close: Actually close the database.
  • After Close: clean up some things.

@jjhbw
Copy link
Author

jjhbw commented May 9, 2020

This should do it: https://github.com/jjhbw/electron-node-sqlite-repro-1323
See comment below; it also crashes in a simple Nodejs process. No electron required.

I couldn't get it to reproduce reliably so i'm afraid that this example is stochastic: it should crash after a few tries. A longer running query may do the trick in one go.

Let me know if you have any questions regarding the details of the setup!

@jjhbw
Copy link
Author

jjhbw commented May 9, 2020

Oh, wow, just noticed you don't need Electron for this at all!

Running the below in Node 14.x crashes with Segmentation fault: 11 after a while.

const sqlite3 = require('sqlite3')
const path = require('path')

setInterval(() => {
    const db = new sqlite3.Database(path.join(__dirname, 'test.db'))
    setTimeout(() => db.all('select * from foo limit 1;', console.log), 1)
    db.close(err => console.log(err))
}, 500)

@jjhbw jjhbw changed the title EXC_BAD_ACCESS / KERN_INVALID_ADDRESS when closing DB when a query is in progress in Electron 8.2.5 segmentation fault / EXC_BAD_ACCESS / KERN_INVALID_ADDRESS when closing DB when a query is in progress May 9, 2020
@jjhbw
Copy link
Author

jjhbw commented Jul 1, 2020

Tested it with the above snippet. It is still a thing in 5.0.0, I'm afraid.

@mohd-akram
Copy link
Contributor

Could you try this with #1368?

@jjhbw
Copy link
Author

jjhbw commented Aug 7, 2020

@mohd-akram awesome, it seems to work when i build the library from source on macOS.

I have made a PR with the test I used (https://github.com/mohd-akram/node-sqlite3/pull/1). It is a rather informal, non-deterministic test case so feel free to discard it if you'd rather use something more formal to verify that this works correctly.

The test just tries to trigger the segfault ad nauseam for a few seconds (and successfully does so within <1 sec on master 6e250c6).

@jjhbw jjhbw mentioned this issue Aug 7, 2020
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.

3 participants