gh-126238: Fix possible null pointer dereference of freevars in _PyCompile_LookupArg by federicovalenso · Pull Request #126239 · python/cpython

federicovalenso · 2024-10-31T13:12:05Z

Issue: Possible null pointer dereference of freevars in _PyCompile_LookupArg #126238

bedevere-app · 2024-10-31T13:12:09Z

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.

iritkatriel

Can we cover this with a test?

ZeroIntensity · 2024-10-31T13:32:18Z

I'm not exactly sure how to trigger it, it was found via a static analyzer.

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

sobolevn · 2024-10-31T13:37:07Z

@@ -0,0 +1 @@
+Fix a possible crash internally when compiling.


I suggest to drop this NEWS entry.

I concur (that's what I did for #126241 because it's a bit hard to phrase it properly and meaningfully).

drop this NEWS entry

Done )

Why? This seems user-facing to me, isn't it?

@sobolevn, @picnixz , what's the bottom line? Is news entry required or not?

The bottom line in general is whether this kind of bug can be triggered easily using public interface. Unless you have a reproducer saying that with X and Y you can do Z, or unless the interface is publicly documented and known to the outside world, a NEWS entry would be fine. But here, we have neither a test nor is _PyCompile_LookupArg something that is exposed to the world.

As an end-user, reading "Fix a possible crash internally when compiling." gives me no information at all except that there was a bug I wasn't aware of (and that it was not always triggerable). I don't know how to make the crash happen, nor do I know what was crashing.

iritkatriel · 2024-10-31T15:54:38Z

@@ -901,7 +901,7 @@ _PyCompile_LookupArg(compiler *c, PyCodeObject *co, PyObject *name)
            c->u->u_metadata.u_name,
            co->co_name,
            freevars);


Does PyErr_Format work for null?

Yes, %R formats it as "<NULL>"

ruidazeng · 2024-11-05T16:11:49Z

I'm not exactly sure how to trigger it, it was found via a static analyzer.

Can you be more specific? I cannot reproduce this internal crash.

ZeroIntensity · 2024-11-05T16:21:37Z

Can you be more specific? I cannot reproduce this internal crash.

See GH-126238. Looking at the source, this can only fail in some rare cases when a memory allocation fails, but it's pretty clearly wrong if that was the case so we might as well fix it.

ZeroIntensity

I'm happy with it.

sobolevn

Thank you, congrats on your first merged PR :)

miss-islington-app · 2024-11-05T17:34:37Z

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

miss-islington-app · 2024-11-05T17:34:41Z

Sorry, @federicovalenso and @sobolevn, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 8525c9375f25e6ec0c0b5dfcab464703f6e78082 3.13

miss-islington-app · 2024-11-05T17:34:45Z

Sorry, @federicovalenso and @sobolevn, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 8525c9375f25e6ec0c0b5dfcab464703f6e78082 3.12

sobolevn · 2024-11-05T17:38:24Z

@federicovalenso do you want to create manual backports? See:

gh-126238: Fix possible null pointer dereference of freevars in _PyCompile_LookupArg #126239 (comment)
gh-126238: Fix possible null pointer dereference of freevars in _PyCompile_LookupArg #126239 (comment)

Or I can do that if you ask me to :)

ZeroIntensity · 2024-11-05T17:41:06Z

See the devguide if you want to know how to do that.

federicovalenso · 2024-11-06T06:40:42Z

@sobolevn , @ZeroIntensity , I'll try to do that)

…vars in _PyCompile_LookupArg (pythonGH-126239) * Replace Py_DECREF by Py_XDECREF (cherry picked from commit 8525c93) Co-authored-by: Valery Fedorenko <federicovalenso@gmail.com> Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com> Co-authored-by: Peter Bierma <zintensitydev@gmail.com>

federicovalenso · 2024-11-06T07:36:45Z

@sobolevn , could you please look at #126474, #126475 )

…vars in _PyCompile_LookupArg (pythonGH-126239) * Replace Py_DECREF by Py_XDECREF (cherry picked from commit 8525c93) Co-authored-by: Valery Fedorenko <federicovalenso@gmail.com> Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com> Co-authored-by: Peter Bierma <zintensitydev@gmail.com>

…e_LookupArg (gh-126238) (#126474) [3.12] gh-126238: Fix possible null pointer dereference of freevars in _PyCompile_LookupArg (GH-126239) * Replace Py_DECREF by Py_XDECREF (cherry picked from commit 8525c93) Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com> Co-authored-by: Peter Bierma <zintensitydev@gmail.com>

…e_LookupArg (gh-126238) (#126475) [3.13] gh-126238: Fix possible null pointer dereference of freevars in _PyCompile_LookupArg (GH-126239) * Replace Py_DECREF by Py_XDECREF (cherry picked from commit 8525c93) Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com> Co-authored-by: Peter Bierma <zintensitydev@gmail.com>

… _PyCompile_LookupArg (python#126239) * Replace Py_DECREF by Py_XDECREF Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com> Co-authored-by: Peter Bierma <zintensitydev@gmail.com>

Replace Py_DECREF by Py_XDECREF

f8940e4

bedevere-app Bot mentioned this pull request Oct 31, 2024

Possible null pointer dereference of freevars in _PyCompile_LookupArg #126238

Closed

📜🤖 Added by blurb_it.

10dcbe0

federicovalenso marked this pull request as ready for review October 31, 2024 13:15

federicovalenso requested review from iritkatriel and markshannon as code owners October 31, 2024 13:15

bedevere-app Bot added the awaiting review label Oct 31, 2024

ZeroIntensity reviewed Oct 31, 2024

View reviewed changes

Comment thread Misc/NEWS.d/next/Security/2024-10-31-13-14-27.gh-issue-126238.CZqaon.rst Outdated

iritkatriel reviewed Oct 31, 2024

View reviewed changes

Update Misc/NEWS.d/next/Security/2024-10-31-13-14-27.gh-issue-126238.…

2c8bc0f

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

sobolevn reviewed Oct 31, 2024

View reviewed changes

picnixz added needs backport to 3.12 only security fixes needs backport to 3.13 bugs and security fixes skip news labels Oct 31, 2024

iritkatriel reviewed Oct 31, 2024

View reviewed changes

Remove news entry due to "skip news" label

547eca1

ruidazeng approved these changes Nov 5, 2024

View reviewed changes

bedevere-app Bot added awaiting core review and removed awaiting review labels Nov 5, 2024

ZeroIntensity approved these changes Nov 5, 2024

View reviewed changes

sobolevn approved these changes Nov 5, 2024

View reviewed changes

bedevere-app Bot added awaiting merge and removed awaiting core review labels Nov 5, 2024

sobolevn merged commit 8525c93 into python:main Nov 5, 2024

bedevere-app Bot removed the awaiting merge label Nov 5, 2024

miss-islington-app Bot assigned sobolevn Nov 5, 2024

federicovalenso mentioned this pull request Nov 6, 2024

[3.13] Fix possible null pointer dereference of freevars in _PyCompile_LookupArg (gh-126238) #126475

Merged

ambv removed needs backport to 3.12 only security fixes needs backport to 3.13 bugs and security fixes labels Nov 12, 2024

		@@ -0,0 +1 @@
		Fix a possible crash internally when compiling.

Uh oh!

Conversation

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

Uh oh!

Uh oh!

bedevere-app Bot commented Oct 31, 2024

Uh oh!

Uh oh!

iritkatriel left a comment

Choose a reason for hiding this comment

Uh oh!

ZeroIntensity commented Oct 31, 2024

Uh oh!

sobolevn Oct 31, 2024

Choose a reason for hiding this comment

Uh oh!

picnixz Oct 31, 2024

Choose a reason for hiding this comment

Uh oh!

federicovalenso Nov 1, 2024

Choose a reason for hiding this comment

Uh oh!

ZeroIntensity Nov 1, 2024

Choose a reason for hiding this comment

Uh oh!

federicovalenso Nov 1, 2024

Choose a reason for hiding this comment

Uh oh!

picnixz Nov 1, 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!

iritkatriel Oct 31, 2024

Choose a reason for hiding this comment

Uh oh!

sobolevn Oct 31, 2024

Choose a reason for hiding this comment

Uh oh!

ruidazeng commented Nov 5, 2024

Uh oh!

ZeroIntensity commented Nov 5, 2024

Uh oh!

ZeroIntensity left a comment

Choose a reason for hiding this comment

Uh oh!

sobolevn left a comment

Choose a reason for hiding this comment

Uh oh!

miss-islington-app Bot commented Nov 5, 2024

Uh oh!

miss-islington-app Bot commented Nov 5, 2024

Uh oh!

miss-islington-app Bot commented Nov 5, 2024

Uh oh!

sobolevn commented Nov 5, 2024

Uh oh!

ZeroIntensity commented Nov 5, 2024

Uh oh!

federicovalenso commented Nov 6, 2024

Uh oh!

federicovalenso commented Nov 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

federicovalenso commented Oct 31, 2024 •

edited by bedevere-app Bot

Loading

picnixz Nov 1, 2024 •

edited

Loading

federicovalenso commented Nov 6, 2024 •

edited

Loading