Repository navigation
Error handling for some instructions is incorrect #99298
Description
In several places, we have goto error; branches in bytecode instructions that occur after modifying the next_instr pointer. This is incorrect, since the error branch will behave as if the error occurred in the new location (most often an adjacent instruction). The result could be as benign as an incorrect location in a traceback, or as problematic as incorrect control flow in or near a try/except block.
I tried for a bit to make the compiler emit code that did the wrong thing here, and I wasn't able to. So this is mostly a theoretical concern (but still worth fixing).
Activity
GH-99298: Don't perform jumps before error handling (GH-99299)
pythonGH-99298: Don't perform jumps before error handling (pythonGH-9…
[3.11] GH-99298: Don't perform jumps before error handling (GH-99343)
pythonGH-99298: Don't perform jumps before error handling (pythonGH-9…
gvanrossum commentedon Nov 15, 2022
Interesting. There are now some instrs that have
JUMPY(<cache size>); CHECK_EVAL_BREAKER();.Once those instrs get cache effects theJUMPBY()would come from the generator; can it safely be moved after the eval_breaker check, or does the generator now need to generate those checks too (presumably based on some one-of flag in the instr definition)?brandtbucher commentedon Nov 16, 2022
The
JUMPBY(...);needs to happen before theCHECK_EVAL_BREAKER();(as it does now) since we need to dispatch to the next instruction after handling eval breakers.(Note also that error handling for eval breakers is unaffected by this issue, since we can just pretend that any errors raised by eval breaker checks happened as part of the next instruction... basically, we act as if eval breakers are checked just before the next instruction, rather than just after the current one.)
brandtbucher commentedon Nov 16, 2022
So yeah, we probably need a way to spell this as part of the DSL. But it might be able to wait for now... I think only
CALL,CALL_FUNCTION_EX, andJUMP_BACKWARDare affected (RESUMEis a weird one... it checks the eval breaker only on certain opargs).brandtbucher commentedon Nov 16, 2022
Another option could be to make
CHECK_EVAL_BREAKERits own instruction, and put it after every call and before every backward jump. Not sure how expensive that would be, though.gvanrossum commentedon Nov 16, 2022
I'd rather not add more opcodes. These can use the legacy format for now; eventually we'll add some silly little flag to the instruction definition format.
GH-99298: Clean up attribute specializations (GH-99398)