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

zooba · zkonge · web-flow · commit b9e288cc1bfd · 2020-07-04T00:04:22.000+01:00
Co-authored-by: Konge &lt;zkonge@outlook.com&gt;
diff --git 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
@@ -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
@@ -0,0 +1 @@
+Audit hooks are now cleared later during finalization to avoid missing events.
diff --git 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
@@ -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

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	`+Audit hooks are now cleared later during finalization to avoid missing events.`
Original file line number	Diff line number	Diff line change
`@@ -1106,8 +1106,11 @@ static int test_open_code_hook(void)`
`1106`	`1106`	`return result;`
`1107`	`1107`	`}`
`1108`	`1108`
	`1109`	`+static int _audit_hook_clear_count = 0;`
	`1110`	`+`
`1109`	`1111`	`static int _audit_hook(const char event, PyObject args, void *userdata)`
`1110`	`1112`	`{`
	`1113`	`+ assert(args && PyTuple_CheckExact(args));`
`1111`	`1114`	`if (strcmp(event, "_testembed.raise") == 0) {`
`1112`	`1115`	`PyErr_SetString(PyExc_RuntimeError, "Intentional error");`
`1113`	`1116`	`return -1;`
`@@ -1116,6 +1119,8 @@ static int _audit_hook(const char event, PyObject args, void *userdata)`
`1116`	`1119`	`return -1;`
`1117`	`1120`	`}`
`1118`	`1121`	`return 0;`
	`1122`	`+ } else if (strcmp(event, "cpython._PySys_ClearAuditHooks") == 0) {`
	`1123`	`+ _audit_hook_clear_count += 1;`
`1119`	`1124`	`}`
`1120`	`1125`	`return 0;`
`1121`	`1126`	`}`
`@@ -1161,6 +1166,9 @@ static int test_audit(void)`
`1161`	`1166`	`{`
`1162`	`1167`	`int result = _test_audit(42);`
`1163`	`1168`	`Py_Finalize();`
	`1169`	`+ if (_audit_hook_clear_count != 1) {`
	`1170`	`+ return 0x1000 \| _audit_hook_clear_count;`
	`1171`	`+ }`
`1164`	`1172`	`return result;`
`1165`	`1173`	`}`
`1166`	`1174`