_PyGen_FetchStopIterationValue() crashes on unnormalised exceptions

[scoder]

BPO	23996
Nosy	@ncoghlan, @pitrou, @scoder, @vstinner, @giampaolo, @serhiy-storchaka, @1st1
Files	fix_stopiteration_crash.patch fix_stopiteration_crash.patch: improved patch with fast paths for all normal cases fix_stopiteration_crash.patch: improved patch that should avoid a performance regression in the normal case fix_stopiteration_value_slow.patch fix_stopiteration_value.patch test_stopiteration_tuple_value.patch gen_set_stopiteration_value.patch gen_set_stopiteration_value_2.patch

^{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 2016-11-08.19:33:58.662>
created_at = <Date 2015-04-18.19:45:13.784>
labels = ['interpreter-core', 'type-bug', '3.7']
title = '_PyGen_FetchStopIterationValue() crashes on unnormalised exceptions'
updated_at = <Date 2016-11-08.19:33:58.660>
user = 'https://github.com/scoder'

bugs.python.org fields:

activity = <Date 2016-11-08.19:33:58.660>
actor = 'serhiy.storchaka'
assignee = 'none'
closed = True
closed_date = <Date 2016-11-08.19:33:58.662>
closer = 'serhiy.storchaka'
components = ['Interpreter Core']
creation = <Date 2015-04-18.19:45:13.784>
creator = 'scoder'
dependencies = []
files = ['39108', '39116', '39119', '39691', '39692', '45347', '45353', '45355']
hgrepos = []
issue_num = 23996
keywords = ['patch']
message_count = 25.0
messages = ['241454', '241493', '241516', '241612', '242058', '242060', '244043', '245209', '245250', '245324', '247919', '247927', '247929', '247930', '247937', '247941', '274203', '280029', '280044', '280045', '280046', '280062', '280067', '280149', '280339']
nosy_count = 8.0
nosy_names = ['ncoghlan', 'pitrou', 'scoder', 'vstinner', 'giampaolo.rodola', 'python-dev', 'serhiy.storchaka', 'yselivanov']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue23996'
versions = ['Python 3.4', 'Python 3.5', 'Python 3.6', 'Python 3.7']

[scoder]

The yield-from implementation calls _PyGen_FetchStopIterationValue() to get the exception value. If the StopIteration exception is not normalised, e.g. because it was set by PyErr_SetObject() in a C extension, then _PyGen_FetchStopIterationValue() will cast to (PyStopIterationObject*) whatever the exception value is and happily interpret an arbitrary memory position as PyObject*.

I attached a possible patch for the function. Another place to fix it would be in the yield-from code in ceval.c, but directly genobject.c seems the safer place.

[scoder]

Here's a better patch that avoids exception normalisation in all "normal" cases.

[scoder]

And another patch update that should avoid any potential performance regressions due to the additional type check.

[scoder]

And in fact, fixing it in ceval.c would not be enough, since gen_throw() also calls the function. So this is really the right place to fix it.

[python-dev]

New changeset 15c80f63ea1c by Antoine Pitrou in branch '3.4':
Issue bpo-23996: Avoid a crash when a delegated generator raises an unnormalized StopIteration exception. Patch by Stefan Behnel.
https://hg.python.org/cpython/rev/15c80f63ea1c

New changeset 9d0c6c66b0ac by Antoine Pitrou in branch 'default':
Issue bpo-23996: Avoid a crash when a delegated generator raises an unnormalized StopIteration exception. Patch by Stefan Behnel.
https://hg.python.org/cpython/rev/9d0c6c66b0ac

[pitrou]

Thanks for the patch!

[scoder]

I noticed that my patch isn't entirely correct. If the exception value is a tuple, both PyErr_SetObject() and PyErr_NormalizeException() use it directly as *argument tuple* for the exception instantiation call, i.e. they essentially unpack it into separate arguments. The StopIteration value is then only the first item of that tuple.

I wonder if it's worth repeating this, uhm, surprising special case in yet another place, or if we should just always instantiate the exception.

[scoder]

Here are two patches that fix this case, one with special casing, one without. Please choose and apply one.

[pitrou]

Have you tried benchmarking the "slow" solution?

[scoder]

No. It's more that it feels wrong to spend actual time on the second most common case that can occur instead of just handling it in no time at all. The third case that it's really required to instantiate the StopIteration exception (if user code didn't do so already, see case 1) should almost never occur in practice.

[scoder]

The fix wasn't applied yet, so the current code in 3.4 and later branches is still incorrect. Any of the last two patches ("*_value") will fix it, with my preference on the last one.