Consider detecting use of Connection after close

[Shimuuar]

SQLite's internal sqlite3 struct is allocated by malloc and freed after database is closed. So any attempt to use database after it's closed results in use after free, malloc's data structures corruption, segfaults and all kind of problems. It would be nice to add some check to convert any such attempt to exceptions.

I ran into this problem when test suite (thankfully not a production) crashed occasionally. It turned out that due to race condition during cleanup it was possible to use database right after it was closed. It took a lot of time to figure out what was going on

[sigrlami]

@Shimuuar thanks for the report. I will try to narrow it down and add additional checks.

[nurpax]

Probably should go into direct-sqlite. I'd maybe just add a "closed" flag in the connection handle, and check that before closing the real handle.

I don't remember how the connection datatype looks like in direct-sqlite, maybe it needs to wrap the actual C sqlite handle.

[Shimuuar]

Connection is just a few layers of newtypes over bare Ptr which is malloc'd by sqlite during open and freed in close

Challenge in adding "closed" flag I think is adding it in the way which doesn't allow for race conditions and is fast at the same time.

[nurpax]

Well, you're supposed to call close only once per connection, so I don't see how it could possibly be a perf issue to check if closed is set when closing the underlying connection.

Sure you'll have one indirection to get to the connection, but that really cannot be a problem, given how much sqlite3 calls themselves cost.

Re races: not sure about this. Seems like normally you wouldn't have two threadds fighting for their turn to close a connection. In general, the only stable way to use SQLite3 from multiple threads was to anyway effectively single-thread access with an MVar ove the connection.

[Shimuuar]

Every database operation should check whether connection is already closed or not. Otherwise it will start working with already free chunk of memory and will wreak malloc's data structures. Original problem arose because thread was working with database in short window between closing the DB and killing of thread

[nurpax]

Good point. Could the connection have a mutable reference like an IORef maybe that holds the connection, then it'd be set to Nothing on close? Then every function using the connection would have to first acquire the real connection using some getter function that raises an error if the connection was inexistent? It might in fact make sense to have this in sqlite-simple to keep direct-sqlite as lean as possible.

I don't see the IORef as a perf problem either. It's a simple check. But it'd be a large, though mechanical, change to sqlite-simple. Wouldn't probably change the public API at all.

[Shimuuar]

IORef won't give you thread safety. If connection is used in multiple threads it's still possible to get use after free. For example two threads racing

Th1. [ Check flag
	       [ SELECT
Th2.    [ flag := False
           [ Close connection

First thread check flag before it set to "closed" state, but runs query after database is closed. To provide thread safety MVar (Maybe Ptr) is needed. But then there's question on granularity of withMVar. Whether it should be used on level for direct-sqlite for each wrapper of C call or at level of sqlite-simple? (AFAIR sqlite-simple's functions make many different direct-sqlite calls underneath)

[nurpax]

Yes, I know -- I was just throwing the general pattern here.

Another consideration is what is the right way to use direct-sqlite (and SQLite3!) from multiple threads. In practice you need to protect your connection with an MVar anyway, or you'll hit those SQLITE_BUSY errors when one or more threads write at concurrently on the same connection.

So probably the MVar shouldn't go into direct-sqlite. But it might be beneficial to have a fix for the "closed/use after free" problem even in a thread unsafe manner. Because that bug can exist even in single-thread cases.

As a retired author/maintainer of sqlite-simple, I'm not in a position to make a call about MVar. I'd probably just make sure it's well documented and provide guidelines on what's a safe way to use this API from multiple threads. This, as opposed to MVaring the connection and making a large, breaking API change to sqlite-simple. There are a lot of users of sqlite-simple and many probably using sqlite in a single-threaded fashion. True concurrent database users are quite likely using something like Postgresql.

[sigrlami]

@nurpax thanks for the hints. I haven't got into details of that, want to reproduce case first, and find if that a typical issue. Are you suggesting it's better to improve documentation on how to use that rather than drastically change API?

[Shimuuar]

direct-sqlite is rather thin wrapper over C so it looks fair to just add warning in documentation and let user deal with it. It's in the spirit of C

I think it's possible to make sqlite-simple to track closed database and thread being thread safe without changing API at all. Here is general idea:

newtype Connection = Connection (MVar (Maybe Base.Database))

open :: String -> IO Connection
open fname = do
  c <- Base.open (T.pack fname)
  Connection <$> newMVar (Just c)

close :: Connection -> IO ()
close (Connection mvar) = modifyMVar_ mvar $ \case
  Nothing -> return Nothing
  Just c   -> Nothing <$ Base.close

execute :: (ToRow q) => Connection -> Query -> q -> IO ()
execute (Connection mvar)template qs = withMvar $ \case
  Nothing -> throwIO DatabaseAlreadyClosed
  Just c  -> 
    withStatementParams conn template qs $ \(Statement stmt) ->
      void . Base.step $ stmt

Such change is boilerplate heavy and requires to think when lock is taken to avoid deadlock. But it doesn't change API