fix more crashes and move test where they belong

Author: picnixz

Lib/test/test_sqlite3/test_dbapi.py

@@ -2029,104 +2029,5 @@ def test_row_is_a_sequence(self):
         self.assertIsInstance(row, Sequence)
 
 
-class CallbackTests(unittest.TestCase):
-
-    def setUp(self):
-        super().setUp()
-        self.cx = sqlite.connect(":memory:")
-        self.addCleanup(self.cx.close)
-        self.cu = self.cx.cursor()
-        self.cu.execute("create table test(a number)")
-
-        class Handler:
-            cx = self.cx
-
-        self.handler_class = Handler
-
-    def assert_not_authorized(self, func, /, *args, **kwargs):
-        with self.assertRaisesRegex(sqlite.DatabaseError, "not authorized"):
-            func(*args, **kwargs)
-
-    def assert_interrupted(self, func, /, *args, **kwargs):
-        with self.assertRaisesRegex(sqlite.OperationalError, "interrupted"):
-            func(*args, **kwargs)
-
-    def assert_invalid_trace(self, func, /, *args, **kwargs):
-        # Exception in trace callbacks are entirely suppressed.
-        pass
-
-    # When a handler has an invalid signature, the exception raised is
-    # the same that would be raised if the handler "negatively" replied.
-
-    def test_authorizer_invalid_signature(self):
-        self.cx.set_authorizer(lambda: None)
-        self.assert_not_authorized(self.cx.execute, "select * from test")
-
-    def test_progress_handler_invalid_signature(self):
-        self.cx.set_progress_handler(lambda x: None, 1)
-        self.assert_interrupted(self.cx.execute, "select * from test")
-
-    def test_trace_callback_invalid_signature_traceback(self):
-        self.cx.set_trace_callback(lambda: None)
-        self.assert_invalid_trace(self.cx.execute, "select * from test")
-
-    # Tests for checking that callback context mutations do not crash.
-    # Regression tests for https://github.com/python/cpython/issues/142830.
-
-    def test_authorizer_concurrent_mutation_in_call(self):
-        class Handler(self.handler_class):
-            def __call__(self, *a, **kw):
-                self.cx.set_authorizer(None)
-                raise ValueError
-
-        self.cx.set_authorizer(Handler())
-        self.assert_not_authorized(self.cx.execute, "select * from test")
-
-    def test_authorizer_concurrent_mutation_with_overflown_value(self):
-        _testcapi = import_helper.import_module("_testcapi")
-
-        class Handler(self.handler_class):
-            def __call__(self, *a, **kw):
-                self.cx.set_authorizer(None)
-                # We expect 'int' at the C level, so this one will raise
-                # when converting via PyLong_Int().
-                return _testcapi.INT_MAX + 1
-
-        self.cx.set_authorizer(Handler())
-        self.assert_not_authorized(self.cx.execute, "select * from test")
-
-    def test_progress_handler_concurrent_mutation_in_call(self):
-        class Handler(self.handler_class):
-            def __call__(self, *a, **kw):
-                self.cx.set_authorizer(None)
-                raise ValueError
-
-        self.cx.set_progress_handler(Handler(), 1)
-        self.assert_interrupted(self.cx.execute, "select * from test")
-
-    def test_progress_handler_concurrent_mutation_in_conversion(self):
-        class Handler(self.handler_class):
-            def __bool__(self):
-                # clear the progress handler
-                self.cx.set_progress_handler(None, 1)
-                raise ValueError  # force PyObject_True() to fail
-
-        self.cx.set_progress_handler(Handler.__init__, 1)
-        self.assert_interrupted(self.cx.execute, "select * from test")
-
-    def test_trace_callback_concurrent_mutation_in_call(self):
-        class Handler:
-            def __call__(self, statement):
-                # clear the progress handler
-                self.cx.set_progress_handler(None, 1)
-                raise ValueError
-
-        self.cx.set_trace_callback(Handler())
-        self.assert_invalid_trace(self.cx.execute, "select * from test")
-
-    # TODO(picnixz): increase test coverage for other callbacks
-    # such as 'func', 'step', 'finalize', and 'collation'.
-
-
 if __name__ == "__main__":
     unittest.main()

Lib/test/test_sqlite3/test_hooks.py

@@ -24,11 +24,15 @@
 import sqlite3 as sqlite
 import unittest
 
+from test.support import import_helper
 from test.support.os_helper import TESTFN, unlink
 
 from .util import memory_database, cx_limit, with_tracebacks
 from .util import MemoryDatabaseMixin
 
+# TODO(picnixz): increase test coverage for other callbacks
+# such as 'func', 'step', 'finalize', and 'collation'.
+
 
 class CollationTests(MemoryDatabaseMixin, unittest.TestCase):
 
@@ -129,8 +133,59 @@ def test_deregister_collation(self):
         self.assertEqual(str(cm.exception), 'no such collation sequence: mycoll')
 
 
+class AuthorizerTests(MemoryDatabaseMixin, unittest.TestCase):
+
+    def assert_not_authorized(self, func, /, *args, **kwargs):
+        with self.assertRaisesRegex(sqlite.DatabaseError, "not authorized"):
+            func(*args, **kwargs)
+
+    # When a handler has an invalid signature, the exception raised is
+    # the same that would be raised if the handler "negatively" replied.
+
+    def test_authorizer_invalid_signature(self):
+        self.cx.execute("create table if not exists test(a number)")
+        self.cx.set_authorizer(lambda: None)
+        self.assert_not_authorized(self.cx.execute, "select * from test")
+
+    # Tests for checking that callback context mutations do not crash.
+    # Regression tests for https://github.com/python/cpython/issues/142830.
+
+    @with_tracebacks(ZeroDivisionError, regex="hello world")
+    def test_authorizer_concurrent_mutation_in_call(self):
+        self.cx.execute("create table if not exists test(a number)")
+
+        class Handler:
+            cx = self.cx
+            def __call__(self, *a, **kw):
+                self.cx.set_authorizer(None)
+                raise ZeroDivisionError("hello world")
+
+        self.cx.set_authorizer(Handler())
+        self.assert_not_authorized(self.cx.execute, "select * from test")
+
+    @with_tracebacks(OverflowError)
+    def test_authorizer_concurrent_mutation_with_overflown_value(self):
+        _testcapi = import_helper.import_module("_testcapi")
+        self.cx.execute("create table if not exists test(a number)")
+
+        class Handler:
+            cx = self.cx
+            def __call__(self, *a, **kw):
+                self.cx.set_authorizer(None)
+                # We expect 'int' at the C level, so this one will raise
+                # when converting via PyLong_Int().
+                return _testcapi.INT_MAX + 1
+
+        self.cx.set_authorizer(Handler())
+        self.assert_not_authorized(self.cx.execute, "select * from test")
+
+
 class ProgressTests(MemoryDatabaseMixin, unittest.TestCase):
 
+    def assert_interrupted(self, func, /, *args, **kwargs):
+        with self.assertRaisesRegex(sqlite.OperationalError, "interrupted"):
+            func(*args, **kwargs)
+
     def test_progress_handler_used(self):
         """
         Test that the progress handler is invoked once it is set.
@@ -219,11 +274,51 @@ def bad_progress():
                 create table foo(a, b)
                 """)
 
-    def test_progress_handler_keyword_args(self):
+    def test_set_progress_handler_keyword_args(self):
         with self.assertRaisesRegex(TypeError,
                 'takes at least 1 positional argument'):
             self.con.set_progress_handler(progress_handler=lambda: None, n=1)
 
+    # When a handler has an invalid signature, the exception raised is
+    # the same that would be raised if the handler "negatively" replied.
+
+    def test_progress_handler_invalid_signature(self):
+        self.cx.execute("create table if not exists test(a number)")
+        self.cx.set_progress_handler(lambda x: None, 1)
+        self.assert_interrupted(self.cx.execute, "select * from test")
+
+    # Tests for checking that callback context mutations do not crash.
+    # Regression tests for https://github.com/python/cpython/issues/142830.
+
+    @with_tracebacks(ZeroDivisionError, regex="hello world")
+    def test_progress_handler_concurrent_mutation_in_call(self):
+        self.cx.execute("create table if not exists test(a number)")
+
+        class Handler:
+            cx = self.cx
+            def __call__(self, *a, **kw):
+                self.cx.set_progress_handler(None, 1)
+                raise ZeroDivisionError("hello world")
+
+        self.cx.set_progress_handler(Handler(), 1)
+        self.assert_interrupted(self.cx.execute, "select * from test")
+
+    def test_progress_handler_concurrent_mutation_in_conversion(self):
+        self.cx.execute("create table if not exists test(a number)")
+
+        class Handler:
+            cx = self.cx
+            def __bool__(self):
+                # clear the progress handler
+                self.cx.set_progress_handler(None, 1)
+                raise ValueError  # force PyObject_True() to fail
+
+        self.cx.set_progress_handler(Handler.__init__, 1)
+        self.assert_interrupted(self.cx.execute, "select * from test")
+
+        # Running with tracebacks makes the second execution of this
+        # function raise another exception because of a database change.
+
 
 class TraceCallbackTests(MemoryDatabaseMixin, unittest.TestCase):
 
@@ -345,11 +440,42 @@ def test_trace_bad_handler(self):
             cx.set_trace_callback(lambda stmt: 5/0)
             cx.execute("select 1")
 
-    def test_trace_keyword_args(self):
+    def test_set_trace_callback_keyword_args(self):
         with self.assertRaisesRegex(TypeError,
                 'takes exactly 1 positional argument'):
             self.con.set_trace_callback(trace_callback=lambda: None)
 
+    # When a handler has an invalid signature, the exception raised is
+    # the same that would be raised if the handler "negatively" replied,
+    # but for the trace handler, exceptions are never re-raised (only
+    # printed when needed).
+
+    @with_tracebacks(
+        TypeError,
+        regex=r".*<lambda>\(\) missing 6 required positional arguments",
+    )
+    def test_trace_handler_invalid_signature(self):
+        self.cx.execute("create table if not exists test(a number)")
+        self.cx.set_trace_callback(lambda x, y, z, t, a, b, c: None)
+        self.cx.execute("select * from test")
+
+    # Tests for checking that callback context mutations do not crash.
+    # Regression tests for https://github.com/python/cpython/issues/142830.
+
+    @with_tracebacks(ZeroDivisionError, regex="hello world")
+    def test_trace_callback_concurrent_mutation_in_call(self):
+        self.cx.execute("create table if not exists test(a number)")
+
+        class Handler:
+            cx = self.cx
+            def __call__(self, statement):
+                # clear the progress handler
+                self.cx.set_trace_callback(None)
+                raise ZeroDivisionError("hello world")
+
+        self.cx.set_trace_callback(Handler())
+        self.cx.execute("select * from test")
+
 
 if __name__ == "__main__":
     unittest.main()

Lib/test/test_sqlite3/util.py

@@ -50,7 +50,7 @@ def check_tracebacks(self, cm, exc, exc_regex, msg_regex, obj_name):
         with contextlib.redirect_stderr(buf):
             yield
 
-        self.assertEqual(cm.unraisable.exc_type, exc)
+        self.assertIsSubclass(cm.unraisable.exc_type, exc)
         if exc_regex:
             msg = str(cm.unraisable.exc_value)
             self.assertIsNotNone(exc_regex.search(msg), (exc_regex, msg))

Modules/_sqlite/connection.c

@@ -1450,6 +1450,8 @@ trace_callback(unsigned int type, void *ctx_vp, void *stmt, void *sql)
 
     assert(ctx_vp != NULL);
     pysqlite_CallbackContext *ctx = pysqlite_CallbackContext_CAST(ctx_vp);
+    // Hold a reference to 'ctx' to prevent concurrent mutations.
+    Py_INCREF(ctx);
     pysqlite_state *state = ctx->state;
     assert(state != NULL);
 
@@ -1474,9 +1476,7 @@ trace_callback(unsigned int type, void *ctx_vp, void *stmt, void *sql)
         sqlite3_free((void *)expanded_sql);
     }
     if (py_statement) {
-        Py_INCREF(ctx);
         PyObject *ret = PyObject_CallOneArg(ctx->callable, py_statement);
-        Py_DECREF(ctx);
         Py_DECREF(py_statement);
         Py_XDECREF(ret);
     }
@@ -1485,6 +1485,7 @@ trace_callback(unsigned int type, void *ctx_vp, void *stmt, void *sql)
     }
 
 exit:
+    Py_DECREF(ctx);
     PyGILState_Release(gilstate);
     return 0;
 }

Modules/_sqlite/context.c

@@ -49,7 +49,6 @@ callback_context_new_impl(PyTypeObject *type, PyObject *callable)
     ctx->state = state;
     PyObject_GC_Track(ctx);
     return (PyObject *)ctx;
-
 }
 
 static int
@@ -104,8 +103,7 @@ static PyType_Spec callback_context_spec = {
 PyObject *
 pysqlite_create_callback_context(pysqlite_state *state, PyObject *callable)
 {
-    PyTypeObject *type = state->CallbackContextType;
-    return callback_context_new_impl(type, callable);
+    return callback_context_new_impl(state->CallbackContextType, callable);
 }
 
 int

Modules/_sqlite/context.h

@@ -9,6 +9,12 @@ extern "C" {
 
 #include "module.h"
 
+/*
+ * UDF/callback context structure.
+ *
+ * In order to ensure that the state pointer always outlives the callback
+ * context, we make sure it owns a reference to the module itself.
+ */
 typedef struct {
     PyObject_HEAD
     PyObject *callable;
@@ -18,12 +24,6 @@ typedef struct {
 
 #define pysqlite_CallbackContext_CAST(op)   ((pysqlite_CallbackContext *)(op))
 
-/* Allocate a UDF/callback context structure. In order to ensure that the state
- * pointer always outlives the callback context, we make sure it owns a
- * reference to the module itself. create_callback_context() is always called
- * from connection methods, so we use the defining class to fetch the module
- * pointer.
- */
 PyObject *
 pysqlite_create_callback_context(pysqlite_state *state, PyObject *callable);