From 09567958c2cf031c3cd1614e04e1467c809d8cb0 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Thu, 17 Aug 2023 13:00:39 +0200 Subject: [PATCH 01/10] gh-108083: Don't ignore exceptions in sqlite3.Connection.__init__() and .close() Always check the return value of connection_exec_stmt() --- .../2023-08-17-12-59-35.gh-issue-108083.9J7UcT.rst | 3 +++ Modules/_sqlite/connection.c | 8 ++++++-- 2 files changed, 9 insertions(+), 2 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2023-08-17-12-59-35.gh-issue-108083.9J7UcT.rst 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 new file mode 100644 index 00000000000000..ff499ced73a309 --- /dev/null +++ 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 index 0819acd0940964..a694ac7377079b 100644 --- a/Modules/_sqlite/connection.c +++ b/Modules/_sqlite/connection.c @@ -334,7 +334,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; @@ -449,7 +451,9 @@ connection_close(pysqlite_Connection *self) if (_Py_IsInterpreterFinalizing(PyInterpreterState_Get())) { remove_callbacks(self->db); } - (void)connection_exec_stmt(self, "ROLLBACK"); + if (connection_exec_stmt(self, "ROLLBACK") < 0) { + PyErr_WriteUnraisable(self); + } } free_callback_contexts(self); From a5f6454684a7f799252ddc52d773fddd72585f54 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Thu, 17 Aug 2023 13:35:06 +0200 Subject: [PATCH 02/10] Cast --- Modules/_sqlite/connection.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Modules/_sqlite/connection.c b/Modules/_sqlite/connection.c index a694ac7377079b..83562f51f56386 100644 --- a/Modules/_sqlite/connection.c +++ b/Modules/_sqlite/connection.c @@ -452,7 +452,7 @@ connection_close(pysqlite_Connection *self) remove_callbacks(self->db); } if (connection_exec_stmt(self, "ROLLBACK") < 0) { - PyErr_WriteUnraisable(self); + PyErr_WriteUnraisable((PyObject *)self); } } From 7b3d032abc2fd619382b0e0e6714eafb93777dbe Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Thu, 17 Aug 2023 21:24:03 +0200 Subject: [PATCH 03/10] Address review: let connection_close() return a value and the caller decide what to do --- Modules/_sqlite/connection.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/Modules/_sqlite/connection.c b/Modules/_sqlite/connection.c index 83562f51f56386..9847894d663c19 100644 --- 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 * @@ -249,7 +249,9 @@ pysqlite_connection_init_impl(pysqlite_Connection *self, PyObject *database, if (self->initialized) { PyTypeObject *tp = Py_TYPE(self); tp->tp_clear((PyObject *)self); - connection_close(self); + if (connection_close(self) < 0) { + return -1; + } self->initialized = 0; } @@ -439,7 +441,7 @@ remove_callbacks(sqlite3 *db) (void)sqlite3_set_authorizer(db, NULL, NULL); } -static void +static int connection_close(pysqlite_Connection *self) { if (self->db) { @@ -452,7 +454,7 @@ connection_close(pysqlite_Connection *self) remove_callbacks(self->db); } if (connection_exec_stmt(self, "ROLLBACK") < 0) { - PyErr_WriteUnraisable((PyObject *)self); + return -1; } } @@ -466,17 +468,21 @@ connection_close(pysqlite_Connection *self) assert(rc == SQLITE_OK), (void)rc; Py_END_ALLOW_THREADS } + return 0; } static void -connection_dealloc(pysqlite_Connection *self) +connection_dealloc(PyObject *self) { + pysqlite_Connection *con = (pysqlite_Connection *)self; PyTypeObject *tp = Py_TYPE(self); PyObject_GC_UnTrack(self); - tp->tp_clear((PyObject *)self); + tp->tp_clear(self); /* Clean up if user has not called .close() explicitly. */ - connection_close(self); + if (connection_close(con) < 0) { + PyErr_WriteUnraisable((PyObject *)self); + } tp->tp_free(self); Py_DECREF(tp); @@ -625,7 +631,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; } From 379e64e8c162cc22d41d1c0fb143e2fd6b926ed7 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Thu, 17 Aug 2023 21:45:55 +0200 Subject: [PATCH 04/10] Don't log unraisable exceptions in case of interp shutdown --- Modules/_sqlite/connection.c | 46 ++++++++++++++++++++++++++---------- 1 file changed, 34 insertions(+), 12 deletions(-) diff --git a/Modules/_sqlite/connection.c b/Modules/_sqlite/connection.c index 9847894d663c19..ada139159677ef 100644 --- a/Modules/_sqlite/connection.c +++ b/Modules/_sqlite/connection.c @@ -436,6 +436,9 @@ free_callback_contexts(pysqlite_Connection *self) static void remove_callbacks(sqlite3 *db) { + if (db == NULL) { + return; + } sqlite3_trace_v2(db, SQLITE_TRACE_STMT, 0, 0); sqlite3_progress_handler(db, 0, 0, (void *)0); (void)sqlite3_set_authorizer(db, NULL, NULL); @@ -448,18 +451,11 @@ connection_close(pysqlite_Connection *self) 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); - } if (connection_exec_stmt(self, "ROLLBACK") < 0) { return -1; } } - free_callback_contexts(self); - sqlite3 *db = self->db; self->db = NULL; @@ -467,23 +463,48 @@ connection_close(pysqlite_Connection *self) int rc = sqlite3_close_v2(db); assert(rc == SQLITE_OK), (void)rc; Py_END_ALLOW_THREADS + + free_callback_contexts(self); } return 0; } static void -connection_dealloc(PyObject *self) +connection_finalize(PyObject *self) { pysqlite_Connection *con = (pysqlite_Connection *)self; - PyTypeObject *tp = Py_TYPE(self); - PyObject_GC_UnTrack(self); - tp->tp_clear(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) { + remove_callbacks(con->db); + } /* Clean up if user has not called .close() explicitly. */ if (connection_close(con) < 0) { - PyErr_WriteUnraisable((PyObject *)self); + 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); } @@ -2567,6 +2588,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}, From b2d49f3605e6093b27f11b51475b361bdd12504c Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Fri, 18 Aug 2023 09:38:33 +0200 Subject: [PATCH 05/10] Assert pre/post state in remove_callbacks() --- Modules/_sqlite/connection.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/Modules/_sqlite/connection.c b/Modules/_sqlite/connection.c index ada139159677ef..54f8f62f069217 100644 --- a/Modules/_sqlite/connection.c +++ b/Modules/_sqlite/connection.c @@ -436,12 +436,14 @@ free_callback_contexts(pysqlite_Connection *self) static void remove_callbacks(sqlite3 *db) { - if (db == NULL) { - return; - } - sqlite3_trace_v2(db, SQLITE_TRACE_STMT, 0, 0); + 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); } static int @@ -479,7 +481,7 @@ connection_finalize(PyObject *self) * tear-down, we must not call back into Python. */ PyInterpreterState *interp = PyInterpreterState_Get(); int teardown = _Py_IsInterpreterFinalizing(interp); - if (teardown) { + if (teardown && con->db) { remove_callbacks(con->db); } From 0d4eb8bd6be18f45ed7e3d750a24dc578227460d Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Fri, 18 Aug 2023 09:40:49 +0200 Subject: [PATCH 06/10] Make sure we're not initialized if reinit fails --- Modules/_sqlite/connection.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Modules/_sqlite/connection.c b/Modules/_sqlite/connection.c index 54f8f62f069217..d614ee996b5d7d 100644 --- a/Modules/_sqlite/connection.c +++ b/Modules/_sqlite/connection.c @@ -247,12 +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); if (connection_close(self) < 0) { return -1; } - self->initialized = 0; } // Create and configure SQLite database object. From 863f5b7e3dc1e014afbf1f449ea04e6fdfc8be5b Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Fri, 18 Aug 2023 09:48:05 +0200 Subject: [PATCH 07/10] Try to close the database even if ROLLBACK fails --- Modules/_sqlite/connection.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/Modules/_sqlite/connection.c b/Modules/_sqlite/connection.c index d614ee996b5d7d..de8c647f2d9e71 100644 --- a/Modules/_sqlite/connection.c +++ b/Modules/_sqlite/connection.c @@ -450,12 +450,13 @@ remove_callbacks(sqlite3 *db) static int connection_close(pysqlite_Connection *self) { + int rc = 0; if (self->db) { if (self->autocommit == AUTOCOMMIT_DISABLED && !sqlite3_get_autocommit(self->db)) { if (connection_exec_stmt(self, "ROLLBACK") < 0) { - return -1; + rc = -1; } } @@ -463,13 +464,14 @@ connection_close(pysqlite_Connection *self) self->db = NULL; Py_BEGIN_ALLOW_THREADS - int rc = sqlite3_close_v2(db); - assert(rc == SQLITE_OK), (void)rc; + /* 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 free_callback_contexts(self); } - return 0; + return rc; } static void From ef5418adac1f40d7a540b909115f0517b402d1dd Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Fri, 18 Aug 2023 09:54:32 +0200 Subject: [PATCH 08/10] Add comment --- Modules/_sqlite/connection.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Modules/_sqlite/connection.c b/Modules/_sqlite/connection.c index de8c647f2d9e71..25cd7fc97c216b 100644 --- a/Modules/_sqlite/connection.c +++ b/Modules/_sqlite/connection.c @@ -437,6 +437,8 @@ free_callback_contexts(pysqlite_Connection *self) static void remove_callbacks(sqlite3 *db) { + /* 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; From 5d308aa71d26e133c6404d97d01666bd1a7d239b Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Fri, 18 Aug 2023 12:20:28 +0200 Subject: [PATCH 09/10] Reduce indent --- Modules/_sqlite/connection.c | 34 ++++++++++++++++++---------------- 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/Modules/_sqlite/connection.c b/Modules/_sqlite/connection.c index 25cd7fc97c216b..5e2ae85b0fa938 100644 --- a/Modules/_sqlite/connection.c +++ b/Modules/_sqlite/connection.c @@ -452,27 +452,29 @@ remove_callbacks(sqlite3 *db) static int connection_close(pysqlite_Connection *self) { + if (self->db == NULL) { + return 0; + } + int rc = 0; - if (self->db) { - if (self->autocommit == AUTOCOMMIT_DISABLED && - !sqlite3_get_autocommit(self->db)) - { - if (connection_exec_stmt(self, "ROLLBACK") < 0) { - rc = -1; - } + if (self->autocommit == AUTOCOMMIT_DISABLED && + !sqlite3_get_autocommit(self->db)) + { + if (connection_exec_stmt(self, "ROLLBACK") < 0) { + rc = -1; } + } - 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 + /* 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 - free_callback_contexts(self); - } + free_callback_contexts(self); return rc; } From 8d571382130389a97803dc7d710ad51b9d223021 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Fri, 18 Aug 2023 12:55:44 +0200 Subject: [PATCH 10/10] Also assert usage after set authorizer Co-authored-by: Serhiy Storchaka --- Modules/_sqlite/connection.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Modules/_sqlite/connection.c b/Modules/_sqlite/connection.c index 5e2ae85b0fa938..282855f8865940 100644 --- a/Modules/_sqlite/connection.c +++ b/Modules/_sqlite/connection.c @@ -446,7 +446,7 @@ remove_callbacks(sqlite3 *db) sqlite3_progress_handler(db, 0, 0, (void *)0); rc = sqlite3_set_authorizer(db, NULL, NULL); - assert(rc == SQLITE_OK); + assert(rc == SQLITE_OK), (void)rc; } static int