[3.7] bpo-17288: Prevent jumps from 'return' and 'exception' trace events

[xdegaye]

https://bugs.python.org/issue17288

[serhiy-storchaka]

Could tests use the jump_test() decorator?

[serhiy-storchaka]

Please left this brace on the same line as if to conform PEP 7.

[serhiy-storchaka]

Is there a need to check for RETURN_VALUE as in your original patch?

[xdegaye]

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.

[serhiy-storchaka]

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 list.append() is implemented in Python this will break tests since the tracer will be confused by new frames.

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.trace

and in the constructor:

        self.firstLine = None if decorated else self.code.co_firstlineno

test_jump_forwards_out_of_with_block can be restored, and two new test should be written as:

    @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.

[xdegaye]

@serhiy-storchaka thanks, this is so much simpler. I have updated the PR with your suggestion.

[serhiy-storchaka]

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?

[xdegaye]

@serhiy-storchaka comments have been restored in the last commit.

[miss-islington]

Thanks @xdegaye for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6.
🐍🍒⛏🤖

[serhiy-storchaka]

Great! Could you please create a backport to master?

[bedevere-bot]

GH-6100 is a backport of this pull request to the 3.6 branch.

[miss-islington]

Thanks @xdegaye for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 2.7.
🐍🍒⛏🤖

[miss-islington]

Sorry, @xdegaye and @serhiy-storchaka, I could not cleanly backport this to 2.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker e32bbaf376a09c149fa7c7f2919d7c9ce4e2a055 2.7