From 3a881d9b41b1a6c982e42ef13004d3d210cb19b5 Mon Sep 17 00:00:00 2001
From: Sebastian Falbesoner <sebastian.falbesoner@gmail.com>
Date: Wed, 6 May 2020 01:35:55 +0200
Subject: [PATCH] db: unregister sqlite3 trace callback also in error case

For sqlite3 versions < 3.14 (i.e. HAVE_SQLITE3_EXPANDED_SQL is not set),
tracing is used to dump statements. The function db_sqlite3_exec()
registers a tracing callback in the beginning and unregisters it at the
end to "avoid it accessing the potentially stale pointer to stmt".
However, the unregistering so far only happened in the success case,
i.e. if the prepare or step calls failed, the callback was still set!

Running the test wallet/test/db-run with sqlite 3.11 leads to a
segmentation fault in the last call to db_commit_transaction():
the tested transaction contains an invalid statement and the (still
registered) trace callback is triggered then by sqlite3_exec() in
db_sqlite3_commit_tx(), leading to a segfault in db_changes_add()
(according to gdb), where it tries to access "stmt->query->readonly".

Changelog-None
---
 wallet/db_sqlite3.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/wallet/db_sqlite3.c b/wallet/db_sqlite3.c
index d6ef92084..e96172430 100644
--- a/wallet/db_sqlite3.c
+++ b/wallet/db_sqlite3.c
@@ -100,6 +100,7 @@ static bool db_sqlite3_query(struct db_stmt *stmt)
 static bool db_sqlite3_exec(struct db_stmt *stmt)
 {
 	int err;
+	bool success;
 #if !HAVE_SQLITE3_EXPANDED_SQL
 	/* Register the tracing function if we don't have an explicit way of
 	 * expanding the statement. */
@@ -108,14 +109,16 @@ static bool db_sqlite3_exec(struct db_stmt *stmt)
 
 	if (!db_sqlite3_query(stmt)) {
 		/* If the prepare step caused an error we hand it up. */
-		return false;
+		success = false;
+		goto done;
 	}
 
 	err = sqlite3_step(stmt->inner_stmt);
 	if (err != SQLITE_DONE) {
 		tal_free(stmt->error);
 		stmt->error = db_sqlite3_fmt_error(stmt);
-		return false;
+		success = false;
+		goto done;
 	}
 
 #if HAVE_SQLITE3_EXPANDED_SQL
@@ -124,13 +127,17 @@ static bool db_sqlite3_exec(struct db_stmt *stmt)
 	expanded_sql = sqlite3_expanded_sql(stmt->inner_stmt);
 	db_changes_add(stmt, expanded_sql);
 	sqlite3_free(expanded_sql);
-#else
+#endif
+	success = true;
+
+done:
+#if !HAVE_SQLITE3_EXPANDED_SQL
 	/* Unregister the trace callback to avoid it accessing the potentially
 	 * stale pointer to stmt */
 	sqlite3_trace(stmt->db->conn, NULL, NULL);
 #endif
 
-	return true;
+	return success;
 }
 
 static bool db_sqlite3_step(struct db_stmt *stmt)