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

[gvanrossum]

Please try to make sure this is fixed before 3.5 rc 1.

[1st1]

Any of the last two patches ("*_value") will fix it, with my preference on the last one.

Stefan, the last patch looks good to me. Do you think we can have a unittest for this?

[serhiy-storchaka]

Could you provide tests covering all branches (normalized exception, unnormalized exception, absent value, non-tuple value, empty tuple value, non-empty tuple value...) Stefan?

[scoder]

Regarding tests, it looks like iteration isn't currently tested at the C
level at all. At least, the xx test modules don't have any types that use
it. I can write one up next week, or add it to one of the existing types
(Xxo_Type?). Unlikely that I'll make the deadline for rc1 next weekend, though.

[serhiy-storchaka]

Is it possible to test from Python level?

[scoder]

Looks like I forgot about this. My final fix still hasn't been applied, so the code in Py3.4+ is incorrect now.

No, this cannot be tested from the Python level.

[1st1]

Looks like I forgot about this. My final fix still hasn't been applied, so the code in Py3.4+ is incorrect now.

Left a question in code review

[serhiy-storchaka]

Here is a test that passed with current code but will fail with the patch. I don't know whether it make much sense. If yes, then perhaps aiter_wrapper_iternext needs the same workaround as other invocations of PyErr_SetObject(PyExc_StopIteration, ...).

[1st1]

Serhiy, I think you forgot to attach the patch.

aiter_wrapper shouldn't ever receive tuples, so it should be fine with PyErr_SetObject.

[1st1]

No, this cannot be tested from the Python level.

Stefan, could you please upload a C program that showcases the bug you're trying to fix?

[serhiy-storchaka]

Yet one special case -- if asynchronous iterator in aiter_wrapper is an instance of StopIteration.

Proposed patch adds the function _PyGen_SetStopIterationValue() that raises StopIteration with correctly wrapped value (exception is normalized only if needed) and replaces 4 code duplications with it. The patch also includes Yury's variant of Stefan's patch and additional tests.

[serhiy-storchaka]

Added comments.

[python-dev]

New changeset bce18f5c0bc4 by Serhiy Storchaka in branch '3.5':
Issue bpo-23996: Added _PyGen_SetStopIterationValue for safe raising
https://hg.python.org/cpython/rev/bce18f5c0bc4

New changeset a2c9f06ada28 by Serhiy Storchaka in branch '3.6':
Issue bpo-23996: Added _PyGen_SetStopIterationValue for safe raising
https://hg.python.org/cpython/rev/a2c9f06ada28

New changeset d33b9fd46cef by Serhiy Storchaka in branch 'default':
Issue bpo-23996: Added _PyGen_SetStopIterationValue for safe raising
https://hg.python.org/cpython/rev/d33b9fd46cef

[serhiy-storchaka]

I think that's all with this issue.