classification
Title: _PyGen_FetchStopIterationValue() crashes on unnormalised exceptions
Type: behavior Stage: resolved
Components: Interpreter Core Versions: Python 3.7, Python 3.6, Python 3.5, Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: giampaolo.rodola, ncoghlan, pitrou, python-dev, scoder, serhiy.storchaka, vstinner, yselivanov
Priority: normal Keywords: patch

Created on 2015-04-18 19:45 by scoder, last changed 2016-11-08 19:33 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
fix_stopiteration_crash.patch scoder, 2015-04-18 19:45 review
fix_stopiteration_crash.patch scoder, 2015-04-19 08:19 improved patch with fast paths for all normal cases review
fix_stopiteration_crash.patch scoder, 2015-04-19 14:09 improved patch that should avoid a performance regression in the normal case review
fix_stopiteration_value_slow.patch scoder, 2015-06-12 05:35 review
fix_stopiteration_value.patch scoder, 2015-06-12 05:35 review
test_stopiteration_tuple_value.patch serhiy.storchaka, 2016-11-04 12:16 review
gen_set_stopiteration_value.patch serhiy.storchaka, 2016-11-04 18:06 review
gen_set_stopiteration_value_2.patch serhiy.storchaka, 2016-11-04 18:52 review
Messages (25)
msg241454 - (view) Author: Stefan Behnel (scoder) * Date: 2015-04-18 19:45
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.
msg241493 - (view) Author: Stefan Behnel (scoder) * Date: 2015-04-19 08:19
Here's a better patch that avoids exception normalisation in all "normal" cases.
msg241516 - (view) Author: Stefan Behnel (scoder) * Date: 2015-04-19 14:09
And another patch update that should avoid any potential performance regressions due to the additional type check.
msg241612 - (view) Author: Stefan Behnel (scoder) * Date: 2015-04-20 06:19
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.
msg242058 - (view) Author: Roundup Robot (python-dev) Date: 2015-04-26 16:49
New changeset 15c80f63ea1c by Antoine Pitrou in branch '3.4':
Issue #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 #23996: Avoid a crash when a delegated generator raises an unnormalized StopIteration exception.  Patch by Stefan Behnel.
https://hg.python.org/cpython/rev/9d0c6c66b0ac
msg242060 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2015-04-26 16:51
Thanks for the patch!
msg244043 - (view) Author: Stefan Behnel (scoder) * Date: 2015-05-25 18:08
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.
msg245209 - (view) Author: Stefan Behnel (scoder) * Date: 2015-06-12 05:35
Here are two patches that fix this case, one with special casing, one without. Please choose and apply one.
msg245250 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2015-06-12 13:36
Have you tried benchmarking the "slow" solution?
msg245324 - (view) Author: Stefan Behnel (scoder) * Date: 2015-06-13 19:41
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.
msg247919 - (view) Author: Stefan Behnel (scoder) * Date: 2015-08-03 07:05
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.
msg247927 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2015-08-03 14:52
Please try to make sure this is fixed before 3.5 rc 1.
msg247929 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-08-03 15:07
> 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?
msg247930 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-08-03 16:06
Could you provide tests covering all branches (normalized exception, unnormalized exception, absent value, non-tuple value, empty tuple value, non-empty tuple value...) Stefan?
msg247937 - (view) Author: Stefan Behnel (scoder) * Date: 2015-08-03 16:48
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.
msg247941 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-08-03 17:13
Is it possible to test from Python level?
msg274203 - (view) Author: Stefan Behnel (scoder) * Date: 2016-09-02 05:19
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.
msg280029 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2016-11-03 22:24
> 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
msg280044 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-11-04 11:01
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, ...).
msg280045 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2016-11-04 11:34
Serhiy, I think you forgot to attach the patch.

aiter_wrapper shouldn't ever receive tuples, so it should be fine with PyErr_SetObject.
msg280046 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2016-11-04 12:30
> 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?
msg280062 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-11-04 18:06
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.
msg280067 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-11-04 18:52
Added comments.
msg280149 - (view) Author: Roundup Robot (python-dev) Date: 2016-11-06 16:48
New changeset bce18f5c0bc4 by Serhiy Storchaka in branch '3.5':
Issue #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 #23996: Added _PyGen_SetStopIterationValue for safe raising
https://hg.python.org/cpython/rev/a2c9f06ada28

New changeset d33b9fd46cef by Serhiy Storchaka in branch 'default':
Issue #23996: Added _PyGen_SetStopIterationValue for safe raising
https://hg.python.org/cpython/rev/d33b9fd46cef
msg280339 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-11-08 19:33
I think that's all with this issue.
History
Date User Action Args
2016-11-08 19:33:58serhiy.storchakasetstatus: open -> closed
resolution: fixed
messages: + msg280339

stage: patch review -> resolved
2016-11-06 16:48:05python-devsetmessages: + msg280149
2016-11-04 21:10:30gvanrossumsetnosy: - gvanrossum
2016-11-04 18:52:08serhiy.storchakasetfiles: + gen_set_stopiteration_value_2.patch

messages: + msg280067
2016-11-04 18:06:42serhiy.storchakasetfiles: + gen_set_stopiteration_value.patch

messages: + msg280062
stage: test needed -> patch review
2016-11-04 12:30:29yselivanovsetmessages: + msg280046
2016-11-04 12:16:44serhiy.storchakasetfiles: + test_stopiteration_tuple_value.patch
2016-11-04 11:34:26yselivanovsetmessages: + msg280045
2016-11-04 11:01:12serhiy.storchakasetmessages: + msg280044
2016-11-03 22:24:51yselivanovsetmessages: + msg280029
2016-11-03 20:42:05serhiy.storchakasettype: crash -> behavior
versions: + Python 3.7
2016-09-02 05:19:04scodersetmessages: + msg274203
2015-08-03 17:13:48serhiy.storchakasetmessages: + msg247941
2015-08-03 16:48:27scodersetmessages: + msg247937
2015-08-03 16:06:57serhiy.storchakasetnosy: + serhiy.storchaka

messages: + msg247930
stage: patch review -> test needed
2015-08-03 15:07:04yselivanovsetmessages: + msg247929
2015-08-03 14:52:16gvanrossumsetmessages: + msg247927
2015-08-03 08:13:11pitrousetnosy: + gvanrossum, vstinner, giampaolo.rodola
2015-08-03 07:05:13scodersetmessages: + msg247919
versions: + Python 3.6
2015-06-17 16:56:58yselivanovsetnosy: + yselivanov

resolution: fixed -> (no value)
stage: resolved -> patch review
2015-06-13 19:41:07scodersetmessages: + msg245324
2015-06-12 13:36:45pitrousetmessages: + msg245250
2015-06-12 05:35:39scodersetfiles: + fix_stopiteration_value.patch
2015-06-12 05:35:13scodersetfiles: + fix_stopiteration_value_slow.patch

messages: + msg245209
2015-05-25 18:08:54scodersetstatus: closed -> open

messages: + msg244043
2015-04-26 16:51:53pitrousetstatus: open -> closed
versions: - Python 3.3
messages: + msg242060

resolution: fixed
stage: resolved
2015-04-26 16:49:49python-devsetnosy: + python-dev
messages: + msg242058
2015-04-26 07:43:37scodersetnosy: + pitrou
2015-04-20 06:19:04scodersetmessages: + msg241612
2015-04-20 06:09:50scodersetnosy: + ncoghlan
2015-04-19 14:09:11scodersetfiles: + fix_stopiteration_crash.patch

messages: + msg241516
2015-04-19 08:19:32scodersetfiles: + fix_stopiteration_crash.patch

messages: + msg241493
2015-04-18 19:45:13scodercreate