CONPY-302: fix segfault with threaded

Callback functions for session or fetch callbacks didn't work properly
when passing connection->thread_state in threaded environment.
Instead we acquire and release the GIL explicitly in these callback
functions.

Author: 9EOR9

include/mariadb_python.h

@@ -241,7 +241,6 @@ typedef struct st_parser {
 /* PEP-249: Connection object */
 typedef struct {
     PyObject_HEAD
-    PyThreadState *thread_state;
     MYSQL *mysql;
     int open;
     uint8_t is_buffered;
@@ -808,33 +807,3 @@ MrdbParser_parse(MrdbParser *p, uint8_t is_batch, char *errmsg, size_t errmsg_le
 
 #endif /* __i386__ OR _WIN32 */
 
-/* Due to callback functions we cannot use PY_BEGIN/END_ALLOW_THREADS */
-
-#define MARIADB_BEGIN_ALLOW_THREADS(obj)\
-{\
-  (obj)->thread_state= PyEval_SaveThread();\
-}
-
-#define MARIADB_END_ALLOW_THREADS(obj)\
-if ((obj)->thread_state)\
-{\
-    PyEval_RestoreThread((obj)->thread_state);\
-    (obj)->thread_state= NULL;\
-}
-
-#define MARIADB_UNBLOCK_THREADS(obj)\
-{\
-    if ((obj)->thread_state)\
-    {\
-        _save= (obj)->thread_state;\
-        PyEval_RestoreThread(_save);\
-        (obj)->thread_state= NULL;\
-    }\
-}
-
-#define MARIADB_BLOCK_THREADS(obj)\
-    if (_save)\
-    {\
-        (obj)->thread_state= PyEval_SaveThread();\
-        _save= NULL;\
-    }

mariadb/mariadb_codecs.c

@@ -677,13 +677,18 @@ field_fetch_callback(void *data, unsigned int column, unsigned char **row)
     MrdbCursor *self= (MrdbCursor *)data;
     Mrdb_ExtFieldType *ext_field_type;
 
+    PyGILState_STATE gstate;
+
+    /* Acquire the GIL */
+    gstate = PyGILState_Ensure();
+
     ext_field_type= mariadb_extended_field_type(&self->fields[column]);
 
     if (!row)
     {
         Py_INCREF(Py_None);
         self->values[column]= Py_None;
-        return;
+        goto end;
     }
     switch(self->fields[column].type) {
         case MYSQL_TYPE_NULL:
@@ -904,6 +909,10 @@ field_fetch_callback(void *data, unsigned int column, unsigned char **row)
         if ((val= ma_convert_value(self, type, self->values[column])))
             self->values[column]= val;
     }
+
+end:
+  /* Release the GIL */
+  PyGILState_Release(gstate);
 }
 
 static uint8_t ma_is_vector(PyObject *obj)

mariadb/mariadb_connection.c

@@ -212,33 +212,33 @@ int connection_datetime_init(void)
 void MrdbConnection_process_status_info(void *data, enum enum_mariadb_status_info type, ...)
 {
   va_list ap;
-  PyThreadState *_save= NULL;
   MrdbConnection *self= (MrdbConnection *)data;
   PyObject *dict= NULL;
   PyObject *dict_key= NULL, *dict_val= NULL;
-  va_start(ap, type);
 
+  PyGILState_STATE gstate;
+  /* Acquire the GIL */
+  gstate = PyGILState_Ensure();
+
+  va_start(ap, type);
   if (self->status_callback) {
     if (type == STATUS_TYPE)
     {
       unsigned int server_status= va_arg(ap, int);
 
-      MARIADB_UNBLOCK_THREADS(self);
       dict_key= PyUnicode_FromString("server_status");
       dict_val= PyLong_FromLong(server_status);
       dict= PyDict_New();
       PyDict_SetItem(dict, dict_key, dict_val);
       Py_DECREF(dict_key);
       Py_DECREF(dict_val);
       PyObject_CallFunction(self->status_callback, "OO", (PyObject *)data, dict);
-      MARIADB_BLOCK_THREADS(self);
     }
   }
   if (type == SESSION_TRACK_TYPE)
   {
     enum enum_session_state_type track_type= va_arg(ap, enum enum_session_state_type);
 
-    MARIADB_UNBLOCK_THREADS(self);
 
     if (self->status_callback) {
       switch (track_type) {
@@ -279,9 +279,9 @@ void MrdbConnection_process_status_info(void *data, enum enum_mariadb_status_inf
 
         memcpy(charset, val->str, val->length);
         charset[val->length]= 0;
-        va_end(ap);
         mariadb_throw_exception(NULL, Mariadb_ProgrammingError, 1,
                 "Character set '%s' is not supported", charset);
+        goto end;
       }
       if (self->status_callback)
       {
@@ -294,9 +294,11 @@ void MrdbConnection_process_status_info(void *data, enum enum_mariadb_status_inf
         PyObject_CallFunction(self->status_callback, "OO", (PyObject *)data, dict);
       }
     }
-    MARIADB_BLOCK_THREADS(self);
   }
+end:
   va_end(ap);
+  /* Release the GIL */
+  PyGILState_Release(gstate);
 } 
 #endif
 
@@ -376,8 +378,6 @@ MrdbConnection_Initialize(MrdbConnection *self,
    }
 #endif
 
-    MARIADB_BEGIN_ALLOW_THREADS(self);
-
     if (mysql_options(self->mysql, MYSQL_SET_CHARSET_NAME, mariadb_default_charset))
        goto end;
 
@@ -474,8 +474,10 @@ MrdbConnection_Initialize(MrdbConnection *self,
           goto end;
     }
 
+    Py_BEGIN_ALLOW_THREADS;
     mysql_real_connect(self->mysql, host, user, password, schema, port,
             socket, client_flags);
+    Py_END_ALLOW_THREADS;
 
     if (mysql_errno(self->mysql))
     {
@@ -489,7 +491,6 @@ MrdbConnection_Initialize(MrdbConnection *self,
 
     has_error= 0;
 end:
-    MARIADB_END_ALLOW_THREADS(self);
 
     if (has_error)
     {
@@ -567,9 +568,9 @@ void MrdbConnection_finalize(MrdbConnection *self)
     {
         if (self->mysql)
         {
-            MARIADB_BEGIN_ALLOW_THREADS(self)
+            Py_BEGIN_ALLOW_THREADS
             mysql_close(self->mysql);
-            MARIADB_END_ALLOW_THREADS(self)
+            Py_END_ALLOW_THREADS
             self->mysql= NULL;
         }
     }
@@ -586,9 +587,9 @@ MrdbConnection_executecommand(MrdbConnection *self,
 
   cmd= PyUnicode_AsUTF8AndSize(command, NULL);
 
-  MARIADB_BEGIN_ALLOW_THREADS(self);
+  Py_BEGIN_ALLOW_THREADS;
   rc= mysql_send_query(self->mysql, cmd, (long)strlen(cmd));
-  MARIADB_END_ALLOW_THREADS(self);
+  Py_END_ALLOW_THREADS;
 
   if (rc)
   {
@@ -604,9 +605,9 @@ PyObject *MrdbConnection_close(MrdbConnection *self)
     /* Todo: check if all the cursor stuff is deleted (when using prepared
        statements this should be handled in mysql_close) */
 
-    MARIADB_BEGIN_ALLOW_THREADS(self)
+    Py_BEGIN_ALLOW_THREADS
     mysql_close(self->mysql);
-    MARIADB_END_ALLOW_THREADS(self)
+    Py_END_ALLOW_THREADS
     self->mysql= NULL;
     self->closed= 1;
     Py_RETURN_NONE;
@@ -628,9 +629,9 @@ PyObject *MrdbConnection_ping(MrdbConnection *self)
 
     MARIADB_CHECK_CONNECTION(self, NULL);
 
-    MARIADB_BEGIN_ALLOW_THREADS(self);
+    Py_BEGIN_ALLOW_THREADS;
     rc= mysql_ping(self->mysql);
-    MARIADB_END_ALLOW_THREADS(self);
+    Py_END_ALLOW_THREADS;
 
     if (rc) {
         mariadb_throw_exception(self->mysql, Mariadb_InterfaceError, 0, NULL);
@@ -654,9 +655,9 @@ PyObject *MrdbConnection_change_user(MrdbConnection *self,
     if (!PyArg_ParseTuple(args, "szz", &user, &password, &database))
         return NULL;
 
-    MARIADB_BEGIN_ALLOW_THREADS(self);
+    Py_BEGIN_ALLOW_THREADS;
     rc= mysql_change_user(self->mysql, user, password, database);
-    MARIADB_END_ALLOW_THREADS(self);
+    Py_END_ALLOW_THREADS;
 
     if (rc)
     {
@@ -863,9 +864,9 @@ PyObject *MrdbConnection_reconnect(MrdbConnection *self)
     if (!save_reconnect)
         mysql_optionsv(self->mysql, MYSQL_OPT_RECONNECT, &reconnect);
 
-    MARIADB_BEGIN_ALLOW_THREADS(self);
+    Py_BEGIN_ALLOW_THREADS;
     rc= mariadb_reconnect(self->mysql);
-    MARIADB_END_ALLOW_THREADS(self);
+    Py_END_ALLOW_THREADS;
 
     if (!save_reconnect)
         mysql_optionsv(self->mysql, MYSQL_OPT_RECONNECT, &save_reconnect);
@@ -886,9 +887,9 @@ PyObject *MrdbConnection_reset(MrdbConnection *self)
     int rc;
     MARIADB_CHECK_CONNECTION(self, NULL);
 
-    MARIADB_BEGIN_ALLOW_THREADS(self);
+    Py_BEGIN_ALLOW_THREADS;
     rc= mysql_reset_connection(self->mysql);
-    MARIADB_END_ALLOW_THREADS(self);
+    Py_END_ALLOW_THREADS;
 
     if (rc)
     {
@@ -952,9 +953,9 @@ MrdbConnection_dump_debug_info(MrdbConnection *self)
     int rc;
     MARIADB_CHECK_CONNECTION(self, NULL);
 
-    MARIADB_BEGIN_ALLOW_THREADS(self);
+    Py_BEGIN_ALLOW_THREADS;
     rc= mysql_dump_debug_info(self->mysql);
-    MARIADB_END_ALLOW_THREADS(self);
+    Py_END_ALLOW_THREADS;
 
     if (rc)
     {
@@ -968,9 +969,9 @@ static PyObject *MrdbConnection_readresponse(MrdbConnection *self)
 {
     int rc;
 
-    MARIADB_BEGIN_ALLOW_THREADS(self);
+    Py_BEGIN_ALLOW_THREADS;
     rc= self->mysql->methods->db_read_query_result(self->mysql);
-    MARIADB_END_ALLOW_THREADS(self);
+    Py_END_ALLOW_THREADS;
 
     if (rc)
     {

mariadb/mariadb_cursor.c

@@ -169,9 +169,6 @@ static PyMethodDef MrdbCursor_Methods[] =
     {"_execute_bulk", (PyCFunction)MrdbCursor_execute_bulk,
         METH_NOARGS,
         NULL},
-    {"_initresult", (PyCFunction)MrdbCursor_InitResultSet,
-        METH_NOARGS,
-        NULL},
     {"_readresponse", (PyCFunction)MrdbCursor_readresponse,
         METH_NOARGS,
          NULL},
@@ -414,7 +411,6 @@ PyObject *MrdbCursor_clear_result(MrdbCursor *self)
             } while (!mysql_next_result(self->connection->mysql));
         }
     }
-    MARIADB_END_ALLOW_THREADS(self->connection);
     /* CONPY-52: Avoid possible double free */
     self->result= NULL;
     Py_RETURN_NONE;
@@ -506,9 +502,9 @@ void ma_cursor_close(MrdbCursor *self)
         {
             /* Todo: check if all the cursor stuff is deleted (when using prepared
                statements this should be handled in mysql_stmt_close) */
-            MARIADB_BEGIN_ALLOW_THREADS(self->connection);
+            Py_BEGIN_ALLOW_THREADS;
             mysql_stmt_close(self->stmt);
-            MARIADB_END_ALLOW_THREADS(self->connection);
+            Py_END_ALLOW_THREADS;
             self->stmt= NULL;
         }
         MrdbCursor_clear(self, 0);
@@ -630,7 +626,7 @@ static int Mrdb_execute_direct(MrdbCursor *self,
 {
    int rc;
 
-   MARIADB_BEGIN_ALLOW_THREADS(self->connection);
+   Py_BEGIN_ALLOW_THREADS;
    long ext_caps;
 
    mariadb_get_infov(self->connection->mysql,
@@ -660,7 +656,7 @@ static int Mrdb_execute_direct(MrdbCursor *self,
        rc= mariadb_stmt_execute_direct(self->stmt, statement, statement_len);
    }
 end:
-   MARIADB_END_ALLOW_THREADS(self->connection);
+   Py_END_ALLOW_THREADS;
    return rc;
 }
 
@@ -890,12 +886,12 @@ static PyObject *MrdbCursor_seek(MrdbCursor *self, PyObject *pos)
 
     new_position= (uint64_t)PyLong_AsUnsignedLongLong(pos);
 
-    MARIADB_BEGIN_ALLOW_THREADS(self->connection);
+    Py_BEGIN_ALLOW_THREADS;
     if (self->parseinfo.is_text)
         mysql_data_seek(self->result, new_position);
     else
         mysql_stmt_data_seek(self->stmt, new_position);
-    MARIADB_END_ALLOW_THREADS(self->connection);
+    Py_END_ALLOW_THREADS;
 
     Py_RETURN_NONE;
 }
@@ -929,9 +925,9 @@ MrdbCursor_nextset(MrdbCursor *self)
     {
         if (!self->stmt)
             Py_RETURN_NONE;
-        MARIADB_BEGIN_ALLOW_THREADS(self->connection);
+        Py_BEGIN_ALLOW_THREADS;
         rc= mysql_stmt_next_result(self->stmt);
-        MARIADB_END_ALLOW_THREADS(self->connection);
+        Py_END_ALLOW_THREADS;
     }
     else
     {
@@ -940,9 +936,9 @@ MrdbCursor_nextset(MrdbCursor *self)
             mysql_free_result(self->result);
             self->result= NULL;
         }
-        MARIADB_BEGIN_ALLOW_THREADS(self->connection);
+        Py_BEGIN_ALLOW_THREADS;
         rc= mysql_next_result(self->connection->mysql);
-        MARIADB_END_ALLOW_THREADS(self->connection);
+        Py_END_ALLOW_THREADS;
     }
 
     if (rc)
@@ -1143,9 +1139,9 @@ MrdbCursor_execute_text(MrdbCursor *self, PyObject *stmt)
     }
     db= self->connection->mysql;
 
-    MARIADB_BEGIN_ALLOW_THREADS(self->connection);
+    Py_BEGIN_ALLOW_THREADS;
     rc= mysql_send_query(db, statement, (long)statement_len);
-    MARIADB_END_ALLOW_THREADS(self->connection);
+    Py_END_ALLOW_THREADS;
 
     if (rc)
     {
@@ -1167,9 +1163,9 @@ MrdbCursor_readresponse(MrdbCursor *self)
 
     if (self->parseinfo.is_text)
     {
-        MARIADB_BEGIN_ALLOW_THREADS(self->connection);
+        Py_BEGIN_ALLOW_THREADS;
         rc= db->methods->db_read_query_result(db);
-        MARIADB_END_ALLOW_THREADS(self->connection);
+        Py_END_ALLOW_THREADS;
 
         if (rc)
         {