gh-125038: Crash after genexpr.gi_frame.f_locals manipulations is fixed

[efimov-mikhail]

Some iterator checks are added for _FOR_ITER, _FOR_ITER_TIER_TWO and INSTRUMENTED_FOR_ITER bytecode implementations. TypeError is raised in case of tp_iternext == NULL. Tests on generator modifying through gi_frame.f_locals are added, both to genexpr generators and function generators.

I've added some test cases to emphasize current code behavior and save it explicitly in tests.
IMHO, there is still a room to improvement.

Previous PR becomes incorrect (#125051) after my typo in git commands.
I'm sorry about that.

@JelleZijlstra, Could you please review this again?
Moreover, label "needs backport to 3.13" should be added once again.

• Issue: SIGSEGV with generators after direct change in gi_frame #125038

[JelleZijlstra]

This error is incorrect as it's looking for __next__, not __iter__. I think we can use the same error as builtins.next():

        PyErr_Format(PyExc_TypeError, 
            "'%.200s' object is not an iterator",
            Py_TYPE(it)->tp_name);

[markshannon]

I think this is wrong approach. Rather than modifying FOR_ITER, etc to handle an incorrect VM state, we should generate code that cannot get into that state, in the bytecode compiler.

Currently, the bytecode compiler assumes that local variables in generator expressions cannot be modified in
This is no longer true, but it remains true that the evaluation stack cannot be modified.

The bytecode for (x for x in ...) starts with:

      0 RETURN_GENERATOR
      2 POP_TOP
      4 RESUME                   0
      6 LOAD_FAST                0 (.0)
>>    8 FOR_ITER                 6 (to 24)

Compare that with the code for for x in ....:

      0 RESUME                   0
      2 LOAD_FAST                0 (...)
      4 GET_ITER
>>    6 FOR_ITER                 4 (to 18)
      10 STORE_FAST               1 (x)

Note the additional GET_ITER as the bytecode compiler doesn't know what ... is.

I think adding the GET_ITER before the FOR_ITER would gain the robustness required in this case, without complicating the iteration code.

[efimov-mikhail]

So, basically, we want to provide equal results for such code fragments:

1. Iterable in f_locals['.0']

g = (x for x in range(10))
g.gi_frame.f_locals['.0'] = range(20)

2. Iterator in f_locals['.0']

g = (x for x in range(10))
g.gi_frame.f_locals['.0'] = iter(range(20))

Have I got your idea correct on high level?

IMHO, this will change be convenient.

[markshannon]

Have I got your idea correct on high level?

Yes, I think those should provide the same results.

[markshannon]

We also want to avoid emitting GET_ITER twice. It is currently emitted in the scope surrounding the generator expression.
We should move it into the generator expression.

In other words, this function:

def f(s):
    return (x for x in s)

currently generates the following bytecode:

  1           RESUME                   0

  2           LOAD_CONST               1 (<code object <genexpr> at 0x7fc1c815e6a0, file "<python-input-0>", line 2>)
              MAKE_FUNCTION
              LOAD_FAST_TEMP           0 (s)
              GET_ITER
              CALL                     0
              RETURN_VALUE

Disassembly of <code object <genexpr> at 0x7fc1c815e6a0, file "<python-input-0>", line 2>:
   2           RETURN_GENERATOR
               POP_TOP
       L1:     RESUME                   0
               LOAD_FAST                0 (.0)
       L2:     FOR_ITER                 6 (to L3)
               ...

I think it should generate:

  1           RESUME                   0

  2           LOAD_CONST               1 (<code object <genexpr> at 0x7fc1c815e6a0, file "<python-input-0>", line 2>)
              MAKE_FUNCTION
              LOAD_FAST_TEMP           0 (s)
              CALL                     0
              RETURN_VALUE

Disassembly of <code object <genexpr> at 0x7fc1c815e6a0, file "<python-input-0>", line 2>:
   2           RETURN_GENERATOR
               POP_TOP
       L1:     RESUME                   0
               LOAD_FAST                0 (.0)
               GET_ITER
       L2:     FOR_ITER                 6 (to L3)
               ...

This is a behavior change, but I think a correct one. If f(2) is called, we get a TypeError, but this seems wrong. The error should occur once the generator expression is iterated over.

[efimov-mikhail]

We also want to avoid emitting GET_ITER twice. It is currently emitted in the scope surrounding the generator expression. We should move it into the generator expression.

Agree.

This is a behavior change, but I think a correct one. If f(2) is called, we get a TypeError, but this seems wrong. The error should occur once the generator expression is iterated over.

Could you please give some additional information about this behavior change?
Some code snippets, which results will be changed.

[JelleZijlstra]

Could you please give some additional information about this behavior change? Some code snippets, which results will be changed.

Currently, this happens:

>>> (x for x in 1)
Traceback (most recent call last):
  File "<python-input-0>", line 1, in <module>
    (x for x in 1)
                ^
TypeError: 'int' object is not iterable

With Mark's proposed change, this error would be thrown only when the generator is iterated over.

I think either behavior is fine and I agree with Mark that the proposed change makes for a more elegant implementation, but it is a change in core language behavior that I could see impacting some users (e.g., if you put a try-except around the construction of the genexp), so I wouldn't feel comfortable making that change in a bugfix release.

@markshannon what do you think of applying this PR's approach in the 3.13 branch, and your idea on main only?

[efimov-mikhail]

With Mark's proposed change, this error would be thrown only when the generator is iterated over.

Understood. Thx for the clarification.

applying this PR's approach in the 3.13 branch, and "GET_ITER changes" on main only?

It looks like a good idea for me, since crash through f_locals modification is already fixed.
And future improvements for GET_ITER bytecode moving could be provided separately.
But it's up to you to decide, of course.

There will be no problem in backports in the case of two separate PRs?

[markshannon]

My proposed fix should have no performance impact (it does no extra work), so I'd definitely prefer that.

If you think that is too intrusive for 3.13, we could leave the GET_ITER in the enclosing scope and add a new CHECK_ITER instruction to be inserted before the FOR_ITER.
CHECK_ITER would raise if the value TOS is not an iterator, otherwise do nothing.

[efimov-mikhail]

I like the idea of new CHECK_ITER bytecode.

What do you think about this plan:

1. Add CHECK_ITER bytecode definition, put it in a proper place in generator expressions code, add correct reaction on it in the interpreter in this PR.
2. Merge this PR to main, provide a backport to 3.13.
3. Create a new PR.
4. Move GET_ITER bytecode from the enclosing function to the place of CHECK_ITER bytecode, remove CHECK_ITER, merge PR to main, do not provide any backports.

And what about Python 3.12 and below?
Should we provide any bug fix to those branches?

[markshannon]

That sounds like a good plan.

If the fix is simple enough, then we can backport to 3.12.
I don't think it is worth worrying about 3.11 and earlier.

[efimov-mikhail]

I'd prefer (3). But (2) is fine too.

What do you think, @markshannon, @JelleZijlstra, @sobolevn ?

[markshannon]

As @JelleZijlstra points out, bumping the magic number could cause problems in a backport.

Given that, maybe the proposed compiler change moving the GET_ITER into the generator is the correct one for the backport.

So, which is the best option?

1. Not fixing the bug at all
2. A magic number change, breaking .pyc files distrubuted without source
3. Delaying when an exception is thrown as described above

Personally, I think 3 is best.
@JelleZijlstra, @sobolevn what do you think?

[sobolevn]

In my opinion this can be qualified as a feature, rather than a bug. No real users ever complained about this problem (@efimov-mikhail confirmed that this was found by his attempts to specifically learn about the generator internals). While technically possible to cause a crash, in practice - it does not.

When messing around with undocumented internals - crashes can happen.

So, I feel like the safest way here is to fix the problem with the new bytecode (current PR state) and just do not backport this at all.

[markshannon]

As @JelleZijlstra points out, GET_ITER is idempotent, so we can drop CHECK_ITER and use GET_ITER instead.

For 3.14, we can then drop the GET_ITER in the caller.

[efimov-mikhail]

As @JelleZijlstra points out, GET_ITER is idempotent, so we can drop CHECK_ITER and use GET_ITER instead.

For 3.14, we can then drop the GET_ITER in the caller.

It seems like this solution will be best tradeoff.
I will provide such changes to this PR.

[cpython-cla-bot]

All commit authors signed the Contributor License Agreement.

[markshannon]

This might introduce a small slowdown in 3.13, but it isn't in the loop, so shouldn't be an issue.

I'm not worried about that though, as we can remove the other GET_ITER and thus the overhead in 3.14.

[markshannon]

Looks good me once you've agreed on a better name for ModifyTest.

[efimov-mikhail]

It's okay for me to change name from "ModifyTest" to something.
Maybe "ModifyBaseIteratorTest"?
Or "ModifyUnderlyingIterableTest"?

[JelleZijlstra]

Either of those names works for me.

[sobolevn]

Thank you!

[efimov-mikhail]

Looks good me once you've agreed on a better name for ModifyTest.

It seems we have agreed. Anything else, or it could be merged?

[miss-islington-app]

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

[miss-islington-app]

Sorry, @efimov-mikhail and @JelleZijlstra, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 079875e39589eb0628b5883f7ffa387e7476ec06 3.13

[bedevere-app]

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