[3.7] bpo-17288: Prevent jumps from 'return' and 'exception' trace events#5928
Conversation
| sys.settrace(None) | ||
| self.compare_jump_output([2, 3, 2, 3, 4], namespace["output"]) | ||
|
|
||
| def test_no_jump_from_call(self): |
There was a problem hiding this comment.
Could tests use the jump_test() decorator?
| * Forbidding jumps from the 'call' event of a new frame is a side effect | ||
| * of allowing to set f_lineno only from trace functions. */ | ||
| if (f->f_lasti == -1) | ||
| { |
There was a problem hiding this comment.
Please left this brace on the same line as if to conform PEP 7.
| /* The trace function is called with a 'return' trace event after the | ||
| * execution of a yield statement. */ | ||
| assert(f->f_lasti != -1); | ||
| if (code[f->f_lasti] == YIELD_VALUE || code[f->f_lasti] == YIELD_FROM) { |
There was a problem hiding this comment.
Is there a need to check for RETURN_VALUE as in your original patch?
There was a problem hiding this comment.
The only function called by _PyEval_EvalFrameDefault() that may allow a jump to another line is maybe_call_line_trace() because stack_pointer is saved on f->f_stacktop before the call and there is a JUMPTO(f->f_lasti) after the call. All the other trace functions called within _PyEval_EvalFrameDefault() are called with f->f_stacktop == NULL except after the handling of the YIELD_FROM and YIELD_VALUE opcodes. So by forbidding jumps in frame_setlineno() when f->f_stacktop == NULL we handle all the cases, including the RETURN_VALUE opcode, except the yield statements. The check for RETURN_VALUE in my original patch was overkill.
Fix also a bug in the previous implementation of the PR where jumpFrom/jumpTo were systematically incremented by 1 by the jump_test() decorator without taking into account that JumpTracer allows now jumpFrom/jumpTo from within nested functions.
|
Thank you @xdegaye! This is very nice PR. I like how it fixes the non-trivial bug. I have learned something new when reading it. But I don't like the complexity of the new implementation of the trace function and the fact that it breaks one existing test. This code looks complicated and fragile. I afraid that it will be hard to modify it for supporting new tests or fix it if some details of frames or code objects will be changed. If in some implementation I suggest to use the following implementation: def trace(self, frame, event, arg):
if self.done:
return
if (self.firstLine is None and frame.f_code == self.code and
event == 'line'):
self.firstLine = frame.f_lineno - 1
if (event == self.event and self.firstLine and
frame.f_lineno == self.firstLine + self.jumpFrom):
f = frame
while f is not None and f.f_code != self.code:
f = f.f_back
if f is not None:
try:
frame.f_lineno = self.firstLine + self.jumpTo
except TypeError:
frame.f_lineno = self.jumpTo
self.done = True
return self.traceand in the constructor: self.firstLine = None if decorated else self.code.co_firstlineno
@jump_test(2, 3, [1], event='call', error=(ValueError, "can't jump from"
" the 'call' trace event of a new frame"))
def test_no_jump_from_call(output):
output.append(1)
def nested():
output.append(3)
nested()
output.append(5)
@jump_test(3, 2, [2], event='return', error=(ValueError,
"can't jump from a yield statement"))
def test_no_jump_from_yield(output):
def gen():
output.append(2)
yield 3
next(gen())
output.append(5)Line numbers are always relative to the first line of the testing function which is determined by the first executed line if decorated is true. |
|
@serhiy-storchaka thanks, this is so much simpler. I have updated the PR with your suggestion. |
serhiy-storchaka
left a comment
There was a problem hiding this comment.
Thank you @xdegaye!
I have removed comments for brevity in the example. Could you please restore the old comment about non-integer jumpTo? Or maybe even add new comments to non-trivial code?
|
@serhiy-storchaka comments have been restored in the last commit. |
|
Thanks @xdegaye for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6. |
|
Great! Could you please create a backport to master? |
…ents. (pythonGH-5928) (cherry picked from commit e32bbaf) Co-authored-by: xdegaye <xdegaye@gmail.com>
|
GH-6100 is a backport of this pull request to the 3.6 branch. |
|
Thanks @xdegaye for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 2.7. |
|
Sorry, @xdegaye and @serhiy-storchaka, I could not cleanly backport this to |
https://bugs.python.org/issue17288