[Python-checkins] bpo-41162: Clear audit hooks later during finalization (GH-21222)

[Steve Dower]

https://github.com/python/cpython/commit/b9e288cc1bfd583e887f784e38d9c511b43c0c3a
commit: b9e288cc1bfd583e887f784e38d9c511b43c0c3a
branch: 3.8
author: Steve Dower <steve.dower at python.org>
committer: GitHub <noreply at github.com>
date: 2020-07-04T00:04:22+01:00
summary:

bpo-41162: Clear audit hooks later during finalization (GH-21222)

Co-authored-by: Konge <zkonge at outlook.com>

files:
A Misc/NEWS.d/next/Security/2020-07-03-20-41-29.bpo-41162.tb8pVj.rst
M Lib/test/audit-tests.py
M Lib/test/test_audit.py
M Programs/_testembed.c
M Python/pylifecycle.c

diff --git a/Lib/test/audit-tests.py b/Lib/test/audit-tests.py
index a58395b068b39..ee6fc93351b75 100644
--- a/Lib/test/audit-tests.py
+++ b/Lib/test/audit-tests.py
@@ -44,28 +44,6 @@ def __call__(self, event, args):
             raise self.exc_type("saw event " + event)
 
 
-class TestFinalizeHook:
-    """Used in the test_finalize_hooks function to ensure that hooks
-    are correctly cleaned up, that they are notified about the cleanup,
-    and are unable to prevent it.
-    """
-
-    def __init__(self):
-        print("Created", id(self), file=sys.stdout, flush=True)
-
-    def __call__(self, event, args):
-        # Avoid recursion when we call id() below
-        if event == "builtins.id":
-            return
-
-        print(event, id(self), file=sys.stdout, flush=True)
-
-        if event == "cpython._PySys_ClearAuditHooks":
-            raise RuntimeError("Should be ignored")
-        elif event == "cpython.PyInterpreterState_Clear":
-            raise RuntimeError("Should be ignored")
-
-
 # Simple helpers, since we are not in unittest here
 def assertEqual(x, y):
     if x != y:
@@ -128,10 +106,6 @@ def test_block_add_hook_baseexception():
                 pass
 
 
-def test_finalize_hooks():
-    sys.addaudithook(TestFinalizeHook())
-
-
 def test_pickle():
     import pickle
 
diff --git a/Lib/test/test_audit.py b/Lib/test/test_audit.py
index f405c6923979c..f79edbc4bd0d9 100644
--- a/Lib/test/test_audit.py
+++ b/Lib/test/test_audit.py
@@ -51,22 +51,6 @@ def test_block_add_hook(self):
     def test_block_add_hook_baseexception(self):
         self.do_test("test_block_add_hook_baseexception")
 
-    def test_finalize_hooks(self):
-        returncode, events, stderr = self.run_python("test_finalize_hooks")
-        if stderr:
-            print(stderr, file=sys.stderr)
-        if returncode:
-            self.fail(stderr)
-
-        firstId = events[0][2]
-        self.assertSequenceEqual(
-            [
-                ("Created", " ", firstId),
-                ("cpython._PySys_ClearAuditHooks", " ", firstId),
-            ],
-            events,
-        )
-
     def test_pickle(self):
         support.import_module("pickle")
 
diff --git a/Misc/NEWS.d/next/Security/2020-07-03-20-41-29.bpo-41162.tb8pVj.rst b/Misc/NEWS.d/next/Security/2020-07-03-20-41-29.bpo-41162.tb8pVj.rst
new file mode 100644
index 0000000000000..f0333ac4a7bb3
--- /dev/null
+++ b/Misc/NEWS.d/next/Security/2020-07-03-20-41-29.bpo-41162.tb8pVj.rst
@@ -0,0 +1 @@
+Audit hooks are now cleared later during finalization to avoid missing events.
\ No newline at end of file
diff --git a/Programs/_testembed.c b/Programs/_testembed.c
index b98a38a1ba678..460d70ccadefd 100644
--- a/Programs/_testembed.c
+++ b/Programs/_testembed.c
@@ -1106,8 +1106,11 @@ static int test_open_code_hook(void)
     return result;
 }
 
+static int _audit_hook_clear_count = 0;
+
 static int _audit_hook(const char *event, PyObject *args, void *userdata)
 {
+    assert(args && PyTuple_CheckExact(args));
     if (strcmp(event, "_testembed.raise") == 0) {
         PyErr_SetString(PyExc_RuntimeError, "Intentional error");
         return -1;
@@ -1116,6 +1119,8 @@ static int _audit_hook(const char *event, PyObject *args, void *userdata)
             return -1;
         }
         return 0;
+    } else if (strcmp(event, "cpython._PySys_ClearAuditHooks") == 0) {
+        _audit_hook_clear_count += 1;
     }
     return 0;
 }
@@ -1161,6 +1166,9 @@ static int test_audit(void)
 {
     int result = _test_audit(42);
     Py_Finalize();
+    if (_audit_hook_clear_count != 1) {
+        return 0x1000 | _audit_hook_clear_count;
+    }
     return result;
 }
 
diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c
index 27cebf33da544..dc2d13db419e5 100644
--- a/Python/pylifecycle.c
+++ b/Python/pylifecycle.c
@@ -1229,13 +1229,6 @@ Py_FinalizeEx(void)
         /* nothing */;
 #endif
 
-    /* Clear all loghooks */
-    /* We want minimal exposure of this function, so define the extern
-     * here. The linker should discover the correct function without
-     * exporting a symbol. */
-    extern void _PySys_ClearAuditHooks(void);
-    _PySys_ClearAuditHooks();
-
     /* Destroy all modules */
     PyImport_Cleanup();
 
@@ -1306,6 +1299,13 @@ Py_FinalizeEx(void)
     /* Clear interpreter state and all thread states. */
     PyInterpreterState_Clear(interp);
 
+    /* Clear all loghooks */
+    /* We want minimal exposure of this function, so define the extern
+     * here. The linker should discover the correct function without
+     * exporting a symbol. */
+    extern void _PySys_ClearAuditHooks(void);
+    _PySys_ClearAuditHooks();
+
     /* Now we decref the exception classes.  After this point nothing
        can raise an exception.  That's okay, because each Fini() method
        below has been checked to make sure no exceptions are ever