Movatterモバイル変換

bedevere-appbot mentioned this pull request

Potential null pointer dereference inPySys_AddWarnOptionUnicode#126108

Closed

📜🤖 Added by blurb_it.

2ca65fe

picnixz reviewed

Misc/NEWS.d/next/Security/2024-10-29-09-15-10.gh-issue-126108.eTIjHY.rst OutdatedShow resolvedHide resolved

Python/sysmodule.cShow resolvedHide resolved

federicovalensoand others added6 commits

October 29, 2024 16:01

Update Misc/NEWS.d/next/Security/2024-10-29-09-15-10.gh-issue-126108.…

0fb17bc

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

add assert

6c22e44

Merge branch 'bug/potential-null-pointer-dereference-in-PySys_AddWarn…

6cd01fd

…OptionUnicode' of github.com:federicovalenso/cpython into bug/potential-null-pointer-dereference-in-PySys_AddWarnOptionUnicode

fix spaces

a969026

fix unbalanced literal markup

1667599

fix backticks

9323183

tomasr8 reviewed

Python/sysmodule.c OutdatedShow resolvedHide resolved

Update Python/sysmodule.c

5cd8e30

Co-authored-by: Tomas R. <tomas.roun8@gmail.com>

Copy link

Member

ZeroIntensity commentedOct 29, 2024

Typically, the convention in the C API is to fully disallow calling without the GIL, via_Py_EnsureTstateNotNULL. But I'm happy with this if there's a real use case for callingPySys_AddWarnOptionUnicode without it.

picnixz added needs backport to 3.12

only security fixes

needs backport to 3.13bugs and security fixes labels

Merge branch 'main' into bug/potential-null-pointer-dereference-in-Py…

f234711

…Sys_AddWarnOptionUnicode

picnixz reviewed

Oct 30, 2024

Copy link

Member

picnixz left a comment

Choose a reason for hiding this comment

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

LGTM (conditioned to a question) cc@sobolevn

Python/sysmodule.c

		if (tstate) {
		_PyErr_Clear(tstate);
		}
		_PyErr_Clear(tstate);

Copy link

Member

picnixzOct 30, 2024

Choose a reason for hiding this comment

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

I assume that there's no way to maketstate NULL in_PySys_AddWarnOptionWithError.

Copy link

Member

sobolevnOct 30, 2024

Choose a reason for hiding this comment

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

I agree,assert is the way to go here. We should not call these functions in places wheretstate can beNULL (while interpreter startup / shutdown).

Copy link

Member

picnixzOct 30, 2024

Choose a reason for hiding this comment

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

Would it actually make sense to have_PyErr_Clear do this automatically?

Copy link

Member

ZeroIntensityOct 30, 2024

Choose a reason for hiding this comment

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

Ok, in that case, I would likePySys_AddWarnOptionUnicode to fail with a fatal error if called without the GIL. No-oping because of a null thread state isn't common at all in the C API.

Copy link

ContributorAuthor

federicovalensoOct 31, 2024

Choose a reason for hiding this comment

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

Should I do some other changes or leave as is?

Copy link

Member

picnixzOct 31, 2024•
edited
Loading

Choose a reason for hiding this comment

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

Should we also do it in_PySys_AddWarnOptionWithError? WDYT@ZeroIntensity?

Copy link

Member

ZeroIntensityOct 31, 2024

Choose a reason for hiding this comment

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

I thinkassert(tstate != NULL) is fine for the private API.

ZeroIntensity requested changes

Oct 30, 2024

Copy link

Member

ZeroIntensity left a comment•
edited
Loading

Choose a reason for hiding this comment

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

Two main changes here (my suggestions address both):

Functions in the C API shouldn't no-op if called without the GIL. Instead, we typically use_Py_EnsureTstateNotNULL to send a fatal error.
This function can clear the error state, so any prior exceptions are lost. It's a good idea to make sure that callers aren't doing that on debug builds.

Python/sysmodule.c OutdatedShow resolvedHide resolved

Python/sysmodule.cShow resolvedHide resolved

bedevere-appbot added awaiting core review and removed awaiting review labels

Oct 30, 2024

federicovalensoand others added2 commits

October 31, 2024 09:36

Apply suggession for Python/sysmodule.c

67b9de2

Co-authored-by: Peter Bierma <zintensitydev@gmail.com>

Apply suggession for Python/sysmodule.c

7d0754f

Co-authored-by: Peter Bierma <zintensitydev@gmail.com>

sobolevn approved these changes

Copy link

Member

sobolevn left a comment

Choose a reason for hiding this comment

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

LGTM

bedevere-appbot added awaiting merge and removed awaiting core review labels

picnixz requested a review fromZeroIntensity

October 31, 2024 12:05

ZeroIntensity approved these changes

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.

Thank you!

picnixz approved these changes

Copy link

ContributorAuthor

federicovalenso commentedNov 15, 2024

@ericsnowcurrently, please review the code :)

ericsnowcurrently removed their request for review

November 19, 2024 20:13

Copy link

ContributorAuthor

federicovalenso commentedDec 3, 2024

@picnixz , can we merge this PR? Changes approved by all reviewers.

kumaraditya303 reviewed

Dec 3, 2024

Update sysmodule.c/issue -#126108#128749

Python/sysmodule.cShow resolvedHide resolved

picnixz mentioned this pull request

Jan 12, 2025

Closed

kumaraditya303 merged commitfad36bf intopython:main

39 checks passed

Copy link

miss-islington-appbot commentedJan 31, 2025

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

bedevere-appbot removed the awaiting merge label

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

pythongh-126108: Fix potential null pointer dereference in `PySys_Add…

1e36f67

…WarnOptionUnicode` (pythonGH-126118)(cherry picked from commitfad36bf)Co-authored-by: Valery Fedorenko <federicovalenso@gmail.com>

Copy link

bedevere-appbot commentedJan 31, 2025

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

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

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

pythongh-126108: Fix potential null pointer dereference in `PySys_Add…

99704de

…WarnOptionUnicode` (pythonGH-126118)(cherry picked from commitfad36bf)Co-authored-by: Valery Fedorenko <federicovalenso@gmail.com>

Copy link

bedevere-appbot commentedJan 31, 2025

GH-129522 is a backport of this pull request to the3.12 branch.

bedevere-appbot removed the needs backport to 3.12only security fixes label

Copy link

miss-islington-appbot commentedJan 31, 2025

Sorry@federicovalenso and@kumaraditya303, I had trouble completing the backport.
Please retry by removing and re-adding the "needs backport to 3.12" label.
Please backport backport usingcherry_picker on the command line.

cherry_picker fad36bf38248130bc48b81a5e7c31a7649a6456e 3.12

miss-islington-appbot assignedkumaraditya303

kumaraditya303 pushed a commit that referenced this pull request

[3.12]gh-126108: Fix potential null pointer dereference in `PySys_Ad…

40d7f74

…dWarnOptionUnicode` (GH-126118) (#129522)gh-126108: Fix potential null pointer dereference in `PySys_AddWarnOptionUnicode` (GH-126118)(cherry picked from commitfad36bf)Co-authored-by: Valery Fedorenko <federicovalenso@gmail.com>

kumaraditya303 pushed a commit that referenced this pull request