From e4f4679448531d1b8090f9b3d8fcb4d95f8ba6d6 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Tue, 24 Aug 2021 19:16:24 +0200 Subject: [PATCH 1/3] Acquire GIL at the very start of callbacks; release at the very end Exception: in the trace callback, do a sanity check before attempting to hold the GIL. --- Modules/_sqlite/connection.c | 42 ++++++++++++++---------------------- 1 file changed, 16 insertions(+), 26 deletions(-) diff --git a/Modules/_sqlite/connection.c b/Modules/_sqlite/connection.c index 8ad5f5f061da5d..e5c072a672c64b 100644 --- a/Modules/_sqlite/connection.c +++ b/Modules/_sqlite/connection.c @@ -626,14 +626,12 @@ set_sqlite_error(sqlite3_context *context, const char *msg) static void _pysqlite_func_callback(sqlite3_context *context, int argc, sqlite3_value **argv) { + PyGILState_STATE threadstate = PyGILState_Ensure(); + PyObject* args; PyObject* py_retval = NULL; int ok; - PyGILState_STATE threadstate; - - threadstate = PyGILState_Ensure(); - args = _pysqlite_build_py_params(context, argc, argv); if (args) { callback_context *ctx = (callback_context *)sqlite3_user_data(context); @@ -656,15 +654,13 @@ _pysqlite_func_callback(sqlite3_context *context, int argc, sqlite3_value **argv static void _pysqlite_step_callback(sqlite3_context *context, int argc, sqlite3_value** params) { + PyGILState_STATE threadstate = PyGILState_Ensure(); + PyObject* args; PyObject* function_result = NULL; PyObject** aggregate_instance; PyObject* stepmethod = NULL; - PyGILState_STATE threadstate; - - threadstate = PyGILState_Ensure(); - aggregate_instance = (PyObject**)sqlite3_aggregate_context(context, sizeof(PyObject*)); if (*aggregate_instance == NULL) { @@ -706,16 +702,14 @@ static void _pysqlite_step_callback(sqlite3_context *context, int argc, sqlite3_ static void _pysqlite_final_callback(sqlite3_context *context) { + PyGILState_STATE threadstate = PyGILState_Ensure(); + PyObject* function_result; PyObject** aggregate_instance; _Py_IDENTIFIER(finalize); int ok; PyObject *exception, *value, *tb; - PyGILState_STATE threadstate; - - threadstate = PyGILState_Ensure(); - aggregate_instance = (PyObject**)sqlite3_aggregate_context(context, 0); if (aggregate_instance == NULL) { /* No rows matched the query; the step handler was never called. */ @@ -914,11 +908,10 @@ pysqlite_connection_create_aggregate_impl(pysqlite_Connection *self, static int _authorizer_callback(void* user_arg, int action, const char* arg1, const char* arg2 , const char* dbname, const char* access_attempt_source) { + PyGILState_STATE gilstate = PyGILState_Ensure(); + PyObject *ret; int rc; - PyGILState_STATE gilstate; - - gilstate = PyGILState_Ensure(); ret = PyObject_CallFunction((PyObject*)user_arg, "issss", action, arg1, arg2, dbname, access_attempt_source); @@ -959,11 +952,10 @@ static int _authorizer_callback(void* user_arg, int action, const char* arg1, co static int _progress_handler(void* user_arg) { + PyGILState_STATE gilstate = PyGILState_Ensure(); + int rc; PyObject *ret; - PyGILState_STATE gilstate; - - gilstate = PyGILState_Ensure(); ret = _PyObject_CallNoArg((PyObject*)user_arg); if (!ret) { @@ -1000,18 +992,16 @@ static int _trace_callback(unsigned int type, void* user_arg, void* prepared_sta static void _trace_callback(void* user_arg, const char* statement_string) #endif { - PyObject *py_statement = NULL; - PyObject *ret = NULL; - - PyGILState_STATE gilstate; - #ifdef HAVE_TRACE_V2 if (type != SQLITE_TRACE_STMT) { return 0; } #endif - gilstate = PyGILState_Ensure(); + PyGILState_STATE gilstate = PyGILState_Ensure(); + + PyObject *py_statement = NULL; + PyObject *ret = NULL; py_statement = PyUnicode_DecodeUTF8(statement_string, strlen(statement_string), "replace"); if (py_statement) { @@ -1461,13 +1451,13 @@ pysqlite_collation_callback( int text1_length, const void* text1_data, int text2_length, const void* text2_data) { + PyGILState_STATE gilstate = PyGILState_Ensure(); + PyObject* string1 = 0; PyObject* string2 = 0; - PyGILState_STATE gilstate; PyObject* retval = NULL; long longval; int result = 0; - gilstate = PyGILState_Ensure(); if (PyErr_Occurred()) { goto finally; From 2a2152dd40bbf78c4d160a98720b264c1fc60cdf Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Tue, 31 Aug 2021 13:45:14 +0200 Subject: [PATCH 2/3] Add explanatory comment about mysterious PyErr_Occurred() --- Modules/_sqlite/connection.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Modules/_sqlite/connection.c b/Modules/_sqlite/connection.c index e5c072a672c64b..33dbf7c50e6b99 100644 --- a/Modules/_sqlite/connection.c +++ b/Modules/_sqlite/connection.c @@ -1459,6 +1459,8 @@ pysqlite_collation_callback( long longval; int result = 0; + /* This callback may be executed multiple times per sqlite3_step(). Bail if + * the previous call failed */ if (PyErr_Occurred()) { goto finally; } From d42eb38fa8935145c3562570acf3481a64b1b616 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Tue, 31 Aug 2021 13:52:13 +0200 Subject: [PATCH 3/3] Address review - since _destructor is a callback, wrap it in GIL lock/unlock - remove GIL lock/unlock from create_callback_context() --- Modules/_sqlite/connection.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/Modules/_sqlite/connection.c b/Modules/_sqlite/connection.c index 33dbf7c50e6b99..c47b4179a6b949 100644 --- a/Modules/_sqlite/connection.c +++ b/Modules/_sqlite/connection.c @@ -781,34 +781,34 @@ static void _pysqlite_drop_unused_cursor_references(pysqlite_Connection* self) static callback_context * create_callback_context(pysqlite_state *state, PyObject *callable) { - PyGILState_STATE gstate = PyGILState_Ensure(); callback_context *ctx = PyMem_Malloc(sizeof(callback_context)); if (ctx != NULL) { ctx->callable = Py_NewRef(callable); ctx->state = state; } - PyGILState_Release(gstate); return ctx; } static void free_callback_context(callback_context *ctx) +{ + assert(ctx != NULL); + Py_DECREF(ctx->callable); + PyMem_Free(ctx); +} + +static void +_destructor(void *ctx) { if (ctx != NULL) { // This function may be called without the GIL held, so we need to // ensure that we destroy 'ctx' with the GIL held. PyGILState_STATE gstate = PyGILState_Ensure(); - Py_DECREF(ctx->callable); - PyMem_Free(ctx); + free_callback_context((callback_context *)ctx); PyGILState_Release(gstate); } } -static void _destructor(void* args) -{ - free_callback_context((callback_context *)args); -} - /*[clinic input] _sqlite3.Connection.create_function as pysqlite_connection_create_function