gh-152912: Fix audit hook exception check in sys.addaudithook()

[k00shi]

Python/sysmodule.c:530 used PyExc_Exception to check exceptions
raised by existing audit hooks, catching every Exception subclass.
PySys_AddAuditHook (line 486) and Doc/library/sys.rst both specify
RuntimeError only. Change to PyExc_RuntimeError.

[python-cla-bot]

All commit authors signed the Contributor License Agreement.

[Aniketsy]

please avoid force-push, also for the lint error you can run pre-commit run before pushing.

[StanFromIreland]

It has been like this since the initial implementation in 2019, and isn't specified in PEP 578. While the sys.addaudithook documentation specifies RuntimeError, the equivalent C-API (PySys_AddAuditHook) correctly specifies Exception. I think it we should update the documentation here.

[bedevere-app]

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

[k00shi]

Thanks for the review, @StanFromIreland. I'd like to push back on the requested change. The asymmetry looks like a regression from 3.8.1, not original intent. The C-API code does specify RuntimeError.

1. The C-API code uses PyExc_RuntimeError, not PyExc_Exception.

Python/sysmodule.c:486 (in PySys_AddAuditHook):

if (_PyErr_ExceptionMatches(tstate, PyExc_RuntimeError)) {
    /* We do not report errors derived from RuntimeError */
    _PyErr_Clear(tstate);
    return 0;
}

This has been the case since the 3.8.1 cycle. The C-API code and the Python-API documentation (Doc/library/sys.rst:96-101 plus the .. versionchanged:: 3.8.1 note at :108-111) agree on RuntimeError only.

2. The asymmetry came from a partial fix in 3.8.1, not from the original PEP 578 landing.

The original PEP 578 commit (b82e17e6, 2019-05-23, Steve Dower) introduced both checks symmetrically as PyExc_Exception.

The 3.8.1 narrowing commit (bea33f5e1, 2019-11-28, same author, bpo-38920 / GH-17392) narrowed only the C-API code at line 486, and in the same commit it updated Doc/library/sys.rst to say RuntimeError and added the versionchanged:: 3.8.1 note. The intent was to narrow to RuntimeError everywhere. sys_addaudithook_impl at line 530, further down in the same file, was missed.

You can verify with git show bea33f5e1 -- Python/sysmodule.c Doc/library/sys.rst. The commit touches line 486 and the Python-API doc, but not line 530.

3. The one place that still says Exception is Doc/c-api/sys.rst.

Doc/c-api/sys.rst:436 and :450 (C-API doc) still say "Exception" when describing PySys_AddAuditHook's suppression behavior. This is a stale doc. bea33f5e1 updated Doc/library/sys.rst but missed Doc/c-api/sys.rst. That is what led to the impression that "the C-API specifies Exception".

Since the C-API code has used RuntimeError since 3.8.1, I'd like to keep my code change as is. To close the loop on the doc inconsistency, I can also fix Doc/c-api/sys.rst in this PR. Would you prefer that here, or as a separate follow-up?

On PEP 578: agreed it is silent on the exception type. The implementation and docs have converged on RuntimeError since 3.8.1, and this PR restores that contract.

[StanFromIreland]

Oh alright, I thought it was just one path. Indeed, the C-API documentation is wrong, and that should be fixed (we can do it in this PR). However, I'm still not sure about this change, while the current logic contradicts the documentation, I worry changing it now would cause too much breakage. I presume that realistically, people are relying on the current behaviour.

[zooba]

Right, apparently when I made the change I fixed one bit of documentation and then the other implementation. Which surprises me a bit, TBH, but I didn't bother doing all the checks to see if any of the refactors since might have led to the changes moving around.

I'm still not sure about this change, while the current logic contradicts the documentation, I worry changing it now would cause too much breakage

I think it's very unlikely. The main change is that a legitimate error (non-RuntimeError) that was previously being swallowed will now be reported at the place it occurs, which is a good thing. Hooks are intended to only raise errors to abort the process (sys.audit docs say "In general, if an exception is raised, it should not be handled and the process should be terminated as quickly as possible"), and so if someone is intentionally raising something other than RuntimeError to abort adding a new hook, despite the documentation saying that's the one they need to raise, they're likely just going to get an unhandled error rather than a potential exploit.

Malicious code that's adding a new hook is outside of our threat model (requires arbitrary code execution), and anyway it doesn't really provide any benefit (you can't get a later hook to run before an existing one, even if you manage to get added because the original hook didn't follow the docs).

That said, we shouldn't be backporting this any earlier than 3.15. It's a bugfix only, not a security issue.

[zooba]

And we should update the docs for the native API as well, as Stan said. Please ask your AI agent to only generate a brief message this time - we don't need to waste our time reading all that, thank you.

[read-the-docs-community]

Documentation build overview

📚 cpython-previews | 🛠️ Build #33478429 | 📁 Comparing f010ca5 against main (2d7a74e)

  🔍 Preview build  

43 files changed · ± 43 modified

± Modified

• c-api/sys.html
• extending/first-extension-module.html
• extending/newtypes_tutorial.html
• faq/programming.html
• howto/descriptor.html
• library/ast.html
• library/codecs.html
• library/datetime.html
• library/dbm.html
• library/decimal.html
• and 33 more...

[k00shi]

I have made the requested changes; please review again.

• Kept the sysmodule.c:530 fix (PyExc_Exception -> PyExc_RuntimeError).
• Added a follow-up commit fixing Doc/c-api/sys.rst:436, 450.
• Targeting 3.15, no backport.

[bedevere-app]

Thanks for making the requested changes!

@StanFromIreland: please review the changes made to this pull request.

[zooba]

LGTM. Not sure why those builds failed, but I'll poke them to re-run and hopefully it's actually clean. If not, you'll need to dig through the output to see why they're failing

[StanFromIreland]

The races in TSan (free threading) and test_logging failures on Windows are known flaky tests.

[StanFromIreland]

Please put this after the .. versionadded:: 3.8 at the bottom.

[k00shi]

Gotcha