From bebc47ac07ac6e5798e2a648b2f0bcc1ae676710 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Thu, 29 Jul 2021 13:39:56 +0200 Subject: [PATCH 1/4] Add stubs for managing callback context --- Modules/_sqlite/connection.c | 20 ++++++++++++++++++++ Modules/_sqlite/connection.h | 6 ++++++ 2 files changed, 26 insertions(+) diff --git a/Modules/_sqlite/connection.c b/Modules/_sqlite/connection.c index a95b75a0fe14a6..87e1926f28b0a6 100644 --- a/Modules/_sqlite/connection.c +++ b/Modules/_sqlite/connection.c @@ -827,6 +827,26 @@ static void _pysqlite_drop_unused_cursor_references(pysqlite_Connection* self) Py_SETREF(self->cursors, new_list); } +static callback_context * +create_callback_context(pysqlite_state *state, PyObject *obj) +{ + callback_context *ctx = PyMem_Malloc(sizeof(callback_context)); + if (ctx != NULL) { + ctx->obj = Py_NewRef(obj); + ctx->state = state; + } + return ctx; +} + +static void +free_callback_context(callback_context *ctx) +{ + if (ctx != NULL) { + Py_DECREF(ctx->obj); + PyMem_Free(ctx); + } +} + static void _destructor(void* args) { // This function may be called without the GIL held, so we need to ensure diff --git a/Modules/_sqlite/connection.h b/Modules/_sqlite/connection.h index edf565020420d3..57055a30ce2701 100644 --- a/Modules/_sqlite/connection.h +++ b/Modules/_sqlite/connection.h @@ -32,6 +32,12 @@ #include "sqlite3.h" +typedef struct _callback_context +{ + PyObject *obj; + pysqlite_state *state; +} callback_context; + typedef struct { PyObject_HEAD From 48a954d88e59bf4639f139ed9cfb4c741c4a0685 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Thu, 29 Jul 2021 13:54:16 +0200 Subject: [PATCH 2/4] Adapt create_*() functions --- Modules/_sqlite/connection.c | 63 ++++++++++++++++++++++-------------- 1 file changed, 38 insertions(+), 25 deletions(-) diff --git a/Modules/_sqlite/connection.c b/Modules/_sqlite/connection.c index 87e1926f28b0a6..239faf13c56308 100644 --- a/Modules/_sqlite/connection.c +++ b/Modules/_sqlite/connection.c @@ -617,7 +617,9 @@ _pysqlite_func_callback(sqlite3_context *context, int argc, sqlite3_value **argv threadstate = PyGILState_Ensure(); - py_func = (PyObject*)sqlite3_user_data(context); + callback_context *ctx = (callback_context *)sqlite3_user_data(context); + assert(ctx != NULL); + py_func = ctx->obj; args = _pysqlite_build_py_params(context, argc, argv); if (args) { @@ -631,8 +633,8 @@ _pysqlite_func_callback(sqlite3_context *context, int argc, sqlite3_value **argv Py_DECREF(py_retval); } if (!ok) { - pysqlite_state *state = pysqlite_get_state(NULL); - if (state->enable_callback_tracebacks) { + assert(ctx->state != NULL); + if (ctx->state->enable_callback_tracebacks) { PyErr_Print(); } else { @@ -656,7 +658,9 @@ static void _pysqlite_step_callback(sqlite3_context *context, int argc, sqlite3_ threadstate = PyGILState_Ensure(); - aggregate_class = (PyObject*)sqlite3_user_data(context); + callback_context *ctx = (callback_context *)sqlite3_user_data(context); + assert(ctx != NULL); + aggregate_class = ctx->obj; aggregate_instance = (PyObject**)sqlite3_aggregate_context(context, sizeof(PyObject*)); @@ -666,8 +670,8 @@ static void _pysqlite_step_callback(sqlite3_context *context, int argc, sqlite3_ if (PyErr_Occurred()) { *aggregate_instance = 0; - pysqlite_state *state = pysqlite_get_state(NULL); - if (state->enable_callback_tracebacks) { + assert(ctx->state != NULL); + if (ctx->state->enable_callback_tracebacks) { PyErr_Print(); } else { @@ -692,8 +696,8 @@ static void _pysqlite_step_callback(sqlite3_context *context, int argc, sqlite3_ Py_DECREF(args); if (!function_result) { - pysqlite_state *state = pysqlite_get_state(NULL); - if (state->enable_callback_tracebacks) { + assert(ctx->state != NULL); + if (ctx->state->enable_callback_tracebacks) { PyErr_Print(); } else { @@ -747,8 +751,10 @@ _pysqlite_final_callback(sqlite3_context *context) Py_DECREF(function_result); } if (!ok) { - pysqlite_state *state = pysqlite_get_state(NULL); - if (state->enable_callback_tracebacks) { + callback_context *ctx = (callback_context *)sqlite3_user_data(context); + assert(ctx != NULL); + assert(ctx->state != NULL); + if (ctx->state->enable_callback_tracebacks) { PyErr_Print(); } else { @@ -853,7 +859,7 @@ static void _destructor(void* args) // that we destroy 'args' with the GIL PyGILState_STATE gstate; gstate = PyGILState_Ensure(); - Py_DECREF((PyObject*)args); + free_callback_context((callback_context *)args); PyGILState_Release(gstate); } @@ -896,11 +902,11 @@ pysqlite_connection_create_function_impl(pysqlite_Connection *self, flags |= SQLITE_DETERMINISTIC; #endif } - rc = sqlite3_create_function_v2(self->db, - name, - narg, - flags, - (void*)Py_NewRef(func), + callback_context *ctx = create_callback_context(self->state, func); + if (ctx == NULL) { + return NULL; + } + rc = sqlite3_create_function_v2(self->db, name, narg, flags, ctx, _pysqlite_func_callback, NULL, NULL, @@ -936,11 +942,12 @@ pysqlite_connection_create_aggregate_impl(pysqlite_Connection *self, return NULL; } - rc = sqlite3_create_function_v2(self->db, - name, - n_arg, - SQLITE_UTF8, - (void*)Py_NewRef(aggregate_class), + callback_context *ctx = create_callback_context(self->state, + aggregate_class); + if (ctx == NULL) { + return NULL; + } + rc = sqlite3_create_function_v2(self->db, name, n_arg, SQLITE_UTF8, ctx, 0, &_pysqlite_step_callback, &_pysqlite_final_callback, @@ -1507,7 +1514,6 @@ pysqlite_collation_callback( int text1_length, const void* text1_data, int text2_length, const void* text2_data) { - PyObject* callback = (PyObject*)context; PyObject* string1 = 0; PyObject* string2 = 0; PyGILState_STATE gilstate; @@ -1527,6 +1533,9 @@ pysqlite_collation_callback( goto finally; /* failed to allocate strings */ } + callback_context *ctx = (callback_context *)context; + assert(ctx != NULL); + PyObject *callback = ctx->obj; PyObject *args[] = { string1, string2 }; // Borrowed refs. retval = PyObject_Vectorcall(callback, args, 2, NULL); if (retval == NULL) { @@ -1758,6 +1767,7 @@ pysqlite_connection_create_collation_impl(pysqlite_Connection *self, return NULL; } + callback_context *ctx = NULL; int rc; int flags = SQLITE_UTF8; if (callable == Py_None) { @@ -1769,8 +1779,11 @@ pysqlite_connection_create_collation_impl(pysqlite_Connection *self, PyErr_SetString(PyExc_TypeError, "parameter must be callable"); return NULL; } - rc = sqlite3_create_collation_v2(self->db, name, flags, - Py_NewRef(callable), + ctx = create_callback_context(self->state, callable); + if (ctx == NULL) { + return NULL; + } + rc = sqlite3_create_collation_v2(self->db, name, flags, ctx, &pysqlite_collation_callback, &_destructor); } @@ -1781,7 +1794,7 @@ pysqlite_connection_create_collation_impl(pysqlite_Connection *self, * the context before returning. */ if (callable != Py_None) { - Py_DECREF(callable); + free_callback_context(ctx); } _pysqlite_seterror(self->state, self->db); return NULL; From c5da5abb74e39cfd29438f3304e7441625cd209f Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Fri, 30 Jul 2021 11:51:49 +0200 Subject: [PATCH 3/4] Hold GIL when allocating/freeing context --- Modules/_sqlite/connection.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/Modules/_sqlite/connection.c b/Modules/_sqlite/connection.c index 239faf13c56308..8d5548e042afdf 100644 --- a/Modules/_sqlite/connection.c +++ b/Modules/_sqlite/connection.c @@ -836,7 +836,9 @@ static void _pysqlite_drop_unused_cursor_references(pysqlite_Connection* self) static callback_context * create_callback_context(pysqlite_state *state, PyObject *obj) { + PyGILState_STATE gstate = PyGILState_Ensure(); callback_context *ctx = PyMem_Malloc(sizeof(callback_context)); + PyGILState_Release(gstate); if (ctx != NULL) { ctx->obj = Py_NewRef(obj); ctx->state = state; @@ -848,19 +850,18 @@ static void free_callback_context(callback_context *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->obj); PyMem_Free(ctx); + PyGILState_Release(gstate); } } static void _destructor(void* args) { - // This function may be called without the GIL held, so we need to ensure - // that we destroy 'args' with the GIL - PyGILState_STATE gstate; - gstate = PyGILState_Ensure(); free_callback_context((callback_context *)args); - PyGILState_Release(gstate); } /*[clinic input] From 271b9b527dbf5a00b47fed08c9cbf53f438dc35f Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Tue, 24 Aug 2021 13:56:41 +0200 Subject: [PATCH 4/4] Address review: protect incref with GIL --- 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 e5fd4f2afc65b4..8ad5f5f061da5d 100644 --- a/Modules/_sqlite/connection.c +++ b/Modules/_sqlite/connection.c @@ -789,11 +789,11 @@ create_callback_context(pysqlite_state *state, PyObject *callable) { PyGILState_STATE gstate = PyGILState_Ensure(); callback_context *ctx = PyMem_Malloc(sizeof(callback_context)); - PyGILState_Release(gstate); if (ctx != NULL) { ctx->callable = Py_NewRef(callable); ctx->state = state; } + PyGILState_Release(gstate); return ctx; }