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

PyType_Ready() should sanity-check the tp_name field #62487

Closed
nkoep mannequin opened this issue Jun 23, 2013 · 8 comments
Closed

PyType_Ready() should sanity-check the tp_name field #62487

nkoep mannequin opened this issue Jun 23, 2013 · 8 comments
Assignees
@serhiy-storchaka
Labels
3.7 (EOL) end of life interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump

Comments

@nkoep
Copy link
Mannequin

nkoep mannequin commented Jun 23, 2013

BPO 18287
Nosy @amauryfa, @benjaminp, @serhiy-storchaka
PRs
  • [Do Not Merge] Convert Misc/NEWS so that it is managed by towncrier #552
    		•
    Files
  • patch.default
    		•

    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 = 'https://github.com/serhiy-storchaka'
closed_at = <Date 2016-10-07.20:28:15.255>
created_at = <Date 2013-06-23.15:12:52.375>
labels = ['interpreter-core', '3.7', 'type-crash']
title = 'PyType_Ready() should sanity-check the tp_name field'
updated_at = <Date 2017-03-31.16:36:27.293>
user = 'https://bugs.python.org/nkoep'

    bugs.python.org fields:

    activity = <Date 2017-03-31.16:36:27.293>
actor = 'dstufft'
assignee = 'serhiy.storchaka'
closed = True
closed_date = <Date 2016-10-07.20:28:15.255>
closer = 'serhiy.storchaka'
components = ['Interpreter Core']
creation = <Date 2013-06-23.15:12:52.375>
creator = 'nkoep'
dependencies = []
files = ['30724']
hgrepos = []
issue_num = 18287
keywords = []
message_count = 8.0
messages = ['191705', '191986', '191990', '191991', '258336', '277882', '278266', '278267']
nosy_count = 6.0
nosy_names = ['amaury.forgeotdarc', 'benjamin.peterson', 'python-dev', 'serhiy.storchaka', 'nkoep', 'superluser']
pr_nums = ['552']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'crash'
url = 'https://bugs.python.org/issue18287'
versions = ['Python 2.7', 'Python 3.5', 'Python 3.6', 'Python 3.7']
    @nkoep
    Copy link
    Mannequin Author

    nkoep mannequin commented Jun 23, 2013

    I noticed that defining a new type where the tp_name field is NULL causes segfaults, for instance, when calling pydoc on the extension module. This particular segfault traces back to type_module() in Objects/typeobject.c where tp_name is passed to strrchr(). Raising an appropriate exception from PyType_Ready() seems sensible to avoid this kind of issue. The field is also used in two calls to PyErr_Format() in PyType_Ready() itself where it'll cause segfaults if not handled properly.

    While we're on the subject, pydoc doesn't list the type documentation if the tp_name string does not have a dot in it. I didn't research this any further as omitting dots seems to be valid according to the docs. However, at the very least it seems like this side effect should be mentioned in the documentation to avoid confusion when someone omits/forgets the package.<subpackage>.module.type hierarchy in the field definition.

    I attached a tiny patch which just jumps to PyType_Ready()'s error label if the field is NULL. I also added a comment about pydoc in the two places of the documentation I could think of where tp_name is mentioned.

    @nkoep nkoep mannequin added interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump labels Jun 23, 2013
    @amauryfa
    Copy link
    Member

    The patch is not complete: PyType_Ready() returns -1 but no no exception is set.

    @nkoep
    Copy link
    Mannequin Author

    nkoep mannequin commented Jun 28, 2013

    Oh, you're right, of course. I completely forgot that any other case which jumps to the error label assumes an appropriate exception has already been set. I attached a new patch which raises a TypeError. Is there a better way to identify the type which is affected by this exception? Since we're complaining about the missing tp_name field we obviously can't supply the name in the exception.

    @amauryfa
    Copy link
    Member

    Since this error can occur only during the development of a C extension, I would not worry too much. The traceback will already indicate the imported module, this is much better than a segfault later in pydoc.

    @superluser
    Copy link
    Mannequin

    superluser mannequin commented Jan 15, 2016

    There's still no check on tp_name. The patch looks reasonable, applies cleanly, compiles, and doesn't break any tests - suggest it be merged.

    @serhiy-storchaka serhiy-storchaka added the 3.7 (EOL) end of life label Oct 2, 2016
    @serhiy-storchaka
    Copy link
    Member

    The patch LGTM, but I think SystemError is more appropriate.

    @serhiy-storchaka serhiy-storchaka self-assigned this Oct 2, 2016
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Oct 7, 2016

    New changeset 5c459b0f2b75 by Serhiy Storchaka in branch '3.5':
    Issue bpo-18287: PyType_Ready() now checks that tp_name is not NULL.
    https://hg.python.org/cpython/rev/5c459b0f2b75

    New changeset ba76dd106e66 by Serhiy Storchaka in branch '2.7':
    Issue bpo-18287: PyType_Ready() now checks that tp_name is not NULL.
    https://hg.python.org/cpython/rev/ba76dd106e66

    New changeset 0b726193ec3c by Serhiy Storchaka in branch '3.6':
    Issue bpo-18287: PyType_Ready() now checks that tp_name is not NULL.
    https://hg.python.org/cpython/rev/0b726193ec3c

    New changeset a60d0e80cc1d by Serhiy Storchaka in branch 'default':
    Issue bpo-18287: PyType_Ready() now checks that tp_name is not NULL.
    https://hg.python.org/cpython/rev/a60d0e80cc1d

    @serhiy-storchaka
    Copy link
    Member

    Thank you Niklas for your report and patch.

    @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

    @serhiy-storchaka serhiy-storchaka

    Labels
    3.7 (EOL) end of life interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    2 participants
    @amauryfa @serhiy-storchaka