Movatterモバイル変換

bedevere-appbot mentioned this pull request

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

Closed

picnixz added needs backport to 3.12

only security fixes

needs backport to 3.13bugs and security fixes labels

Copy link

Member

picnixz commentedOct 26, 2024

This likely requires some NEWS entry, for instance:

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

picnixz reviewed

Lib/test/audit-tests.pyShow resolvedHide resolved

Lib/test/test_audit.py OutdatedShow resolvedHide resolved

Eclips4 reviewed

Lib/test/test_audit.py OutdatedShow resolvedHide resolved

📜🤖 Added by blurb_it.

b775f5d

picnixz removed the needs backport to 3.12only security fixes label

cpython/Python/sysmodule.c

Copy link

Member

picnixz commentedOct 26, 2024

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

Lines 499 to 514 in67b2701

	if (!should_audit(tstate->interp)) {
	Py_RETURN_NONE;
	}

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

devdanzinand others added2 commits

October 26, 2024 20:53

Apply suggestions from code review

6234c5d

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>

Apply suggestions from code review by@Eclips4.

a6edd2c

Copy link

MemberAuthor

devdanzin commentedOct 27, 2024

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

JelleZijlstra reviewed

Lib/test/audit-tests.pyShow resolvedHide resolved

Misc/NEWS.d/next/Core_and_Builtins/2024-10-26-23-50-03.gh-issue-126018.Hq-qcM.rst OutdatedShow resolvedHide resolved

ZeroIntensity reviewed

Copy link

Member

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.

devdanzinand others added2 commits

October 27, 2024 06:47

Fail test_assert_unicode if no exception is raised by sys.audit(9) (t…

57cc87c

…hanks@JelleZijlstra).

Apply review suggestion for better description of issue in NEWS.

c42c4d6

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>

Copy link

MemberAuthor

devdanzin commentedOct 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 approved these changes

bedevere-appbot added awaiting merge and removed awaiting review labels

Copy link

Member

JelleZijlstra commentedOct 27, 2024

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

JelleZijlstra merged commit80eec52 intopython:main

36 checks passed

Copy link

miss-islington-appbot commentedOct 27, 2024

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

bedevere-appbot removed the awaiting merge label

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request

pythongh-126018: Avoid aborting due to unnecessary assert in `sys.aud…

e0eeb70

…it` (pythonGH-126020)(cherry picked from commit80eec52)Co-authored-by: devdanzin <74280297+devdanzin@users.noreply.github.com>Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>

Copy link

bedevere-appbot commentedOct 27, 2024

GH-126042 is a backport of this pull request to the3.13 branch.

bedevere-appbot removed the needs backport to 3.13bugs and security fixes label

JelleZijlstra added a commit that referenced this pull request