| 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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
Date: 2016-11-08 19:33 |
| I think that's all with this issue. |
|
|