gh-108083: Don't ignore exceptions in sqlite3.Connection.__init__() and .close() (#108084)

erlend-aasland · vstinner · serhiy-storchaka · web-flow · commit fd195092204a · 2023-08-18T11:39:12.000Z
- Add explanatory comments
- Add return value to connection_close() for propagating errors
- Always check the return value of connection_exec_stmt()
- Assert pre/post state in remove_callbacks()
- Don't log unraisable exceptions in case of interpreter shutdown
- Make sure we're not initialized if reinit fails
- Try to close the database even if ROLLBACK fails

Co-authored-by: Victor Stinner &lt;vstinner@python.org&gt;
Co-authored-by: Serhiy Storchaka &lt;storchaka@gmail.com&gt;
diff --git a/Misc/NEWS.d/next/Library/2023-08-17-12-59-35.gh-issue-108083.9J7UcT.rst b/Misc/NEWS.d/next/Library/2023-08-17-12-59-35.gh-issue-108083.9J7UcT.rst
@@ -0,0 +1,3 @@
+Fix bugs in the constructor of :mod:`sqlite3.Connection` and
+:meth:`sqlite3.Connection.close` where exceptions could be leaked. Patch by
+Erlend E. Aasland.
diff --git a/Modules/_sqlite/connection.c b/Modules/_sqlite/connection.c
@@ -149,7 +149,7 @@ static void _pysqlite_drop_unused_cursor_references(pysqlite_Connection* self);
 static void free_callback_context(callback_context *ctx);
 static void set_callback_context(callback_context **ctx_pp,
                                  callback_context *ctx);
-static void connection_close(pysqlite_Connection *self);
+static int connection_close(pysqlite_Connection *self);
 PyObject *_pysqlite_query_execute(pysqlite_Cursor *, int, PyObject *, PyObject *);
 
 static PyObject *
@@ -247,10 +247,13 @@ pysqlite_connection_init_impl(pysqlite_Connection *self, PyObject *database,
     }
 
     if (self->initialized) {
+        self->initialized = 0;
+
         PyTypeObject *tp = Py_TYPE(self);
         tp->tp_clear((PyObject *)self);
-        connection_close(self);
-        self->initialized = 0;
+        if (connection_close(self) < 0) {
+            return -1;
+        }
     }
 
     // Create and configure SQLite database object.
@@ -334,7 +337,9 @@ pysqlite_connection_init_impl(pysqlite_Connection *self, PyObject *database,
     self->initialized = 1;
 
     if (autocommit == AUTOCOMMIT_DISABLED) {
-        (void)connection_exec_stmt(self, "BEGIN");
+        if (connection_exec_stmt(self, "BEGIN") < 0) {
+            return -1;
+        }
     }
     return 0;
 
@@ -432,48 +437,83 @@ free_callback_contexts(pysqlite_Connection *self)
 static void
 remove_callbacks(sqlite3 *db)
 {
-    sqlite3_trace_v2(db, SQLITE_TRACE_STMT, 0, 0);
+    /* None of these APIs can fail, as long as they are given a valid
+     * database pointer. */
+    assert(db != NULL);
+    int rc = sqlite3_trace_v2(db, SQLITE_TRACE_STMT, 0, 0);
+    assert(rc == SQLITE_OK), (void)rc;
+
     sqlite3_progress_handler(db, 0, 0, (void *)0);
-    (void)sqlite3_set_authorizer(db, NULL, NULL);
+
+    rc = sqlite3_set_authorizer(db, NULL, NULL);
+    assert(rc == SQLITE_OK), (void)rc;
 }
 
-static void
+static int
 connection_close(pysqlite_Connection *self)
 {
-    if (self->db) {
-        if (self->autocommit == AUTOCOMMIT_DISABLED &&
-            !sqlite3_get_autocommit(self->db))
-        {
-            /* If close is implicitly called as a result of interpreter
-             * tear-down, we must not call back into Python. */
-            if (_Py_IsInterpreterFinalizing(PyInterpreterState_Get())) {
-                remove_callbacks(self->db);
-            }
-            (void)connection_exec_stmt(self, "ROLLBACK");
+    if (self->db == NULL) {
+        return 0;
+    }
+
+    int rc = 0;
+    if (self->autocommit == AUTOCOMMIT_DISABLED &&
+        !sqlite3_get_autocommit(self->db))
+    {
+        if (connection_exec_stmt(self, "ROLLBACK") < 0) {
+            rc = -1;
         }
+    }
 
-        free_callback_contexts(self);
+    sqlite3 *db = self->db;
+    self->db = NULL;
 
-        sqlite3 *db = self->db;
-        self->db = NULL;
+    Py_BEGIN_ALLOW_THREADS
+    /* The v2 close call always returns SQLITE_OK if given a valid database
+     * pointer (which we do), so we can safely ignore the return value */
+    (void)sqlite3_close_v2(db);
+    Py_END_ALLOW_THREADS
 
-        Py_BEGIN_ALLOW_THREADS
-        int rc = sqlite3_close_v2(db);
-        assert(rc == SQLITE_OK), (void)rc;
-        Py_END_ALLOW_THREADS
-    }
+    free_callback_contexts(self);
+    return rc;
 }
 
 static void
-connection_dealloc(pysqlite_Connection *self)
+connection_finalize(PyObject *self)
 {
-    PyTypeObject *tp = Py_TYPE(self);
-    PyObject_GC_UnTrack(self);
-    tp->tp_clear((PyObject *)self);
+    pysqlite_Connection *con = (pysqlite_Connection *)self;
+    PyObject *exc = PyErr_GetRaisedException();
+
+    /* If close is implicitly called as a result of interpreter
+     * tear-down, we must not call back into Python. */
+    PyInterpreterState *interp = PyInterpreterState_Get();
+    int teardown = _Py_IsInterpreterFinalizing(interp);
+    if (teardown && con->db) {
+        remove_callbacks(con->db);
+    }
 
     /* Clean up if user has not called .close() explicitly. */
-    connection_close(self);
+    if (connection_close(con) < 0) {
+        if (teardown) {
+            PyErr_Clear();
+        }
+        else {
+            PyErr_WriteUnraisable((PyObject *)self);
+        }
+    }
+
+    PyErr_SetRaisedException(exc);
+}
 
+static void
+connection_dealloc(PyObject *self)
+{
+    if (PyObject_CallFinalizerFromDealloc(self) < 0) {
+        return;
+    }
+    PyTypeObject *tp = Py_TYPE(self);
+    PyObject_GC_UnTrack(self);
+    tp->tp_clear(self);
     tp->tp_free(self);
     Py_DECREF(tp);
 }
@@ -621,7 +661,9 @@ pysqlite_connection_close_impl(pysqlite_Connection *self)
 
     pysqlite_close_all_blobs(self);
     Py_CLEAR(self->statement_cache);
-    connection_close(self);
+    if (connection_close(self) < 0) {
+        return NULL;
+    }
 
     Py_RETURN_NONE;
 }
@@ -2555,6 +2597,7 @@ static struct PyMemberDef connection_members[] =
 };
 
 static PyType_Slot connection_slots[] = {
+    {Py_tp_finalize, connection_finalize},
     {Py_tp_dealloc, connection_dealloc},
     {Py_tp_doc, (void *)connection_doc},
     {Py_tp_methods, connection_methods},

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	+Fix bugs in the constructor of :mod:`sqlite3.Connection` and
	`2`	+:meth:`sqlite3.Connection.close` where exceptions could be leaked. Patch by
	`3`	`+Erlend E. Aasland.`