Failed assert(defcount <= code->co_argcount) with sneaky __defaults__ replacement

[brandtbucher]

Crash report

It's possible to fail an assert on debug builds by cleverly constructing function objects with the same version, but different numbers of defaults.

Example:

>>> def outer():
...     def inner(x=None):
...         pass
...     return inner
... 
>>> a = outer()
>>> co_consts = outer.__code__.co_consts[:-1] + ((None, None),)
>>> outer.__code__ = outer.__code__.replace(co_consts=co_consts)
>>> b = outer()
>>> a.__defaults__
(None,)
>>> b.__defaults__
(None, None)
>>> def loop(f):
...     for _ in range(1000):
...         f()
... 
>>> loop(a)
>>> loop(b)
python: Python/generated_cases.c.h:1828: _PyEval_EvalFrameDefault: Assertion `defcount <= code->co_argcount' failed.
Aborted

I think this is just an incorrect assert, but I'm not 100% sure. Haven't tested on earlier versions, but I suspect that 3.12 (and maybe even 3.11) fails here too.

This may be a good first issue for someone trying to dip their toes in the call specializations.

[brandtbucher]

Originally discovered during discussion of #117045 (comment).

[gvanrossum]

I suspect that 3.12 (and maybe even 3.11) fails here too.

Note that to test this, ./configure --with-debug is required, or ./configure --with-assertions; otherwise the assert() will never trigger. :-)

[gaogaotiantian]

What's the meaning to have a (None, None) default for a function inner(x=None) that only has one parameter? Sure you can construct it with smart code const replacements, but that does not quite make sense.

[brandtbucher]

Sure you can construct it with smart code const replacements, but that does not quite make sense.

Eh, yeah. Maybe we should just say "we can't possibly support all of the weird code objects you can possibly create, so this isn't really a bug".

This one just feels "different" since it only breaks when specializing.

[gaogaotiantian]

Yeah, you can easily crash the interpreter if you do weird stuff to the code object

outer.__code__ = outer.__code__.replace(co_consts=tuple())

That's just something that you need to be responsible for - I don't think it's feasible for CPython to avoid crashing in any arbitrary case when the user can basically construct their code objects freely.

[gvanrossum]

Actually, I believe there is a difference. Yes, using code.replace() you can create crazy crashing code objects. Don't do that. But no, assignment of odd values to critical attributes of function objects should not allow crashing.

This bug falls into the latter category and needs to be fixed. IMO just deleting the offending assert() happens to work in this particular case.

PS. Critical function attributes are: __code__, __closure__, __defaults__, __kwdefaults__.

[gaogaotiantian]

But this is not assignment to critical attributes. You can't repro this by assigning value to func.__default__ - it will clear the function version thus deopt the specialization. This is an example of constructing crazy code objects by replacing the co_consts. If there's a way to trigger this with assigning to function attributes, then I agree this is a problem and should be fixed.

You can easily replace the co_consts line with

co_consts = outer.__code__.co_consts[:-1] + ([None, None],)

and trigger another assert:

python: Python/generated_cases.c.h:5231: _PyEval_EvalFrameDefault: Assertion `PyTuple_CheckExact(attr)' failed.

I don't see a difference between the mechnism - once you change co_consts, you can do a lot of illegal stuff.

[gvanrossum]

Yeah. Using code.replace() you can create arbitrary nonsense. Using only critical function attribute assignment you should not be. The example at the top of this issue is the only known instance of the latter.

[gaogaotiantian]

Yeah. Using code.replace() you can create arbitrary nonsense. Using only critical function attribute assignment you should not be. The example at the top of this issue is the only known instance of the latter.

Sorry which example? The example Brandt gave in this issue uses code.replace(), not attribute assignment. Maybe I missed something here?

[gvanrossum]

Oh, that's true. There may be a variant that just requires assignment to __defaults__, but we haven't demonstrated it.

[gaogaotiantian]

In func_set_defaults (descriptor for __defaults__), the function version is set to 0 by _PyFunction_SetVersion(op, 0);. Hence any writes to __defaults__ will clear the function version, which will thus deopt the instruction before that assert is hit. I tried it in a naive way and failed to repro the issue. I doubt if there is one.

[gvanrossum]

Mark wants to keep the assert, so let’s close the issue.