libstore: make SQLiteTxn resource-safe

it was possible to copy a transaction, with predictably bad effects.

Change-Id: Ibbcfa85a63b45296245223e26b9671c933467194

lix/libexpr/eval-cache.cc

@@ -59,7 +59,7 @@ struct AttrDb
         state->queryAttributes.create(state->db,
             "select name from Attributes where parent = ?");
 
-        state->txn = std::make_unique<SQLiteTxn>(state->db);
+        state->txn = std::make_unique<SQLiteTxn>(state->db.beginTransaction());
     }
 
     ~AttrDb()

lix/libstore/local-store.cc

@@ -110,7 +110,7 @@ void migrateCASchema(SQLite& db, Path schemaPath, AutoCloseFD& lockFd)
         }
 
         if (curCASchema < 2) {
-            SQLiteTxn txn(db);
+            SQLiteTxn txn = db.beginTransaction();
             // Ugly little sql dance to add a new `id` column and make it the primary key
             db.exec(R"(
                 create table Realisations2 (
@@ -140,7 +140,7 @@ void migrateCASchema(SQLite& db, Path schemaPath, AutoCloseFD& lockFd)
         }
 
         if (curCASchema < 3) {
-            SQLiteTxn txn(db);
+            SQLiteTxn txn = db.beginTransaction();
             // Apply new indices added in this schema update.
             db.exec(R"(
                 -- used by QueryRealisationReferences
@@ -151,7 +151,7 @@ void migrateCASchema(SQLite& db, Path schemaPath, AutoCloseFD& lockFd)
             txn.commit();
         }
         if (curCASchema < 4) {
-            SQLiteTxn txn(db);
+            SQLiteTxn txn = db.beginTransaction();
             db.exec(R"(
                 create trigger if not exists DeleteSelfRefsViaRealisations before delete on ValidPaths
                 begin
@@ -331,20 +331,20 @@ LocalStore::LocalStore(LocalStoreConfig config)
         openDB(*state, false);
 
         if (curSchema < 8) {
-            SQLiteTxn txn(state->db);
+            SQLiteTxn txn = state->db.beginTransaction();
             state->db.exec("alter table ValidPaths add column ultimate integer");
             state->db.exec("alter table ValidPaths add column sigs text");
             txn.commit();
         }
 
         if (curSchema < 9) {
-            SQLiteTxn txn(state->db);
+            SQLiteTxn txn = state->db.beginTransaction();
             state->db.exec("drop table FailedPaths");
             txn.commit();
         }
 
         if (curSchema < 10) {
-            SQLiteTxn txn(state->db);
+            SQLiteTxn txn = state->db.beginTransaction();
             state->db.exec("alter table ValidPaths add column ca text");
             txn.commit();
         }
@@ -1115,7 +1115,7 @@ void LocalStore::registerValidPaths(const ValidPathInfos & infos)
     return retrySQLite([&]() {
         auto state(_dbState.lock());
 
-        SQLiteTxn txn(state->db);
+        SQLiteTxn txn = state->db.beginTransaction();
         StorePathSet paths;
 
         for (auto & [_, i] : infos) {
@@ -1498,7 +1498,7 @@ void LocalStore::invalidatePathChecked(const StorePath & path)
     retrySQLite([&]() {
         auto state(_dbState.lock());
 
-        SQLiteTxn txn(state->db);
+        SQLiteTxn txn = state->db.beginTransaction();
 
         if (isValidPath_(*state, path)) {
             StorePathSet referrers; queryReferrers(*state, path, referrers);
@@ -1749,7 +1749,7 @@ void LocalStore::addSignatures(const StorePath & storePath, const StringSet & si
     retrySQLite([&]() {
         auto state(_dbState.lock());
 
-        SQLiteTxn txn(state->db);
+        SQLiteTxn txn = state->db.beginTransaction();
 
         auto info = std::const_pointer_cast<ValidPathInfo>(queryPathInfoInternal(*state, storePath));
 

lix/libstore/nar-info-disk-cache.cc

@@ -193,7 +193,7 @@ public:
     {
         return retrySQLite([&]() {
             auto state(_state.lock());
-            SQLiteTxn txn(state->db);
+            SQLiteTxn txn = state->db.beginTransaction();
 
             // To avoid the race, we have to check if maybe someone hasn't yet created
             // the cache for this URI in the meantime.

lix/libstore/sqlite.cc

@@ -104,6 +104,11 @@ void SQLite::exec(const std::string & stmt)
     });
 }
 
+SQLiteTxn SQLite::beginTransaction()
+{
+    return SQLiteTxn(db);
+}
+
 uint64_t SQLite::getLastInsertedRowId()
 {
     return sqlite3_last_insert_rowid(db);
@@ -230,23 +235,22 @@ bool SQLiteStmt::Use::isNull(int col)
 
 SQLiteTxn::SQLiteTxn(sqlite3 * db)
 {
-    this->db = db;
     if (sqlite3_exec(db, "begin;", 0, 0, 0) != SQLITE_OK)
         SQLiteError::throw_(db, "starting transaction");
-    active = true;
+    this->db.reset(db);
 }
 
 void SQLiteTxn::commit()
 {
-    if (sqlite3_exec(db, "commit;", 0, 0, 0) != SQLITE_OK)
-        SQLiteError::throw_(db, "committing transaction");
-    active = false;
+    if (sqlite3_exec(db.get(), "commit;", 0, 0, 0) != SQLITE_OK)
+        SQLiteError::throw_(db.get(), "committing transaction");
+    (void) db.release(); // not a leak, the deleter only runs `rollback;`
 }
 
-SQLiteTxn::~SQLiteTxn()
+void SQLiteTxn::Rollback::operator()(sqlite3 * db)
 {
     try {
-        if (active && sqlite3_exec(db, "rollback;", 0, 0, 0) != SQLITE_OK)
+        if (sqlite3_exec(db, "rollback;", 0, 0, 0) != SQLITE_OK)
             SQLiteError::throw_(db, "aborting transaction");
     } catch (...) {
         ignoreExceptionInDestructor();

lix/libstore/sqlite.hh

@@ -32,6 +32,8 @@ enum class SQLiteOpenMode {
     Immutable,
 };
 
+class SQLiteTxn;
+
 /**
  * RAII wrapper to close a SQLite database automatically.
  */
@@ -53,6 +55,8 @@ struct SQLite
 
     void exec(const std::string & stmt);
 
+    SQLiteTxn beginTransaction();
+
     uint64_t getLastInsertedRowId();
 };
 
@@ -122,16 +126,17 @@ struct SQLiteStmt
  * RAII helper that ensures transactions are aborted unless explicitly
  * committed.
  */
-struct SQLiteTxn
+class SQLiteTxn
 {
-    bool active = false;
-    sqlite3 * db;
+    struct Rollback {
+        void operator()(sqlite3 * db);
+    };
+    std::unique_ptr<sqlite3, Rollback> db;
 
-    SQLiteTxn(sqlite3 * db);
+public:
+    explicit SQLiteTxn(sqlite3 * db);
 
     void commit();
-
-    ~SQLiteTxn();
 };