From c4ca8c0b848ba221454d11a6883f1c62bde6a93c Mon Sep 17 00:00:00 2001
From: Wander Nauta <info@wandernauta.nl>
Date: Fri, 20 Dec 2024 21:56:13 +0100
Subject: [PATCH] Swallow commit() exception in MiniSQLite destructor, improve
 state check

This makes it so that MiniSQLite's destructor only attempts to commit if
a transaction is actually active, and also makes it so that a failure to
commit in the destructor is quietly ignored with no exception thrown.

Before, MiniSQLite carried a boolean flag to indicate whether the SQLite
database being wrapped was in a transaction, but this flag could diverge
from the actual state by...

- the "begin" query or "commit" query failing
- calling code mistakenly doing exec("rollback") or exec("commit")
- automatic rollbacks caused by e.g. disk I/O or disk full errors

In the last case, the error would cause an exception to be thrown, which
would trigger stack unwinding, destroying the MiniSQLite. The MiniSQLite
destructor, not having noticed the autorollback, would attempt a commit,
which would throw another exception, finally reaching std::terminate.

Also adds tests.
---
 sqlwriter.cc  | 17 ++++++++++++----
 sqlwriter.hh  |  2 +-
 testrunner.cc | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 70 insertions(+), 5 deletions(-)

diff --git a/sqlwriter.cc b/sqlwriter.cc
index aacde15..08a6105 100644
--- a/sqlwriter.cc
+++ b/sqlwriter.cc
@@ -225,12 +225,10 @@ void MiniSQLite::execPrep(const std::string& table, std::vector<std::unordered_m
 
 void MiniSQLite::begin()
 {
-  d_intransaction=true;
   exec("begin");
 }
 void MiniSQLite::commit()
 {
-  d_intransaction=false;
   exec("commit");
 }
 
@@ -239,6 +237,11 @@ void MiniSQLite::cycle()
   exec("commit;begin");
 }
 
+bool MiniSQLite::inTransaction()
+{
+  return sqlite3_get_autocommit(d_sqlite) == 0;
+}
+
 bool MiniSQLite::haveTable(const string& table)
 {
   return !getSchema(table).empty();
@@ -437,8 +440,14 @@ vector<std::unordered_map<string, MiniSQLite::outvar_t>> SQLiteWriter::queryGen(
 MiniSQLite::~MiniSQLite()
 {
   // needs to close down d_sqlite3
-  if(d_intransaction)
-    commit();
+  if(inTransaction()) {
+    try {
+      commit();
+    } catch (std::runtime_error &e) {
+      // don't have a good way to report e
+      // transaction will be rolled back instead
+    }
+  }
 
   for(auto& stmt: d_stmts)
     if(stmt.second)
diff --git a/sqlwriter.hh b/sqlwriter.hh
index a0b062b..0d3f74f 100644
--- a/sqlwriter.hh
+++ b/sqlwriter.hh
@@ -55,7 +55,7 @@ private:
   std::unordered_map<std::string, sqlite3_stmt*> d_stmts;
   std::vector<std::vector<std::string>> d_rows; // for exec()
   static int helperFunc(void* ptr, int cols, char** colvals, char** colnames);
-  bool d_intransaction{false};
+  bool inTransaction();
   bool haveTable(const std::string& table);
 };
 
diff --git a/testrunner.cc b/testrunner.cc
index f6ef378..ea9bf7b 100644
--- a/testrunner.cc
+++ b/testrunner.cc
@@ -349,6 +349,62 @@ TEST_CASE("readonly test") {
   unlink(fname.c_str());
 }
 
+TEST_CASE("MiniSQLite destructor commits") {
+  const char* name = "testrunner-dtor.sqlite3";
+
+  unlink(name);
+
+  {
+    MiniSQLite ms(name);
+    ms.begin();
+    ms.exec("create table top (role, playing, game)");
+    ms.exec("insert into top values ('d', '&', 'd')");
+  }
+
+  {
+    MiniSQLite ms(name);
+    CHECK(ms.exec("select count(*) from top").at(0).at(0) == "1");
+  }
+
+  unlink(name);
+}
+
+TEST_CASE("MiniSQLite destructor commit failure is ignored") {
+  const char* name = "testrunner-dtor.sqlite3";
+
+  unlink(name);
+
+  {
+    MiniSQLite ms(name);
+    ms.begin();
+    unlink(name);
+  }
+
+  unlink(name);
+}
+
+TEST_CASE("MiniSQLite destructor is not confused by rollback query") {
+  CHECK_NOTHROW([](){
+    MiniSQLite ms(":memory:");
+
+    ms.begin();
+    ms.exec("rollback");
+  }());
+}
+
+TEST_CASE("MiniSQLite destructor is not confused by rollback caused by error") {
+  CHECK_THROWS([](){
+    MiniSQLite ms(":memory:");
+
+    ms.begin();
+    ms.exec("create table foo (bar)");
+    ms.exec("pragma max_page_count=5");
+
+    // error happens here: 'database or disk is full'
+    ms.exec("insert into foo values (zeroblob(1000000))");
+  }());
+}
+
 TEST_CASE("test scale") {
   unlink("testrunner-example.sqlite3");
   {