Skip to content

Comments

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

Merged
JelleZijlstra merged 7 commits intopython:mainfrom
devdanzin:remove_sysaudit_assert
Oct 27, 2024
Merged

gh-126018: Avoid aborting due to unnecessary assert in sys.audit#126020
JelleZijlstra merged 7 commits intopython:mainfrom
devdanzin:remove_sysaudit_assert

Conversation

@devdanzin
Copy link
Member

@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 only security fixes needs backport to 3.13 bugs and security fixes labels Oct 26, 2024
@picnixz
Copy link
Member

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
Eclips4
Eclips4 reviewed Oct 26, 2024
View reviewed changes
@picnixz picnixz removed the needs backport to 3.12 only security fixes label Oct 26, 2024
@picnixz
Copy link
Member

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
Member Author

devdanzin commented Oct 27, 2024

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

JelleZijlstra
JelleZijlstra reviewed Oct 27, 2024
View reviewed changes
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
Member Author

devdanzin commented Oct 27, 2024

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

JelleZijlstra commented Oct 27, 2024

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
@miss-islington-app
Copy link

miss-islington-app bot commented Oct 27, 2024

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]>
picnixz added a commit to picnixz/cpython that referenced this pull request Dec 8, 2024
@devdanzin @picnixz @JelleZijlstra
pythongh-126018: Avoid aborting due to unnecessary assert in `sys.aud…
353994f 
…it` (python#126020)


Co-authored-by: Bénédikt Tran <[email protected]>
Co-authored-by: Jelle Zijlstra <[email protected]>
ebonnal pushed a commit to ebonnal/cpython that referenced this pull request Jan 12, 2025
@devdanzin @picnixz
@JelleZijlstra @ebonnal
pythongh-126018: Avoid aborting due to unnecessary assert in `sys.aud…
3a9bc3f 
…it` (python#126020)


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