From 36ae3eb25ab9c1c7f8d159a2ac3a000d66dd3ed8 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Mon, 31 May 2021 23:55:24 +0200 Subject: [PATCH 1/5] bpo-42972: Track sqlite3 statement objects --- Modules/_sqlite/connection.c | 1 + Modules/_sqlite/cursor.c | 1 + 2 files changed, 2 insertions(+) diff --git a/Modules/_sqlite/connection.c b/Modules/_sqlite/connection.c index 54e477739bd457..7bebeeb196cfd6 100644 --- a/Modules/_sqlite/connection.c +++ b/Modules/_sqlite/connection.c @@ -1363,6 +1363,7 @@ pysqlite_connection_call(pysqlite_Connection *self, PyObject *args, statement->in_weakreflist = NULL; rc = pysqlite_statement_create(statement, self, sql); + PyObject_GC_Track(statement); if (rc != SQLITE_OK) { if (rc == PYSQLITE_TOO_MUCH_SQL) { PyErr_SetString(pysqlite_Warning, "You can only execute one statement at a time."); diff --git a/Modules/_sqlite/cursor.c b/Modules/_sqlite/cursor.c index 4eb9c6d28b103e..83ee14b919e607 100644 --- a/Modules/_sqlite/cursor.c +++ b/Modules/_sqlite/cursor.c @@ -512,6 +512,7 @@ _pysqlite_query_execute(pysqlite_Cursor* self, int multiple, PyObject* operation goto error; } rc = pysqlite_statement_create(self->statement, self->connection, operation); + PyObject_GC_Track(self->statement); if (rc != SQLITE_OK) { Py_CLEAR(self->statement); goto error; From 1b0f5839b51c4e92c2e83aeddd61c130167f255d Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Tue, 1 Jun 2021 09:25:59 +0200 Subject: [PATCH 2/5] Allocate and track statement objects in pysqlite_statement_create By allocating and tracking creation of statement object in pysqlite_statement_create(), the caller does not need to worry about GC syncronization, and eliminates the possibility of getting a badly created object. All related fault handling is moved to pysqlite_statement_create(). --- Modules/_sqlite/connection.c | 26 ++--------------------- Modules/_sqlite/cursor.c | 10 ++------- Modules/_sqlite/statement.c | 40 +++++++++++++++++++++++++++--------- Modules/_sqlite/statement.h | 2 +- 4 files changed, 35 insertions(+), 43 deletions(-) diff --git a/Modules/_sqlite/connection.c b/Modules/_sqlite/connection.c index 7bebeeb196cfd6..7252ccab10b4bc 100644 --- a/Modules/_sqlite/connection.c +++ b/Modules/_sqlite/connection.c @@ -1337,7 +1337,6 @@ pysqlite_connection_call(pysqlite_Connection *self, PyObject *args, PyObject* sql; pysqlite_Statement* statement; PyObject* weakref; - int rc; if (!pysqlite_check_thread(self) || !pysqlite_check_connection(self)) { return NULL; @@ -1351,32 +1350,11 @@ pysqlite_connection_call(pysqlite_Connection *self, PyObject *args, _pysqlite_drop_unused_statement_references(self); - statement = PyObject_GC_New(pysqlite_Statement, pysqlite_StatementType); - if (!statement) { + statement = pysqlite_statement_create(self, sql); + if (statement == NULL) { return NULL; } - statement->db = NULL; - statement->st = NULL; - statement->sql = NULL; - statement->in_use = 0; - statement->in_weakreflist = NULL; - - rc = pysqlite_statement_create(statement, self, sql); - PyObject_GC_Track(statement); - if (rc != SQLITE_OK) { - if (rc == PYSQLITE_TOO_MUCH_SQL) { - PyErr_SetString(pysqlite_Warning, "You can only execute one statement at a time."); - } else if (rc == PYSQLITE_SQL_WRONG_TYPE) { - if (PyErr_ExceptionMatches(PyExc_TypeError)) - PyErr_SetString(pysqlite_Warning, "SQL is of wrong type. Must be string."); - } else { - (void)pysqlite_statement_reset(statement); - _pysqlite_seterror(self->db); - } - goto error; - } - weakref = PyWeakref_NewRef((PyObject*)statement, NULL); if (weakref == NULL) goto error; diff --git a/Modules/_sqlite/cursor.c b/Modules/_sqlite/cursor.c index 83ee14b919e607..7656c903a4acff 100644 --- a/Modules/_sqlite/cursor.c +++ b/Modules/_sqlite/cursor.c @@ -507,14 +507,8 @@ _pysqlite_query_execute(pysqlite_Cursor* self, int multiple, PyObject* operation if (self->statement->in_use) { Py_SETREF(self->statement, - PyObject_GC_New(pysqlite_Statement, pysqlite_StatementType)); - if (!self->statement) { - goto error; - } - rc = pysqlite_statement_create(self->statement, self->connection, operation); - PyObject_GC_Track(self->statement); - if (rc != SQLITE_OK) { - Py_CLEAR(self->statement); + pysqlite_statement_create(self->connection, operation)); + if (self->statement == NULL) { goto error; } } diff --git a/Modules/_sqlite/statement.c b/Modules/_sqlite/statement.c index 332bf030a9b909..89fb887e005ff1 100644 --- a/Modules/_sqlite/statement.c +++ b/Modules/_sqlite/statement.c @@ -48,7 +48,8 @@ typedef enum { TYPE_UNKNOWN } parameter_type; -int pysqlite_statement_create(pysqlite_Statement* self, pysqlite_Connection* connection, PyObject* sql) +pysqlite_Statement * +pysqlite_statement_create(pysqlite_Connection *connection, PyObject *sql) { const char* tail; int rc; @@ -56,21 +57,28 @@ int pysqlite_statement_create(pysqlite_Statement* self, pysqlite_Connection* con Py_ssize_t sql_cstr_len; const char* p; - self->st = NULL; - self->in_use = 0; - assert(PyUnicode_Check(sql)); sql_cstr = PyUnicode_AsUTF8AndSize(sql, &sql_cstr_len); if (sql_cstr == NULL) { - rc = PYSQLITE_SQL_WRONG_TYPE; - return rc; + PyErr_SetString(pysqlite_Warning, + "SQL is of wrong type. Must be string."); + return NULL; } if (strlen(sql_cstr) != (size_t)sql_cstr_len) { - PyErr_SetString(PyExc_ValueError, "the query contains a null character"); - return PYSQLITE_SQL_WRONG_TYPE; + PyErr_SetString(PyExc_ValueError, + "the query contains a null character"); + return NULL; } + pysqlite_Statement *self = PyObject_GC_New(pysqlite_Statement, + pysqlite_StatementType); + if (self == NULL) { + return NULL; + } + + self->st = NULL; + self->in_use = 0; self->in_weakreflist = NULL; self->sql = Py_NewRef(sql); @@ -102,14 +110,26 @@ int pysqlite_statement_create(pysqlite_Statement* self, pysqlite_Connection* con Py_END_ALLOW_THREADS self->db = connection->db; + PyObject_GC_Track(self); + + if (rc != SQLITE_OK) { + _pysqlite_seterror(self->db); + goto error; + } if (rc == SQLITE_OK && pysqlite_check_remaining_sql(tail)) { (void)sqlite3_finalize(self->st); self->st = NULL; - rc = PYSQLITE_TOO_MUCH_SQL; + PyErr_SetString(pysqlite_Warning, + "You can only execute one statement at a time."); + goto error; } - return rc; + return self; + +error: + Py_DECREF(self); + return NULL; } int pysqlite_statement_bind_parameter(pysqlite_Statement* self, int pos, PyObject* parameter) diff --git a/Modules/_sqlite/statement.h b/Modules/_sqlite/statement.h index 56ff7271448d1c..b876dc75fd9d0c 100644 --- a/Modules/_sqlite/statement.h +++ b/Modules/_sqlite/statement.h @@ -45,7 +45,7 @@ typedef struct extern PyTypeObject *pysqlite_StatementType; -int pysqlite_statement_create(pysqlite_Statement* self, pysqlite_Connection* connection, PyObject* sql); +pysqlite_Statement *pysqlite_statement_create(pysqlite_Connection *connection, PyObject *sql); int pysqlite_statement_bind_parameter(pysqlite_Statement* self, int pos, PyObject* parameter); void pysqlite_statement_bind_parameters(pysqlite_Statement* self, PyObject* parameters); From 0a6ca751469d3a89d39a9de13d61665692b15066 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Tue, 1 Jun 2021 09:31:46 +0200 Subject: [PATCH 3/5] Remove now unused constants --- Modules/_sqlite/statement.h | 3 --- 1 file changed, 3 deletions(-) diff --git a/Modules/_sqlite/statement.h b/Modules/_sqlite/statement.h index b876dc75fd9d0c..e8c86a0ec963fd 100644 --- a/Modules/_sqlite/statement.h +++ b/Modules/_sqlite/statement.h @@ -29,9 +29,6 @@ #include "connection.h" #include "sqlite3.h" -#define PYSQLITE_TOO_MUCH_SQL (-100) -#define PYSQLITE_SQL_WRONG_TYPE (-101) - typedef struct { PyObject_HEAD From df284eb0efd77d962e64e977bbf499bbf254ec80 Mon Sep 17 00:00:00 2001 From: Erlend Egeberg Aasland Date: Tue, 1 Jun 2021 12:06:43 +0200 Subject: [PATCH 4/5] Improve error message Co-authored-by: Victor Stinner --- Modules/_sqlite/statement.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Modules/_sqlite/statement.c b/Modules/_sqlite/statement.c index 89fb887e005ff1..fe47e0d5d2a493 100644 --- a/Modules/_sqlite/statement.c +++ b/Modules/_sqlite/statement.c @@ -61,8 +61,9 @@ pysqlite_statement_create(pysqlite_Connection *connection, PyObject *sql) sql_cstr = PyUnicode_AsUTF8AndSize(sql, &sql_cstr_len); if (sql_cstr == NULL) { - PyErr_SetString(pysqlite_Warning, - "SQL is of wrong type. Must be string."); + PyErr_Format(pysqlite_Warning, + "SQL is of wrong type ('%s'). Must be string.", + Py_TYPE(sql)->tp_name); return NULL; } if (strlen(sql_cstr) != (size_t)sql_cstr_len) { From 4f03b79a16f4dfdd3923c090f00a132cd71421dc Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Tue, 1 Jun 2021 12:13:06 +0200 Subject: [PATCH 5/5] Group and order member initialisation --- Modules/_sqlite/statement.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Modules/_sqlite/statement.c b/Modules/_sqlite/statement.c index fe47e0d5d2a493..3a18ad8331f69f 100644 --- a/Modules/_sqlite/statement.c +++ b/Modules/_sqlite/statement.c @@ -78,14 +78,15 @@ pysqlite_statement_create(pysqlite_Connection *connection, PyObject *sql) return NULL; } + self->db = connection->db; self->st = NULL; + self->sql = Py_NewRef(sql); self->in_use = 0; + self->is_dml = 0; self->in_weakreflist = NULL; - self->sql = Py_NewRef(sql); /* Determine if the statement is a DML statement. SELECT is the only exception. See #9924. */ - self->is_dml = 0; for (p = sql_cstr; *p != 0; p++) { switch (*p) { case ' ': @@ -103,14 +104,13 @@ pysqlite_statement_create(pysqlite_Connection *connection, PyObject *sql) } Py_BEGIN_ALLOW_THREADS - rc = sqlite3_prepare_v2(connection->db, + rc = sqlite3_prepare_v2(self->db, sql_cstr, -1, &self->st, &tail); Py_END_ALLOW_THREADS - self->db = connection->db; PyObject_GC_Track(self); if (rc != SQLITE_OK) {