diff --git a/CHANGELOG.md b/CHANGELOG.md index 7fb52662..374dd5cd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -309,3 +309,5 @@ Version 3.4.0 - 2026 ??? - Fix Column::operator<< to stream the exact column bytes via getString() (#556) - Restore the Coverity Scan static analysis as a GitHub Actions workflow, replacing the old Travis CI job - Fix Database::getHeaderInfo() signed-shift UB and use fixed-width types for the Header struct (#558) +- Fix Savepoint destructor to catch all exceptions and track rollback state to avoid std::terminate (#559) +- Fix Transaction destructor to catch all exceptions to avoid std::terminate (#559) diff --git a/include/SQLiteCpp/Savepoint.h b/include/SQLiteCpp/Savepoint.h index 180f8ac4..f0b67701 100644 --- a/include/SQLiteCpp/Savepoint.h +++ b/include/SQLiteCpp/Savepoint.h @@ -90,9 +90,10 @@ class SQLITECPP_API Savepoint void rollback() { rollbackTo(); } private: - Database& mDatabase; ///< Reference to the SQLite Database Connection - std::string msName; ///< Name of the Savepoint - bool mbReleased = false; ///< True when release has been called + Database& mDatabase; ///< Reference to the SQLite Database Connection + std::string msName; ///< Name of the Savepoint + bool mbReleased = false; ///< True when release has been called + bool mbRolledBack = false; ///< True when a rollback to the savepoint has been done }; } // namespace SQLite diff --git a/src/Savepoint.cpp b/src/Savepoint.cpp index 5bdd0a8d..b0fa9e76 100644 --- a/src/Savepoint.cpp +++ b/src/Savepoint.cpp @@ -40,10 +40,13 @@ Savepoint::~Savepoint() { try { - rollback(); + if (!mbRolledBack) + { + rollbackTo(); + } release(); } - catch (SQLite::Exception&) + catch (...) { // Never throw an exception in a destructor: error if already released, // but no harm is caused by this. @@ -71,6 +74,7 @@ void Savepoint::rollbackTo() if (!mbReleased) { mDatabase.exec(std::string("ROLLBACK TO SAVEPOINT ") + msName); + mbRolledBack = true; } else { diff --git a/src/Transaction.cpp b/src/Transaction.cpp index 0e59e5f3..f31ef5e4 100644 --- a/src/Transaction.cpp +++ b/src/Transaction.cpp @@ -55,7 +55,7 @@ Transaction::~Transaction() { mDatabase.exec("ROLLBACK TRANSACTION"); } - catch (SQLite::Exception&) + catch (...) { // Never throw an exception in a destructor: error if already rollbacked, but no harm is caused by this. } diff --git a/tests/Savepoint_test.cpp b/tests/Savepoint_test.cpp index 082d5f34..a5c39423 100644 --- a/tests/Savepoint_test.cpp +++ b/tests/Savepoint_test.cpp @@ -105,6 +105,31 @@ TEST(Savepoint, commitRollback) EXPECT_EQ(1, nbRows); } +TEST(Savepoint, rollbackToThenRelease) +{ + // Create a new database + SQLite::Database db(":memory:", SQLite::OPEN_READWRITE | SQLite::OPEN_CREATE); + EXPECT_EQ(SQLite::OK, db.getErrorCode()); + db.exec("CREATE TABLE test (id INTEGER PRIMARY KEY, value TEXT)"); + + { + SQLite::Savepoint savepoint(db, "sp"); + + EXPECT_EQ(1, db.exec("INSERT INTO test VALUES (NULL, 'rolled back')")); + + // A manual rollback leaves the savepoint on the stack, so releasing it afterwards succeeds. + savepoint.rollbackTo(); + EXPECT_NO_THROW(savepoint.release()); + + // end of scope: already released, the destructor must do nothing and not throw + } + + // The rolled-back insert must not be persisted + SQLite::Statement query(db, "SELECT COUNT(*) FROM test"); + ASSERT_TRUE(query.executeStep()); + EXPECT_EQ(0, query.getColumn(0).getInt()); +} + TEST(Savepoint, destructorSwallowsException) { // Create a new database