Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

_PyGen_FetchStopIterationValue() crashes on unnormalised exceptions #68184

Closed
scoder opened this issue Apr 18, 2015 · 25 comments
Closed

_PyGen_FetchStopIterationValue() crashes on unnormalised exceptions #68184

scoder opened this issue Apr 18, 2015 · 25 comments
Labels
3.7 (EOL) end of life interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error

Comments

@scoder
Copy link
Contributor

scoder commented Apr 18, 2015

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
    Copy link
    Contributor Author

    scoder commented Apr 18, 2015

    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 scoder added type-crash A hard crash of the interpreter, possibly with a core dump interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Apr 18, 2015
    @scoder
    Copy link
    Contributor Author

    scoder commented Apr 19, 2015

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

    @scoder
    Copy link
    Contributor Author

    scoder commented Apr 19, 2015

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

    @scoder
    Copy link
    Contributor Author

    scoder commented Apr 20, 2015

    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
    Copy link
    Mannequin

    python-dev mannequin commented Apr 26, 2015

    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
    Copy link
    Member

    pitrou commented Apr 26, 2015

    Thanks for the patch!

    @pitrou pitrou closed this as completed Apr 26, 2015
    @scoder
    Copy link
    Contributor Author

    scoder commented May 25, 2015

    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 scoder reopened this May 25, 2015
    @scoder
    Copy link
    Contributor Author

    scoder commented Jun 12, 2015

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

    @pitrou
    Copy link
    Member

    pitrou commented Jun 12, 2015

    Have you tried benchmarking the "slow" solution?

    @scoder
    Copy link
    Contributor Author

    scoder commented Jun 13, 2015

    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
    Copy link
    Contributor Author

    scoder commented Aug 3, 2015

    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
    Copy link
    Member

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

    @1st1
    Copy link
    Member

    1st1 commented Aug 3, 2015

    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
    Copy link
    Member

    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
    Copy link
    Contributor Author

    scoder commented Aug 3, 2015

    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
    Copy link
    Member

    Is it possible to test from Python level?

    @scoder
    Copy link
    Contributor Author

    scoder commented Sep 2, 2016

    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.

    @serhiy-storchaka serhiy-storchaka added 3.7 (EOL) end of life type-bug An unexpected behavior, bug, or error and removed type-crash A hard crash of the interpreter, possibly with a core dump labels Nov 3, 2016
    @1st1
    Copy link
    Member

    1st1 commented Nov 3, 2016

    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
    Copy link
    Member

    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
    Copy link
    Member

    1st1 commented Nov 4, 2016

    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
    Copy link
    Member

    1st1 commented Nov 4, 2016

    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
    Copy link
    Member

    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
    Copy link
    Member

    Added comments.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Nov 6, 2016

    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
    Copy link
    Member

    I think that's all with this issue.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees
    No one assigned
    Labels
    3.7 (EOL) end of life interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    5 participants
    @1st1 @scoder @pitrou @gvanrossum @serhiy-storchaka