GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-126018: Avoid aborting due to unnecessary assert in `sys.audit` by devdanzin · Pull Request #126020 · python/cpython

Conversation

@devdanzin

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

  • Issue: sys.audit(0) aborts due to an assertion in debug build #126018

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
devdanzin added 2 commits October 26, 2024 19:31
@devdanzin
Remove an assert that sys.audit's args[0] passes PyUnicode_Check.
ba8ac4d
@devdanzin
Test that passing a non-string to sys.audit doesn't trigger an assert…
620f682 
… in debug builds.
@devdanzin
devdanzin requested a review from ericsnowcurrently as a code owner October 26, 2024 23:43
@bedevere-app bedevere-app Bot added the awaiting review label Oct 26, 2024
@bedevere-app bedevere-app Bot mentioned this pull request Oct 26, 2024
sys.audit(0) aborts due to an assertion in debug build #126018
Closed
@picnixz picnixz added needs backport to 3.12 only security fixes needs backport to 3.13 only security fixes labels Oct 26, 2024
@picnixz

picnixz commented Oct 26, 2024

Copy link
Copy Markdown
Member

This likely requires some NEWS entry, for instance:

Fix a crash in :func:`sys.audit` when passing a non-string argument.
Eclips4 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
picnixz
picnixz reviewed Oct 26, 2024
View reviewed changes
Comment thread Lib/test/audit-tests.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_audit.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Eclips4
Eclips4 reviewed Oct 26, 2024
View reviewed changes
Comment thread Lib/test/test_audit.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@blurb-it
📜🤖 Added by blurb_it.
b775f5d
@picnixz picnixz removed the needs backport to 3.12 only security fixes label Oct 26, 2024
@picnixz

picnixz commented Oct 26, 2024

Copy link
Copy Markdown
Member

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;
}

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
devdanzin and others added 2 commits October 26, 2024 20:53
@devdanzin @picnixz
Apply suggestions from code review
6234c5d 
Co-authored-by: Bénédikt Tran <[email protected]>
@devdanzin
Apply suggestions from code review by @Eclips4.
a6edd2c
@devdanzin

devdanzin commented Oct 27, 2024

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
JelleZijlstra
JelleZijlstra reviewed Oct 27, 2024
View reviewed changes
Comment thread Lib/test/audit-tests.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Misc/NEWS.d/next/Core_and_Builtins/2024-10-26-23-50-03.gh-issue-126018.Hq-qcM.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
ZeroIntensity
ZeroIntensity reviewed Oct 27, 2024
View reviewed changes

@ZeroIntensity ZeroIntensity left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
devdanzin and others added 2 commits October 27, 2024 06:47
@devdanzin
Fail test_assert_unicode if no exception is raised by sys.audit(9) (t…
57cc87c 
…hanks @JelleZijlstra).
@devdanzin @JelleZijlstra
Apply review suggestion for better description of issue in NEWS.
c42c4d6 
Co-authored-by: Jelle Zijlstra <[email protected]>
@devdanzin

devdanzin commented Oct 27, 2024

Copy link
Copy Markdown
Member 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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
JelleZijlstra
JelleZijlstra approved these changes Oct 27, 2024
View reviewed changes
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels Oct 27, 2024
@JelleZijlstra

JelleZijlstra commented Oct 27, 2024

Copy link
Copy Markdown
Member

Converting to AC can wait for a later PR, and probably should be done only in main.
ZeroIntensity and AlexWaygood reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@JelleZijlstra
JelleZijlstra merged commit 80eec52 into python:main Oct 27, 2024
@miss-islington-app

miss-islington-app Bot commented Oct 27, 2024

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot removed the awaiting merge label Oct 27, 2024
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

bedevere-app Bot commented Oct 27, 2024

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot removed the needs backport to 3.13 only 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

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@devdanzin @picnixz @JelleZijlstra @ZeroIntensity @Eclips4
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.