GeezerVison is showing github.com — Error handling for some instructions is incorrect · Issue #99298 · python/cpython
Error handling for some instructions is incorrect #99298Labels 3.11only security fixesonly security fixes3.12only security fixesonly security fixesinterpreter-core(Objects, Python, Grammar, and Parser dirs)(Objects, Python, Grammar, and Parser dirs)type-bugAn unexpected behavior, bug, or errorAn unexpected behavior, bug, or error DescriptionIn 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).
Activitygvanrossum commented on Nov 15, 2022 Interesting. There are now some instrs that have JUMPY(<cache size>); CHECK_EVAL_BREAKER(); .Once those instrs get cache effects the JUMPBY() 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 commented on Nov 16, 2022 The JUMPBY(...); needs to happen before the CHECK_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 commented on 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, and JUMP_BACKWARD are affected (RESUME is a weird one... it checks the eval breaker only on certain opargs). brandtbucher commented on Nov 16, 2022 Another option could be to make CHECK_EVAL_BREAKER its own instruction, and put it after every call and before every backward jump. Not sure how expensive that would be, though. gvanrossum commented on 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.
MetadataMetadata3.11only security fixesonly security fixes3.12only security fixesonly security fixesinterpreter-core(Objects, Python, Grammar, and Parser dirs)(Objects, Python, Grammar, and Parser dirs)type-bugAn unexpected behavior, bug, or errorAn unexpected behavior, bug, or error DevelopmentNo branches or pull requestsIssue actions
|