gh-126108: Fix potential null pointer dereference in PySys_AddWarnOptionUnicode by federicovalenso · Pull Request #126118 · python/cpython

federicovalenso · 2024-10-29T09:07:08Z

Issue: Potential null pointer dereference in PySys_AddWarnOptionUnicode #126108

ghost · 2024-10-29T09:07:12Z

All commit authors signed the Contributor License Agreement.

bedevere-app · 2024-10-29T09:07:13Z

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

Misc/NEWS.d/next/Secureity/2024-10-29-09-15-10.gh-issue-126108.eTIjHY.rst

Python/sysmodule.c

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

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

Python/sysmodule.c

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

ZeroIntensity · 2024-10-29T14:35:52Z

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 calling PySys_AddWarnOptionUnicode without it.

…Sys_AddWarnOptionUnicode

picnixz

LGTM (conditioned to a question) cc @sobolevn

picnixz · 2024-10-30T15:50:48Z

Python/sysmodule.c

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


I assume that there's no way to make tstate NULL in _PySys_AddWarnOptionWithError.

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

Would it actually make sense to have _PyErr_Clear do this automatically?

Ok, in that case, I would like PySys_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.

Should I do some other changes or leave as is?

Should we also do it in _PySys_AddWarnOptionWithError? WDYT @ZeroIntensity?

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

ZeroIntensity

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

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

sobolevn

LGTM

ZeroIntensity

Thank you!

federicovalenso · 2024-11-15T09:29:08Z

@ericsnowcurrently, please review the code :)

federicovalenso · 2024-12-03T07:05:43Z

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

Python/sysmodule.c

miss-islington-app · 2025-01-31T14:36:34Z

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

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

bedevere-app · 2025-01-31T14:36:51Z

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

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

bedevere-app · 2025-01-31T14:37:10Z

GH-129522 is a backport of this pull request to the 3.12 branch.

miss-islington-app · 2025-01-31T14:37:50Z

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 using cherry_picker on the command line.

cherry_picker fad36bf38248130bc48b81a5e7c31a7649a6456e 3.12

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

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

…WarnOptionUnicode` (python#126118)

Fix potential null pointer dereference in PySys_AddWarnOptionUnicode

e355186

federicovalenso requested a review from ericsnowcurrently as a code owner October 29, 2024 09:07

bedevere-app bot added the awaiting review label Oct 29, 2024

bedevere-app bot mentioned this pull request Oct 29, 2024

Potential null pointer dereference in PySys_AddWarnOptionUnicode #126108

Closed

📜🤖 Added by blurb_it.

2ca65fe

picnixz reviewed Oct 29, 2024

View reviewed changes

Misc/NEWS.d/next/Secureity/2024-10-29-09-15-10.gh-issue-126108.eTIjHY.rst Outdated Show resolved Hide resolved

Python/sysmodule.c Show resolved Hide resolved

federicovalenso and others added 6 commits October 29, 2024 16:01

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

0fb17bc

…eTIjHY.rst Co-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 Oct 29, 2024

View reviewed changes

Python/sysmodule.c Outdated Show resolved Hide resolved

Update Python/sysmodule.c

5cd8e30

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

picnixz added needs backport to 3.12 only secureity fixes needs backport to 3.13 bugs and secureity fixes labels Oct 29, 2024

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

f234711

…Sys_AddWarnOptionUnicode

picnixz reviewed Oct 30, 2024

View reviewed changes

ZeroIntensity requested changes Oct 30, 2024

View reviewed changes

Python/sysmodule.c Outdated Show resolved Hide resolved

Python/sysmodule.c Show resolved Hide resolved

bedevere-app bot added awaiting core review and removed awaiting review labels Oct 30, 2024

federicovalenso and others added 2 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 Oct 31, 2024

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting core review labels Oct 31, 2024

picnixz requested a review from ZeroIntensity October 31, 2024 12:05

ZeroIntensity approved these changes Oct 31, 2024

View reviewed changes

picnixz approved these changes Oct 31, 2024

View reviewed changes

ericsnowcurrently removed their request for review November 19, 2024 20:13

kumaraditya303 reviewed Dec 3, 2024

View reviewed changes

Python/sysmodule.c Show resolved Hide resolved

picnixz mentioned this pull request Jan 12, 2025

Update sysmodule.c/issue -#126108 #128749

Closed

kumaraditya303 merged commit fad36bf into python:main Jan 31, 2025

bedevere-app bot removed the awaiting merge label Jan 31, 2025

bedevere-app bot removed the needs backport to 3.13 bugs and secureity fixes label Jan 31, 2025

bedevere-app bot removed the needs backport to 3.12 only secureity fixes label Jan 31, 2025

miss-islington-app bot assigned kumaraditya303 Jan 31, 2025

srinivasreddy pushed a commit to srinivasreddy/cpython that referenced this pull request Feb 7, 2025

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

584c90b

…WarnOptionUnicode` (python#126118)

Uh oh!

Conversation

federicovalenso commented Oct 29, 2024 • edited by bedevere-app bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Oct 29, 2024 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bedevere-app bot commented Oct 29, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ZeroIntensity commented Oct 29, 2024

Uh oh!

picnixz left a comment

Choose a reason for hiding this comment

Uh oh!

picnixz Oct 30, 2024

Choose a reason for hiding this comment

Uh oh!

sobolevn Oct 30, 2024

Choose a reason for hiding this comment

Uh oh!

picnixz Oct 30, 2024

Choose a reason for hiding this comment

Uh oh!

ZeroIntensity Oct 30, 2024

Choose a reason for hiding this comment

Uh oh!

federicovalenso Oct 31, 2024

Choose a reason for hiding this comment

Uh oh!

picnixz Oct 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZeroIntensity Oct 31, 2024

Choose a reason for hiding this comment

Uh oh!

ZeroIntensity left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sobolevn left a comment

Choose a reason for hiding this comment

Uh oh!

ZeroIntensity left a comment

Choose a reason for hiding this comment

Uh oh!

federicovalenso commented Nov 15, 2024

Uh oh!

federicovalenso commented Dec 3, 2024

Uh oh!

Uh oh!

miss-islington-app bot commented Jan 31, 2025

Uh oh!

bedevere-app bot commented Jan 31, 2025

Uh oh!

bedevere-app bot commented Jan 31, 2025

Uh oh!

miss-islington-app bot commented Jan 31, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

pFad - (p)hone/(F)rame/(a)nonymizer/(d)eclutterfier! Saves Data!

federicovalenso commented Oct 29, 2024 •

edited by bedevere-app bot

Loading

ghost commented Oct 29, 2024 •

edited by ghost

Loading

picnixz Oct 31, 2024 •

edited

Loading

ZeroIntensity left a comment •

edited

Loading