Skip to content
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

Jump to bottom

gh-126018: Avoid aborting due to unnecessary assert in sys.audit #126020

Merged
merged 7 commits into from
Oct 27, 2024
Merged

gh-126018: Avoid aborting due to unnecessary assert in sys.audit #126020

merged 7 commits into from
Oct 27, 2024

Conversation

devdanzin
Copy link
Contributor

@devdanzin devdanzin commented Oct 26, 2024 •
edited by bedevere-app bot
Loading

This PR removes an assert in sysmodule.c that caused the interpreter to abort when sys.audit received a non-string as first argument.

Without this assert, the code that runs the hooks will raise an exception instead.

@bedevere-app bedevere-app bot mentioned this pull request Oct 26, 2024
Closed
@picnixz picnixz added needs backport to 3.12 bug and security fixes needs backport to 3.13 bugs and security fixes labels Oct 26, 2024
@picnixz
Copy link
Contributor

picnixz commented Oct 26, 2024

This likely requires some NEWS entry, for instance:

Fix a crash in :func:`sys.audit` when passing a non-string argument.

picnixz
picnixz reviewed Oct 26, 2024
View reviewed changes
Lib/test/audit-tests.py Outdated Show resolved Hide resolved
Lib/test/test_audit.py Outdated Show resolved Hide resolved
Eclips4
Eclips4 reviewed Oct 26, 2024
View reviewed changes
Lib/test/test_audit.py Outdated Show resolved Hide resolved
@picnixz picnixz removed the needs backport to 3.12 bug and security fixes label Oct 26, 2024
@picnixz
Copy link
Contributor

picnixz commented Oct 26, 2024

FTR: 3.12 is not affected because the assertion does not exist:

cpython/Python/sysmodule.c

Lines 499 to 514 in 67b2701

if (!should_audit(tstate->interp)) {
Py_RETURN_NONE;
}
PyObject *auditEvent = args[0];
if (!auditEvent) {
_PyErr_SetString(tstate, PyExc_TypeError,
"expected str for argument 'event'");
return NULL;
}
if (!PyUnicode_Check(auditEvent)) {
_PyErr_Format(tstate, PyExc_TypeError,
"expected str for argument 'event', not %.200s",
Py_TYPE(auditEvent)->tp_name);
return NULL;
}

@devdanzin
Copy link
Contributor Author

Thank you very much for the review and suggestions @picnixz and @Eclips4!

ZeroIntensity
ZeroIntensity reviewed Oct 27, 2024
View reviewed changes
Copy link
Member

@ZeroIntensity ZeroIntensity left a comment

Choose a reason for hiding this comment

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

This works, but I would be more comfortable just converting this to clinic.

@devdanzin
Copy link
Contributor Author

This works, but I would be more comfortable just converting this to clinic.

I'm not too attached to this PR, but converting to AC is beyond my skills. If you or someone else wants to propose another PR with that approach, I'll gladly close this one.

@JelleZijlstra
Copy link
Member

Converting to AC can wait for a later PR, and probably should be done only in main.

@JelleZijlstra JelleZijlstra merged commit 80eec52 into python:main Oct 27, 2024
40 checks passed
@miss-islington-app
Copy link

Thanks @devdanzin for the PR, and @JelleZijlstra for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 27, 2024
@devdanzin @picnixz
@JelleZijlstra @miss-islington
pythongh-126018: Avoid aborting due to unnecessary assert in `sys.aud…
e0eeb70 
…it` (pythonGH-126020)

(cherry picked from commit 80eec52)

Co-authored-by: devdanzin <[email protected]>
Co-authored-by: Bénédikt Tran <[email protected]>
Co-authored-by: Jelle Zijlstra <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Oct 27, 2024

GH-126042 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Oct 27, 2024
JelleZijlstra added a commit that referenced this pull request Oct 27, 2024
@miss-islington @devdanzin
@picnixz @JelleZijlstra
[3.13] gh-126018: Avoid aborting due to unnecessary assert in `sys.au…
e4157f0 
…dit` (GH-126020) (#126042)



(cherry picked from commit 80eec52)

Co-authored-by: devdanzin <[email protected]>
Co-authored-by: Bénédikt Tran <[email protected]>
Co-authored-by: Jelle Zijlstra <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@picnixz picnixz picnixz left review comments

@ZeroIntensity ZeroIntensity ZeroIntensity left review comments

@Eclips4 Eclips4 Eclips4 left review comments

@JelleZijlstra JelleZijlstra JelleZijlstra approved these changes

@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@devdanzin @picnixz @JelleZijlstra @ZeroIntensity @Eclips4