gh-128396: Fix a crash when inline comprehension has the same local variable as the outside scope #130235

gaogaotiantian · 2025-02-17T18:54:53Z

With PEP 709 + PEP 667, we have a rather special case.

When we have code like

def f():
    lambda: k
    k = 1
    [locals() for k in [0]]
f()

In f, the code object will consider k a free var (so in C it's a CellType) because it is used in closure (lambda: k). However for the list comprehension [locals() for k in [0]], k will be considered as a pure fast variable which is defined by PEP 709.

I think this is correct because PEP 709 says that the inlined list comprehension should behave the same as we created a new function. If we created a new function

def g()
    lst = []
    for k in [0]:
        lst.append(k)
    return lst

k would be a pure fast, not a cell variable.

However, in reality, this piece of code does execute in the same frame, and locals() tries to read the kind data of the variable k before accessing the value - and it is a CO_FAST_FREE - so locals() will thus try to get the actual value from the cell that does not exist - which would eventually cause a crash (with --py-debug, an assertion failure).

We can of course do something in the runtime, for example setting some special flag on k or secretly change the kind of the code object during execution, but I feel like that's not necessary.

My fix here is simply check if the cell exists, and if not, just get the value, which is how PyFrame_GetVar does it. Unless people think this run time behavior should be improved, I think this fix is straightforward and simple.

Issue: PEP 667 + PEP 709 segfaults from accessing closure variables bound by any inlined comprehensions #128396

as outside scope

JelleZijlstra · 2025-02-17T19:34:51Z

Objects/frameobject.c

@@ -45,8 +45,15 @@ framelocalsproxy_getval(_PyInterpreterFrame *frame, PyCodeObject *co, int i)
    if (kind == CO_FAST_FREE || kind & CO_FAST_CELL) {
        // The cell was set when the frame was created from
        // the function's closure.
-        assert(PyCell_Check(value));
-        cell = value;
+        // GH-128396: With PEP 709, it's possible to have a fast variable in


Is it possible this will do the wrong thing if the value comes from within the list comprehension, but happens to hold a cell object?

Something like this:

def test_closure_with_inline_comprehension(self): lambda: k k = 1 lst = [locals() for k in [types.CellType()]] self.assertEqual(lst[0]['k'], 0)

Yes, that's possible. Even though we do not recommend using CellType directly, but that's always a possibility. The good news is that this PR does not make it worse - it does not work properly without this PR either. Also the existing C API PyFrame_GetVar probably suffers from this as well.

To solve this, we need a reliable way to figure out if we are in an inlined comprehension - I think that's a loophole PEP 709 left us.

The fundamental reason of this issue is the inconsistency between the variable kind that's static in the code object and the actual kind that is dynamic with inline comprehension. PEP 667 is one way to expose that but anything that relies on the kind of a variable in an inline comprehension will expose the issue.

@carljm , when PEP 709 was implemented, did the idea of creating a new iteration variable on the code object with the same name came up in some way? So

def f(): x = 1 [x for x in [0]]

actually compiles to a code object with co_varnames = ['x', 'x']

I think I ruled this out on the assumption that there is too much code (possibly even in CPython) relying on not having two different variable indices with the same varname in a code object, and I didn't think it was necessary. But it's possible I ruled it out too hastily; it would certainly simplify comprehension inlining a lot to do this.

bswck · 2025-02-17T21:11:33Z

Can't believe that was so trivial 😱

gaogaotiantian · 2025-02-17T21:20:54Z

The fix might be trivial, but we still have unsolved issues behind the actual cause :)

gaogaotiantian · 2025-02-18T16:58:06Z

It seems to me that we should merge and backport this even if this is not the perfect solution, because it does not make things worse. The example given by @JelleZijlstra does not work before this PR either, and it definitely takes much more effort to make it work properly. Meanwhile, this PR solves a segfault case with minimum impact on other code. We should continue to discuss about the better solution about how PEP 709 should work, but it should not block this PR. All the other CPython/C-extension code will suffer from the same issue as well, it's not a frame proxy specific issue.

bswck · 2025-02-18T17:01:17Z

+1, shoutout for such a quick fix!

carljm

This looks like a reasonable stopgap fix to me. Thanks @gaogaotiantian for the investigation and fix.

I agree we need more consideration of a more thorough fix; unfortunately I am very short on time these days to devote to this area, so I'm not sure how actively I'll be able to contribute to that in the next few months.

miss-islington-app · 2025-02-19T17:11:21Z

Thanks @gaogaotiantian for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

miss-islington-app · 2025-02-19T17:11:43Z

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

cherry_picker ccf17323c218a2fdcf7f4845d3eaa74ebddefa44 3.13

bedevere-app · 2025-02-19T17:16:11Z

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

… same local variable as the outside scope (pythonGH-130235) (cherry picked from commit ccf1732) Co-authored-by: Tian Gao <[email protected]>

bedevere-bot · 2025-02-19T17:25:42Z

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot AMD64 FreeBSD14 3.x has failed when building commit ccf1732.

What do you need to do:

Don't panic.
Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
Go to the page of the buildbot that failed (https://buildbot.python.org/#/builders/1232/builds/4715) and take a look at the build logs.
Check if the failure is related to this commit (ccf1732) or if it is a false positive.
If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/#/builders/1232/builds/4715

Failed tests:

test_interpreters
test.test_concurrent_futures.test_interpreter_pool

Summary of the results of the build (if available):

==

Click to see traceback logs

Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.opsec-fbsd14/build/Lib/threading.py", line 1054, in _bootstrap_inner
    self.run()
    ~~~~~~~~^^
  File "/home/buildbot/buildarea/3.x.opsec-fbsd14/build/Lib/threading.py", line 996, in run
    self._target(*self._args, **self._kwargs)
    ~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.opsec-fbsd14/build/Lib/test/test_interpreters/test_stress.py", line 30, in task
    interp = interpreters.create()
  File "/home/buildbot/buildarea/3.x.opsec-fbsd14/build/Lib/test/support/interpreters/__init__.py", line 76, in create
    id = _interpreters.create(reqrefs=True)
interpreters.InterpreterError: interpreter creation failed
k


Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.opsec-fbsd14/build/Lib/threading.py", line 1054, in _bootstrap_inner
    self.run()
    ~~~~~~~~^^
  File "/home/buildbot/buildarea/3.x.opsec-fbsd14/build/Lib/threading.py", line 996, in run
    self._target(*self._args, **self._kwargs)
    ~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.opsec-fbsd14/build/Lib/test/test_interpreters/test_stress.py", line 47, in run
    interp = interpreters.create()
  File "/home/buildbot/buildarea/3.x.opsec-fbsd14/build/Lib/test/support/interpreters/__init__.py", line 76, in create
    id = _interpreters.create(reqrefs=True)
interpreters.InterpreterError: interpreter creation failed
k


Traceback (most recent call last):
  File "<frozen getpath>", line 358, in <module>
ValueError: embedded null byte
Warning -- Uncaught thread exception: InterpreterError
Exception in thread Thread-145 (task):
RuntimeError: error evaluating path

#130311) [3.13] gh-128396: Fix a crash when inline comprehension has the same local variable as the outside scope (GH-130235) (cherry picked from commit ccf1732)

gaogaotiantian added 2 commits February 17, 2025 13:42

Fix a crash when inline comprehension has the same local variable

49ffaea

as outside scope

Add regression test

5b6afbb

gaogaotiantian requested a review from carljm February 17, 2025 18:54

gaogaotiantian requested a review from markshannon as a code owner February 17, 2025 18:54

bedevere-app bot added the awaiting core review label Feb 17, 2025

gaogaotiantian changed the title ~~Fix a crash when inline comprehension has the same local variable as the outside scope~~ gh-128396: Fix a crash when inline comprehension has the same local variable as the outside scope Feb 17, 2025

bedevere-app bot mentioned this pull request Feb 17, 2025

PEP 667 + PEP 709 segfaults from accessing closure variables bound by any inlined comprehensions #128396

Closed

blurb-it bot and others added 2 commits February 17, 2025 18:59

📜🤖 Added by blurb_it.

4758bd1

Fix lint

13142cc

JelleZijlstra reviewed Feb 17, 2025

View reviewed changes

gaogaotiantian added the needs backport to 3.13 bugs and security fixes label Feb 17, 2025

JelleZijlstra approved these changes Feb 19, 2025

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting core review labels Feb 19, 2025

carljm approved these changes Feb 19, 2025

View reviewed changes

gaogaotiantian merged commit ccf1732 into python:main Feb 19, 2025
48 checks passed

bedevere-app bot removed the awaiting merge label Feb 19, 2025

gaogaotiantian deleted the locals-inline-comp branch February 19, 2025 17:11

miss-islington-app bot assigned gaogaotiantian Feb 19, 2025

gaogaotiantian mentioned this pull request Feb 19, 2025

[3.13] gh-128396: Fix a crash when inline comprehension has the same … #130311

Merged

bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Feb 19, 2025

sergey-miryanov mentioned this pull request Feb 22, 2025

Crash with locals() used in list comprehension #130451

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-128396: Fix a crash when inline comprehension has the same local variable as the outside scope #130235

gh-128396: Fix a crash when inline comprehension has the same local variable as the outside scope #130235

gaogaotiantian commented Feb 17, 2025 •

edited by bedevere-app bot

Loading

JelleZijlstra Feb 17, 2025

gaogaotiantian Feb 17, 2025 •

edited

Loading

gaogaotiantian Feb 17, 2025

gaogaotiantian Feb 17, 2025

carljm Feb 19, 2025

bswck commented Feb 17, 2025

gaogaotiantian commented Feb 17, 2025

gaogaotiantian commented Feb 18, 2025

bswck commented Feb 18, 2025

carljm left a comment

miss-islington-app bot commented Feb 19, 2025

miss-islington-app bot commented Feb 19, 2025

bedevere-app bot commented Feb 19, 2025

bedevere-bot commented Feb 19, 2025

gh-128396: Fix a crash when inline comprehension has the same local variable as the outside scope #130235

gh-128396: Fix a crash when inline comprehension has the same local variable as the outside scope #130235

Conversation

gaogaotiantian commented Feb 17, 2025 • edited by bedevere-app bot Loading

JelleZijlstra Feb 17, 2025

Choose a reason for hiding this comment

gaogaotiantian Feb 17, 2025 • edited Loading

Choose a reason for hiding this comment

gaogaotiantian Feb 17, 2025

Choose a reason for hiding this comment

gaogaotiantian Feb 17, 2025

Choose a reason for hiding this comment

carljm Feb 19, 2025

Choose a reason for hiding this comment

bswck commented Feb 17, 2025

gaogaotiantian commented Feb 17, 2025

gaogaotiantian commented Feb 18, 2025

bswck commented Feb 18, 2025

carljm left a comment

Choose a reason for hiding this comment

miss-islington-app bot commented Feb 19, 2025

miss-islington-app bot commented Feb 19, 2025

bedevere-app bot commented Feb 19, 2025

bedevere-bot commented Feb 19, 2025

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

gaogaotiantian commented Feb 17, 2025 •

edited by bedevere-app bot

Loading

gaogaotiantian Feb 17, 2025 •

edited

Loading