From 8596da2fcadce7dae02cccc0cd40b13411acdba4 Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Mon, 20 Jan 2025 23:06:25 +0100 Subject: [PATCH] libstore: make SQLiteTxn resource-safe it was possible to copy a transaction, with predictably bad effects. Change-Id: Ibbcfa85a63b45296245223e26b9671c933467194 --- lix/libexpr/eval-cache.cc | 2 +- lix/libstore/local-store.cc | 18 +++++++++--------- lix/libstore/nar-info-disk-cache.cc | 2 +- lix/libstore/sqlite.cc | 18 +++++++++++------- lix/libstore/sqlite.hh | 17 +++++++++++------ 5 files changed, 33 insertions(+), 24 deletions(-) diff --git a/lix/libexpr/eval-cache.cc b/lix/libexpr/eval-cache.cc index 2ca311dbf..311905d0b 100644 --- a/lix/libexpr/eval-cache.cc +++ b/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(state->db); + state->txn = std::make_unique(state->db.beginTransaction()); } ~AttrDb() diff --git a/lix/libstore/local-store.cc b/lix/libstore/local-store.cc index af0100782..e8041e997 100644 --- a/lix/libstore/local-store.cc +++ b/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(queryPathInfoInternal(*state, storePath)); diff --git a/lix/libstore/nar-info-disk-cache.cc b/lix/libstore/nar-info-disk-cache.cc index a02c6c602..6c69567ab 100644 --- a/lix/libstore/nar-info-disk-cache.cc +++ b/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. diff --git a/lix/libstore/sqlite.cc b/lix/libstore/sqlite.cc index f3ccaf22f..aef6146b9 100644 --- a/lix/libstore/sqlite.cc +++ b/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(); diff --git a/lix/libstore/sqlite.hh b/lix/libstore/sqlite.hh index 01fb7e064..695c404d6 100644 --- a/lix/libstore/sqlite.hh +++ b/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 db; - SQLiteTxn(sqlite3 * db); +public: + explicit SQLiteTxn(sqlite3 * db); void commit(); - - ~SQLiteTxn(); };