bpo-41162: Clear audit hooks after destructors

[zkonge]

https://bugs.python.org/issue41162

[zkonge]

Although calling _PySys_ClearAuditHooks just before _PyImport_Cleanup make the test success.

I think it's better to remove the global hook after the interpreter hook?

[vstinner]

It would be safer to call it after PyInterpreterState_Delete(). It requires to simplify _PySys_ClearAuditHooks() to only access _PyRuntime: don't call _PySys_Audit() anymore, and avoid PyThreadState * since it doesn't exist anymore after PyInterpreterState_Delete().

After PyInterpreterState_Delete() is called, it's no longer possible to execute Python code.

I suggest to move the call in Py_Finalize() after finalize_interp_delete() call.

[zkonge]

That means we also need to change pep-0578. C-level hook removal won't cause sys._clearaudithooks event.

I think there is no chance for pure python code to execute code in this patch.
...Or maybe they change object in finalize_interp_types with ctypes.

If it's possible to execute Python code in this place, should we also redesign interpreter hook removal opportunity？

[zooba]

The hook can (and will) do anything, but the more important issue is that PySys_Audit will create a tuple object, which can't be allowed after finalization. And we can't change the API to allow passing NULL for args at this stage.

After doing my own experimentation, this is the best place to put it.

[vstinner]

That means we also need to change pep-0578. C-level hook removal won't cause sys._clearaudithooks event.

It seems safer to me to remove the sys._clearaudithooks event.

https://bugs.python.org/issue41162 is in contradiction with the sys._clearaudithooks event. https://bugs.python.org/issue41162 says: "it should not be possible to execute arbitrary Python code after the sys._clearaudithooks event. But an audit hook can execute arbitrary Python code (on purpose) and so needs to be able to do that. We cannot emit the event once it is no longer possible to execute arbitrary Python code. And IMO the safest option is to only clear audit hooks once it is no possible to execute Python code.

cc @zooba @tiran

[zkonge]

I found _PySys_ClearAuditHooks will trigger a cpython._PySys_ClearAuditHooks event instead of sys._clearaudithooks in pep-0578. Is it a mistake?

[zooba]

The PEP only listed suggested events, not the official list. https://docs.python.org/3/library/audit_events.html is the reference.

Also, I made these updates to _testembed.c to check that the event is raised, since the pure Python hooks in test_audit can no longer see it.

--- a/Programs/_testembed.c
+++ b/Programs/_testembed.c
@@ -1112,8 +1112,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;
@@ -1122,6 +1125,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;
 }
@@ -1167,6 +1172,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;
 }

[zkonge]

Do we need more discussion about the security?

[zooba]

@zkonge Probably, but let's do it on the issue tracker. PR discussions basically disappear after merging.

[zooba]

@zkonge We will want a NEWS entry for this - click on Details for the failing check for instructions.

I suggest: "Audit hooks are now cleared later during finalization to avoid missing events"

[zkonge]

Thanks for your guidance!

[miss-islington]

Thanks @zkonge for the PR, and @zooba for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8, 3.9.
🐍🍒⛏🤖

[miss-islington]

Sorry, @zkonge and @zooba, I could not cleanly backport this to 3.9 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker daa0fe03a517d335d48e65ace8e5da636e265a8f 3.9

[miss-islington]

Sorry @zkonge and @zooba, I had trouble checking out the 3.8 backport branch.
Please backport using cherry_picker on command line.
cherry_picker daa0fe03a517d335d48e65ace8e5da636e265a8f 3.8

[zooba]

Thanks for the contribution! If you've got other ideas about how to make stuff happen after hooks are cleaned up, please post them on the bug so we can figure out how to detect them.

[bedevere-bot]

GH-21302 is a backport of this pull request to the 3.9 branch.