gh-125038: Iterator checks are added for some FOR_ITER bytecodes, crash fixed

[efimov-mikhail]

SIGSEGV on generators in case of gi_frame.f_locals is fixed.

This applies to _FOR_ITER bytecode implementation.
Similar checks are added to _FOR_ITER_TIER_TWO and INSTRUMENTED_FOR_ITER bytecode implementations.

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

[JelleZijlstra]

Calling PyIter_Check feels a little wasteful. Instead, I'd assign (*Py_TYPE(iter_o)->tp_iternext) to a variable and check if it's NULL. If it is NULL, we should raise a TypeError. Your code instead makes it immediately end the loop, which doesn't feel like the right behavior.

[efimov-mikhail]

Hmm, I've tried to add a TypeError
Such piece of code is provided:

            PyTypeObject *type = Py_TYPE(iter_o);
            iternextfunc iternext = type->tp_iternext;
            if (iternext == NULL) {
                _PyErr_Format(tstate, PyExc_TypeError,
                              "'for' requires an object with "
                              "__iter__ method, got %.100s",
                              type->tp_name);
                DECREF_INPUTS();
                ERROR_IF(true, error);
            }
            PyObject *next_o = (*iternext)(iter_o);

And there is a temporarily result of my test case:

-> % ./python -m unittest -v test.test_generators.GeneratorTest.test_issue125038
test_issue125038 (test.test_generators.GeneratorTest.test_issue125038) ... ERROR

======================================================================
ERROR: test_issue125038 (test.test_generators.GeneratorTest.test_issue125038)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/mikhail.efimov/projects/cpython/Lib/test/test_generators.py", line 274, in test_issue125038
    l = list(g)
  File "/home/mikhail.efimov/projects/cpython/Lib/test/test_generators.py", line 272, in <genexpr>
    g = (x for x in range(10))
                    ~~~~~^^^^
TypeError: 'for' requires an object with __iter__ method, got range

----------------------------------------------------------------------
Ran 1 test in 0.001s

It seems like such a message doesn't provide any clarity.
Do you have any better suggestions about error message?

[JelleZijlstra]

That seems like a pretty good error message, much better than silently stopping the loop.

[JelleZijlstra]

I think you want with self.assertRaisesRegex.

[gaogaotiantian]

We should confirm with @markshannon whether we should fix the interpreter or f_locals. Maybe it's a valid assumption? PEP 667 does not implement everything dict-like for FrameLocalsProxy, but maybe this is necessary? We are adding a small overhead to a very commonly used instruction if I understood it correctly.

[JelleZijlstra]

I looked at PEP 668 and it seems to assume that mutating f_locals is supported, which makes me think that we need to add this check. I wouldn't personally mind blocking the f_locals mutation instead if you can find a principled way to do it.

We are adding a small overhead to a very commonly used instruction if I understood it correctly.

Yes, that's right; we're adding a NULL check and a branch. However, FOR_ITER has a couple of specialized versions, so the overhead should be minimal when iterating over the most common types. Still, it may be worth benchmarking how bad the overhead is.

[JelleZijlstra]

This looks correct to me, but will leave to Mark to see if there's a better solution.

[efimov-mikhail]

Thanks for your advice, it definitely looks better now.

But I have a question.
Is it possible to make the piece of code in the original issue correct?
Why we can't assign different value to the iterator object directly through the frame's f_locals?
I can't see something like this in PEP 667.

[efimov-mikhail]

It seems that this code is incorrect now:

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

But very similar code is correct:

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

Maybe, some change in framelocalsproxy_setitem function can be provided?
Something like this:

if (_PyUnicode_EqualToASCIIString(key, ".0")) {
    PyObject *it = PyObject_GetIter(value);
    if (it != NULL) {
        value = it;
    }
} 

I've provided this change, but it breaks two tests: test_proxy_key_stringlikes_overwrite and test_proxy_key_stringlikes_ftrst_write. Those tests are authored by @encukou and @ncoghlan.

[JelleZijlstra]

Setting .0 to a iter(range(20)) should work for now, yes. I don't know what you're trying to use this for at a higher level, but I'd recommend trying to find a solution that doesn't involve mutating f_locals; that's a low-level interface that should probably only be used by tools like debuggers.

I would oppose adding special handling in the FrameLocalsProxy that mutates objects; doing so would complicate the implementation and make it less useful for tools like debuggers.

[efimov-mikhail]

Actually, there is no real world use case when changing underlying iterator for a generator is needed.
And I understand that f_locals is a low-level interface.

Main purpose of my questions is achieving some clarity.
That's totally okay if the current code behavior is desired.
Maybe just some piece of docs about this use case should be provided.
And, of course, raising the appropriate exceptions is much better than crashing.

[efimov-mikhail]

I've moved my test from test_generators.py to test_frame.py, renamed it, and added more test cases to emphasize current code behavior and save it explicitly in tests.
IMHO, there is still a room to improvement.

[efimov-mikhail]

See #125178