Skip to content

bpo-41162: Clear audit hooks after destructors #21222

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
zooba merged 5 commits into from
Jul 3, 2020
Merged

bpo-41162: Clear audit hooks after destructors #21222

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c232752
bpo-41162: Clear audit hooks after destructors
zkonge Jun 29, 2020
76350cc
Improve the description of comment
zkonge Jul 3, 2020
2ba1352
bpo-41162: Apply test patch by @zooba
zkonge Jul 3, 2020
3e8202d
bpo-41162: Delete useless tests
zkonge Jul 3, 2020
da16861
📜🤖 Added by blurb_it.
blurb-it[bot] Jul 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 0 additions & 26 deletions Lib/test/audit-tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -128,10 +106,6 @@ def test_block_add_hook_baseexception():
pass


def test_finalize_hooks():
sys.addaudithook(TestFinalizeHook())


def test_pickle():
import pickle

Expand Down
16 changes: 0 additions & 16 deletions Lib/test/test_audit.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")

Expand Down
1 change: 1 addition & 0 deletions Misc/NEWS.d/next/Security/2020-07-03-20-41-29.bpo-41162.tb8pVj.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Audit hooks are now cleared later during finalization to avoid missing events.
8 changes: 8 additions & 0 deletions Programs/_testembed.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
}
Expand Down Expand Up @@ -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;
}

Expand Down
10 changes: 7 additions & 3 deletions Python/pylifecycle.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1290,6 +1290,13 @@ finalize_interp_clear(PyThreadState *tstate)
_PyGC_CollectNoFail();
}

/* 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);

@vstinner vstinner Jun 30, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 zkonge Jun 30, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 zooba Jul 2, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

_PyGC_Fini(tstate);

if (is_main_interp) {
Expand Down Expand Up @@ -1404,9 +1411,6 @@ Py_FinalizeEx(void)
*/
_PyGC_CollectIfEnabled();

/* Clear all loghooks */
_PySys_ClearAuditHooks(tstate);

/* Destroy all modules */
_PyImport_Cleanup(tstate);

Expand Down