Skip to content

Commit 4ee4be1

Browse files
authored
Fix Savepoint destructor exception-safety and rollback bookkeeping (#559)
## Summary Fixes two findings from the deep code review (SP-03, SP-02) in the `Savepoint` destructor. The documented auto-rollback-on-scope-exit semantics are unchanged. ### SP-03 — destructor could call `std::terminate` `~Savepoint()` caught only `SQLite::Exception`. Building the `"ROLLBACK TO SAVEPOINT " + msName` / `"RELEASE SAVEPOINT " + msName` strings can throw `std::bad_alloc`, and any non-`SQLite::Exception` thrown during cleanup would escape the implicitly `noexcept` destructor and terminate the process. The handler is now `catch (...)`, matching the stated intent ("Never throw an exception in a destructor"). ### SP-02 — no rolled-back state, fragile double-rollback There was a single `mbReleased` flag and no notion of "already rolled back", so after a manual `rollbackTo()` (or on the normal scope-exit path) the destructor issued a `ROLLBACK TO` and then a `RELEASE`, relying on SQLite tolerating a repeated `ROLLBACK TO`. A new `mbRolledBack` flag (set in `rollbackTo()`) lets the destructor do the minimum: skip the redundant `ROLLBACK TO` and just `RELEASE`. ## Test plan - Added `Savepoint.rollbackToThenRelease` covering a manual `rollbackTo()` followed by `release()` and a clean scope exit. - Built `SQLiteCpp_tests` (MSVC, Debug); `ctest -C Debug` 100% passed, including the existing `Savepoint.commitRollback` and `Savepoint.destructorSwallowsException` tests. No public API or behavioural change on the happy path; only the (previously UB-on-OOM) destructor error handling and the redundant-command path change.
2 parents 6c7a96f + 5e4908a commit 4ee4be1

5 files changed

Lines changed: 38 additions & 6 deletions

File tree

CHANGELOG.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -309,3 +309,5 @@ Version 3.4.0 - 2026 ???
309309
- Fix Column::operator<< to stream the exact column bytes via getString() (#556)
310310
- Restore the Coverity Scan static analysis as a GitHub Actions workflow, replacing the old Travis CI job
311311
- Fix Database::getHeaderInfo() signed-shift UB and use fixed-width types for the Header struct (#558)
312+
- Fix Savepoint destructor to catch all exceptions and track rollback state to avoid std::terminate (#559)
313+
- Fix Transaction destructor to catch all exceptions to avoid std::terminate (#559)

include/SQLiteCpp/Savepoint.h

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -90,9 +90,10 @@ class SQLITECPP_API Savepoint
9090
void rollback() { rollbackTo(); }
9191

9292
private:
93-
Database& mDatabase; ///< Reference to the SQLite Database Connection
94-
std::string msName; ///< Name of the Savepoint
95-
bool mbReleased = false; ///< True when release has been called
93+
Database& mDatabase; ///< Reference to the SQLite Database Connection
94+
std::string msName; ///< Name of the Savepoint
95+
bool mbReleased = false; ///< True when release has been called
96+
bool mbRolledBack = false; ///< True when a rollback to the savepoint has been done
9697
};
9798

9899
} // namespace SQLite

src/Savepoint.cpp

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,10 +40,13 @@ Savepoint::~Savepoint()
4040
{
4141
try
4242
{
43-
rollback();
43+
if (!mbRolledBack)
44+
{
45+
rollbackTo();
46+
}
4447
release();
4548
}
46-
catch (SQLite::Exception&)
49+
catch (...)
4750
{
4851
// Never throw an exception in a destructor: error if already released,
4952
// but no harm is caused by this.
@@ -71,6 +74,7 @@ void Savepoint::rollbackTo()
7174
if (!mbReleased)
7275
{
7376
mDatabase.exec(std::string("ROLLBACK TO SAVEPOINT ") + msName);
77+
mbRolledBack = true;
7478
}
7579
else
7680
{

src/Transaction.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ Transaction::~Transaction()
5555
{
5656
mDatabase.exec("ROLLBACK TRANSACTION");
5757
}
58-
catch (SQLite::Exception&)
58+
catch (...)
5959
{
6060
// Never throw an exception in a destructor: error if already rollbacked, but no harm is caused by this.
6161
}

tests/Savepoint_test.cpp

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,31 @@ TEST(Savepoint, commitRollback)
105105
EXPECT_EQ(1, nbRows);
106106
}
107107

108+
TEST(Savepoint, rollbackToThenRelease)
109+
{
110+
// Create a new database
111+
SQLite::Database db(":memory:", SQLite::OPEN_READWRITE | SQLite::OPEN_CREATE);
112+
EXPECT_EQ(SQLite::OK, db.getErrorCode());
113+
db.exec("CREATE TABLE test (id INTEGER PRIMARY KEY, value TEXT)");
114+
115+
{
116+
SQLite::Savepoint savepoint(db, "sp");
117+
118+
EXPECT_EQ(1, db.exec("INSERT INTO test VALUES (NULL, 'rolled back')"));
119+
120+
// A manual rollback leaves the savepoint on the stack, so releasing it afterwards succeeds.
121+
savepoint.rollbackTo();
122+
EXPECT_NO_THROW(savepoint.release());
123+
124+
// end of scope: already released, the destructor must do nothing and not throw
125+
}
126+
127+
// The rolled-back insert must not be persisted
128+
SQLite::Statement query(db, "SELECT COUNT(*) FROM test");
129+
ASSERT_TRUE(query.executeStep());
130+
EXPECT_EQ(0, query.getColumn(0).getInt());
131+
}
132+
108133
TEST(Savepoint, destructorSwallowsException)
109134
{
110135
// Create a new database

0 commit comments

Comments
 (0)