[Python-checkins] bpo-44991: Make GIL handling more explicit in `sqlite3` callbacks (GH-27934)

[miss-islington]

https://github.com/python/cpython/commit/001ef4600f5ab2e1d7825ddbc0f253377c234d7e
commit: 001ef4600f5ab2e1d7825ddbc0f253377c234d7e
branch: main
author: Erlend Egeberg Aasland <erlend.aasland at innova.no>
committer: miss-islington <31488909+miss-islington at users.noreply.github.com>
date: 2021-08-31T05:18:43-07:00
summary:

bpo-44991: Make GIL handling more explicit in `sqlite3` callbacks (GH-27934)



- acquire the GIL at the very start[1]
- release the GIL at the very end

[1] The trace callback performs a sanity check before acquiring the GIL

Automerge-Triggered-By: GH:encukou

files:
M Modules/_sqlite/connection.c

diff --git a/Modules/_sqlite/connection.c b/Modules/_sqlite/connection.c
index 19d30d24d7f2e7..9ff5fbaae78529 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. */
@@ -787,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
 
@@ -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,14 +1451,16 @@ 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();
 
+    /* This callback may be executed multiple times per sqlite3_step(). Bail if
+     * the previous call failed */
     if (PyErr_Occurred()) {
         goto finally;
     }