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

zooba · zkonge · web-flow · commit e1d4fdc53347 · 2020-07-03T22:58:29.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
@@ -1116,8 +1116,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;
@@ -1126,6 +1129,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;
 }
@@ -1171,6 +1176,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
@@ -1303,6 +1303,13 @@ finalize_interp_clear(PyThreadState *tstate)
 
     finalize_interp_types(tstate, is_main_interp);
 
+    /* Clear all loghooks */
+    /* Both _PySys_Audit function and users still need PyObject, such as tuple.
+       Call _PySys_ClearAuditHooks when PyObject available. */
+    if (is_main_interp) {
+        _PySys_ClearAuditHooks(tstate);
+    }
+
     if (is_main_interp) {
         /* XXX Still allocated:
            - various static ad-hoc pointers to interned strings
@@ -1419,9 +1426,6 @@ Py_FinalizeEx(void)
      */
     _PyGC_CollectIfEnabled();
 
-    /* Clear all loghooks */
-    _PySys_ClearAuditHooks(tstate);
-
     /* Destroy all modules */
     _PyImport_Cleanup(tstate);
 

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
`@@ -1116,8 +1116,11 @@ static int test_open_code_hook(void)`
`1116`	`1116`	`return result;`
`1117`	`1117`	`}`
`1118`	`1118`
	`1119`	`+static int _audit_hook_clear_count = 0;`
	`1120`	`+`
`1119`	`1121`	`static int _audit_hook(const char event, PyObject args, void *userdata)`
`1120`	`1122`	`{`
	`1123`	`+ assert(args && PyTuple_CheckExact(args));`
`1121`	`1124`	`if (strcmp(event, "_testembed.raise") == 0) {`
`1122`	`1125`	`PyErr_SetString(PyExc_RuntimeError, "Intentional error");`
`1123`	`1126`	`return -1;`
`@@ -1126,6 +1129,8 @@ static int _audit_hook(const char event, PyObject args, void *userdata)`
`1126`	`1129`	`return -1;`
`1127`	`1130`	`}`
`1128`	`1131`	`return 0;`
	`1132`	`+ } else if (strcmp(event, "cpython._PySys_ClearAuditHooks") == 0) {`
	`1133`	`+ _audit_hook_clear_count += 1;`
`1129`	`1134`	`}`
`1130`	`1135`	`return 0;`
`1131`	`1136`	`}`
`@@ -1171,6 +1176,9 @@ static int test_audit(void)`
`1171`	`1176`	`{`
`1172`	`1177`	`int result = _test_audit(42);`
`1173`	`1178`	`Py_Finalize();`
	`1179`	`+ if (_audit_hook_clear_count != 1) {`
	`1180`	`+ return 0x1000 \| _audit_hook_clear_count;`
	`1181`	`+ }`
`1174`	`1182`	`return result;`
`1175`	`1183`	`}`
`1176`	`1184`