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_FromSpec does not copy the name #89478

Closed
seberg mannequin opened this issue Sep 28, 2021 · 6 comments
Closed

PyType_FromSpec does not copy the name #89478

seberg mannequin opened this issue Sep 28, 2021 · 6 comments
Labels
3.11 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump

Comments

@seberg
Copy link
Mannequin

seberg mannequin commented Sep 28, 2021

BPO 45315
Nosy @encukou, @seberg, @shihai1991
PRs
  • bpo-45315: PyType_FromSpec: Copy spec->name and have the type own the memory for its name #29103
    		•

    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 2021-10-21.10:04:47.123>
created_at = <Date 2021-09-28.20:45:06.803>
labels = ['interpreter-core', 'type-crash', '3.11']
title = '`PyType_FromSpec` does not copy the name'
updated_at = <Date 2021-10-21.10:04:47.122>
user = 'https://github.com/seberg'

    bugs.python.org fields:

    activity = <Date 2021-10-21.10:04:47.122>
actor = 'petr.viktorin'
assignee = 'none'
closed = True
closed_date = <Date 2021-10-21.10:04:47.123>
closer = 'petr.viktorin'
components = ['Interpreter Core']
creation = <Date 2021-09-28.20:45:06.803>
creator = 'seberg'
dependencies = []
files = []
hgrepos = []
issue_num = 45315
keywords = ['patch']
message_count = 6.0
messages = ['402804', '403347', '403734', '404493', '404577', '404578']
nosy_count = 3.0
nosy_names = ['petr.viktorin', 'seberg', 'shihai1991']
pr_nums = ['29103']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'crash'
url = 'https://bugs.python.org/issue45315'
versions = ['Python 3.11']
    @seberg
    Copy link
    Mannequin Author

    seberg mannequin commented Sep 28, 2021

    As noted in the issue: https://bugs.python.org/issue15870#msg402800 PyType_FromSpec assumes that the name passed is persistent for the program lifetime. This seems wrong/unnecessary: We are creating a heap-type, a heap-type's name is stored as a unicode object anyway!

    So, there is no reason to require the string be persistent, and a program that decides to build a custom name dynamically (for whatever reasons) would run into a crash when the name is accessed.

    The code:

    cpython/Objects/typeobject.c

    Line 3427 in 0c50b8c

    type->tp_name = spec->name;

    Should be modified to point to to the ht_name (utf8) data instead. My guess is, the simplest solution is calling type_set_name, even if that runs some unnecessary checks.

    I understand that the FromSpec API is mostly designed to replace the static type definition rather than extend it, but it seems an unintentional/strange requirement.

    @seberg seberg mannequin added 3.11 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump labels Sep 28, 2021
    @shihai1991
    Copy link
    Member

    the simplest solution is calling type_set_name, even if that runs some unnecessary checks.

    Hm, I haven't find any case who use dynamical tp_name of Type_Spec temporarily.
    If we meet this user case, Adding a new object pointer of type name in is an other option maybe:)

    @encukou
    Copy link
    Member

    encukou commented Oct 12, 2021

    Hm, I haven't find any case who use dynamical tp_name of Type_Spec temporarily.

    The use case is creating types entirely on demand, with dynamically created specs.
    This is useful e.g. for wrapping objects of a different object system, like C++ classes or Qt/GTK widgets.

    But this issue is not about adding new API to enable a new use case. The *current* API can reasonably be used with a dynamic name string. So we shouldn't wait until someone tells us they need this fixed :)

    So this should either be fixed or the requirement should be documented. (And the documentation would IMO sound like we're acknowledging a design bug, so it's better to fix.)

    @encukou
    Copy link
    Member

    encukou commented Oct 20, 2021

    the simplest solution is calling type_set_name, even if that runs some unnecessary checks.

    Unfortunately this won't work, because it sets ht_name to the same value as tp_name. For historical reasons, the two can be different (and often are) for types created with PyType_From*Spec*.

    I haven't found a way to fix this issue without adding yet another pointer to the PyHeapTypeObject struct.

    @encukou
    Copy link
    Member

    encukou commented Oct 21, 2021

    New changeset 8a310dd by Petr Viktorin in branch 'main':
    bpo-45315: PyType_FromSpec: Copy spec->name and have the type own the memory for its name (GH-29103)
    8a310dd

    @encukou
    Copy link
    Member

    encukou commented Oct 21, 2021

    Since the fix changes the size of PyObject, it can't be backported.

    @encukou encukou closed this as completed Oct 21, 2021
    @encukou encukou closed this as completed Oct 21, 2021
    @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.11 only security fixes 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
    @encukou @shihai1991