cannot jump from a 'return' or 'exception' trace event

[xdegaye]

BPO	17288
Nosy	@birkenfeld, @jcea, @vstinner, @benjaminp, @xdegaye, @serhiy-storchaka, @miss-islington
PRs	[3.7] bpo-17288: Prevent jumps from 'return' and 'exception' trace events #5928 [3.6] bpo-17288: Prevent jumps from 'return' and 'exception' trace events. (GH-5928) #6100 bpo-17288: Prevent jumps from 'return' and 'exception' trace events #6107 [2.7] bpo-17288: Prevent jumps from 'return' and 'exception' trace ev… #6111
Files	return.py jump.py default.patch default_2.patch exception.py

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2018-03-13.21:11:32.839>
created_at = <Date 2013-02-24.17:26:53.129>
labels = ['interpreter-core', '3.7', '3.8', 'type-crash']
title = "cannot jump from a 'return' or 'exception' trace event"
updated_at = <Date 2018-03-13.21:11:32.838>
user = 'https://github.com/xdegaye'

bugs.python.org fields:

activity = <Date 2018-03-13.21:11:32.838>
actor = 'serhiy.storchaka'
assignee = 'none'
closed = True
closed_date = <Date 2018-03-13.21:11:32.839>
closer = 'serhiy.storchaka'
components = ['Interpreter Core']
creation = <Date 2013-02-24.17:26:53.129>
creator = 'xdegaye'
dependencies = []
files = ['29223', '29282', '29283', '35898', '47463']
hgrepos = []
issue_num = 17288
keywords = ['patch']
message_count = 27.0
messages = ['182880', '182881', '183254', '183255', '183264', '222555', '312797', '312808', '312939', '313003', '313007', '313009', '313012', '313015', '313051', '313428', '313432', '313594', '313730', '313737', '313761', '313765', '313766', '313775', '313776', '313778', '313779']
nosy_count = 7.0
nosy_names = ['georg.brandl', 'jcea', 'vstinner', 'benjamin.peterson', 'xdegaye', 'serhiy.storchaka', 'miss-islington']
pr_nums = ['5928', '6100', '6107', '6111']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'crash'
url = 'https://bugs.python.org/issue17288'
versions = ['Python 2.7', 'Python 3.6', 'Python 3.7', 'Python 3.8']

[xdegaye]

On python 3.3 and the default branch, the jump from a 'return' fails although
the change to f_lineno is validated, see below.

This problem does not occur with python 2.7.

$ python return.py
> /tmp/return.py(8)<module>()
-> foo()
(Pdb) break 5
Breakpoint 1 at /tmp/return.py:5
(Pdb) continue
> /tmp/return.py(5)foo()
-> lineno = 5
(Pdb) step
--Return--
> /tmp/return.py(5)foo()->None
-> lineno = 5
(Pdb) jump 4
> /tmp/return.py(4)foo()->None
-> lineno = 4
(Pdb) where
  /tmp/return.py(8)<module>()
-> foo()
> /tmp/return.py(4)foo()->None
-> lineno = 4
(Pdb) step
--Return--
> /tmp/return.py(8)<module>()->None
-> foo()
(Pdb)

[xdegaye]

Oops, it occurs too with python 2.7.

[xdegaye]

Nosying Benjamin Peterson who knows frame_setlineno (bpo-14612) and nosying
Jesús Cea Avión. Hoping they don't mind.

This problem occurs also when setting f_lineno from an exception debug event.

One may crash the interpreter (or get a "SystemError: unknown opcode") when the
return debug event is one that handles a yield statement. For example the
following test causes a segmentation fault on python 3.3.0:

$ python /tmp/jump.py
> /tmp/jump.py(7)<module>()
-> for i in gen():
(Pdb) step
--Call--
> /tmp/jump.py(1)gen()
-> def gen():
(Pdb) step
> /tmp/jump.py(2)gen()
-> for i in range(1):
(Pdb) step
> /tmp/jump.py(3)gen()
-> yield i
(Pdb) step
--Return--
> /tmp/jump.py(3)gen()->0
-> yield i
(Pdb) jump 2
> /tmp/jump.py(2)gen()->0
-> for i in range(1):
(Pdb) step
> /tmp/jump.py(8)<module>()
-> lineno = 8
(Pdb) step
> /tmp/jump.py(7)<module>()
-> for i in gen():
(Pdb) step
--Call--
> /tmp/jump.py(2)gen()->0
-> for i in range(1):
(Pdb) step
Segmentation fault

[xdegaye]

The proposed patch fixes the problem:

• f_lineno cannot be set now from an exception trace function or from a return
  trace function.
• The broken arithmetic involving a null pointer (f->f_stacktop, at the end of
  frame_setlineno when popping blocks that are being jumped from and the
  function is invoked from an exception trace function or a return trace
  function), is fixed now.
• Blocks cannot be popped now in frame_setlineno (the cause of the crash) when
  tracing a yield.

To summarize the proposed fixes on f_lineno accessors:

• setter: f_lineno can only be set in the frame that is being traced (issue
  17277) and from within a line trace function (current issue).
• getter: f_lineno is valid in the frame being traced (bpo-17277) and:
  + not from within a call trace function (i.e. when f->f_trace == NULL)
  + from within a line trace function
  + and from within an exception or return trace function. There is a corner
  case here when returning to a frame that was not being traced previously
  (its f_lineno is not valid) and that has had its local trace function
  f_trace set from within a trace function in one of its callees (the fix of
  bpo-13183 does that) and when the first trace function invoked on
  returning to that frame is an exception or a return trace function: this is
  correctly handled by the f_trace setter that makes sure that f_lineno is
  accurate when setting f_trace (and an extension module implementing tracing,
  must ensure that f_lineno is accurate when setting f_trace).

[xdegaye]

Must add for completeness that f_lineno must be valid within a return trace
function for the reasons explained in the last paragraph of
Objects/lnotab_notes.txt.

[xdegaye]

Python 3.5 is still crashing with this test:

$ python jump.py
> jump.py(7)<module>()
-> for i in gen():
(Pdb) break 3
Breakpoint 1 at jump.py:3
(Pdb) continue
> jump.py(3)gen()
-> yield i
(Pdb) step
--Return--
> jump.py(3)gen()->0
-> yield i
(Pdb) jump 2
> jump.py(2)gen()->0
-> for i in range(1):
(Pdb) continue
Segmentation fault (core dumped)

It is true that frame_setlineno() assumes incorrectly that f->f_stacktop is not NULL, but the reason of the crash or of the "SystemError: unknown opcode" is that PyEval_EvalFrameEx() expects on its invocation f->f_lasti to refer to the index of the last instruction executed and sets (and assumes this instruction does not have argument) 'next_instr' accordingly to the next byte, while by jumping to line 2 we set f->f_lasti to zero and 'SETUP_LOOP', the opcode at this index, has an argument.

The attached patch is a slight improvement over the last one.

[serhiy-storchaka]

Do you mind to create a PR from your patch Xavier? The code of the f_lineno setter was changed in the master branch, so that it is better to create a PR for the 3.7 branch and later port it to master.

[serhiy-storchaka]

Example in msg183254 doesn't crash Python 3.8.

[xdegaye]

Those are the changes with the current behavior from the behavior in 3.5 observed at the time of the initial bug report:
python 3.7:
return.py Unchanged.
exception.py After a jump from the 'exception' event into the previous statement, the ensuing 'step' command triggers a trace 'return' event.
jump.py The sequence of trace events is unchanged and Python aborts now at the last 'step' command with:
python: Python/ceval.c:1083: _PyEval_EvalFrameDefault: Assertion `STACK_LEVEL() <= co->co_stacksize' failed.
python 3.8:
return.py Unchanged.
exception.py Unchanged from 3.7.
jump.py The sequence of trace events is unchanged and the last 'step' command prints now the cryptic error msg:
TypeError: 'NoneType' object is not callable
> /path/to/jump.py(2)gen()->0
-> for i in range(1):

Applying the last patch fixes both 3.7 and 3.8. I can build a PR from this patch. An explanation should be given for the behavior of 3.7 and 3.8 in the jump.py case.

[xdegaye]

Added PR 5928 for the 3.7 branch.

[serhiy-storchaka]

Are there any problems with rebasing it to master?

[xdegaye]

Yes, rebase fails merging two changes in Objects/frameobject.c.
I can write another PR for master if you want.

[serhiy-storchaka]

Usually we create a PR for master (unless the issue is gone here) and backport it to other branches. In this case it may be better to start from 3.7, but it is worth to look at the patch against master for the case if the change can written in the form that minimizes difference between 3.7 and master.

[xdegaye]

Actually

    $ git rebase --onto master 3.7 bpo-17288

fails with one single change in Objects/frameobject.c and one simply needs to (as I have tested it)
remove all the three confict markers in this file and remove the following two lines that were enclosed by these markers:

    min_addr = Py_MIN(new_lasti, f->f_lasti);
    max_addr = Py_MAX(new_lasti, f->f_lasti);

then run:

    $ git add Objects/frameobject.c
    $ git rebase --continue
    $ git co master; git merge bpo-17288

[xdegaye]

xdegaye wrote:

An explanation should be given for the behavior of 3.7 and 3.8 in the jump.py case.

In 3.7 when running jump.py as in msg183254, Python aborts with a frame stack overflow. The reason is that when the generator is resumed after the jump, its send() method is called with an argument and pushes the argument on the frame stack (i.e. f->f_stacktop - f->f_valuestack == 1 in gen_send_ex() before the call to PyEval_EvalFrameEx()). This argument is supposed to be popped by the first instruction executed by the generator which is expected to be YIELD_VALUE but, because of the jump, f->f_lasti is now 0 and the send() argument is not popped. Hence the stack overflow.

When LLTRACE is undefined in ceval.c, stack oveflow checking is disabled. I have checked with gdb that, in that case, when YIELD_VALUE is about to be executed then STACK_LEVEL() is 3 instead of 1 and therefore YIELD_VALUE does not pop the right value from the stack. The stack is indeed corrupted.

So there are two reasons for forbiddig to jump from a yield statement:

• the ceval loop has exited and the jump is not allowing the user to jump to another line
• after the jump, resuming a generator corrupts the frame stack

[serhiy-storchaka]

Thank you Xavier. I had pleasure from reviewing your patch. But please update tests for using the jump_test() decorator.

[xdegaye]

Sorry, for some reason github did not send me the emails of your review and I did not think about checking the PR :-(
Thanks for your review, I will work on it shortly.

[xdegaye]

Serhiy, I have updated the tests for using the jump_test() decorator and replied to your comment about RETURN_VALUE.

[serhiy-storchaka]

New changeset e32bbaf by Serhiy Storchaka (xdegaye) in branch '3.7':
[3.7] bpo-17288: Prevent jumps from 'return' and 'exception' trace events. (GH-5928)
e32bbaf

[miss-islington]

New changeset cf61a81 by Miss Islington (bot) in branch '3.6':
[3.7] bpo-17288: Prevent jumps from 'return' and 'exception' trace events. (GH-5928)
cf61a81

[xdegaye]

In PR 5928 Serhiy Storchaka wrote:

Great! Could you please create a backport to master?

Yes.
I prefer to answer in the issue as I am still not getting any mail from github.

[serhiy-storchaka]

New changeset b8e9d6c by Serhiy Storchaka (xdegaye) in branch 'master':
bpo-17288: Prevent jumps from 'return' and 'exception' trace events. (GH-6107)
b8e9d6c

[serhiy-storchaka]

Thank you! Do you mind to make a backport to 2.7 (if this makes a sense)?

[xdegaye]

The conditions used for the fix are still the same in 2.7: f->f_trace == NULL on a call event, f->f_lasti == -1 in a new frame and f->f_stacktop == NULL in a trace function with a non-line event except for yield statements.
There are minor differences: the YIELD_FROM opcode does not exist and function.__code__ is written as function.func_code.

Added a PR for 2.7.

[xdegaye]

FWIW I now get github notifications, the problem was some bad configuration of my github profile (my fault).

[serhiy-storchaka]

New changeset baca85f by Serhiy Storchaka (xdegaye) in branch '2.7':
[2.7] bpo-17288: Prevent jumps from 'return' and 'exception' trace events. (GH-6111)
baca85f

[serhiy-storchaka]

Thank you Xavier!