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.

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

PyType_FromSpec() should accept tp_doc=NULL #85998

Closed
vstinner opened this issue Sep 22, 2020 · 13 comments
Closed

PyType_FromSpec() should accept tp_doc=NULL #85998

vstinner opened this issue Sep 22, 2020 · 13 comments
Labels
3.10 interpreter-core

Comments

@vstinner
Copy link
Member

@vstinner vstinner commented Sep 22, 2020

BPO 41832
Nosy @vstinner, @encukou, @corona10, @shihai1991, @Fidget-Spinner
PRs
  • #23123
  • #23243
  • #25852
  • 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 2020-11-15.09:09:14.483>
    created_at = <Date 2020-09-22.10:29:31.974>
    labels = ['interpreter-core', '3.10']
    title = 'PyType_FromSpec() should accept tp_doc=NULL'
    updated_at = <Date 2021-05-03.14:06:50.277>
    user = 'https://github.com/vstinner'

    bugs.python.org fields:

    activity = <Date 2021-05-03.14:06:50.277>
    actor = 'kj'
    assignee = 'none'
    closed = True
    closed_date = <Date 2020-11-15.09:09:14.483>
    closer = 'shihai1991'
    components = ['Interpreter Core']
    creation = <Date 2020-09-22.10:29:31.974>
    creator = 'vstinner'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 41832
    keywords = ['patch']
    message_count = 13.0
    messages = ['377308', '380238', '380302', '380316', '380452', '380453', '380488', '380709', '380767', '380779', '380976', '381006', '381022']
    nosy_count = 5.0
    nosy_names = ['vstinner', 'petr.viktorin', 'corona10', 'shihai1991', 'kj']
    pr_nums = ['23123', '23243', '25852']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue41832'
    versions = ['Python 3.10']

    @vstinner
    Copy link
    Member Author

    @vstinner vstinner commented Sep 22, 2020

    When a Python type is defined in a C extension module as a static type, its documentation can be a NULL string (NULL pointer). But PyType_FromSpec() does crash if tp_doc is NULL, since this change:

    commit 032400b
    Author: Georg Brandl <georg@python.org>
    Date: Sat Feb 19 21:47:02 2011 +0000

    bpo-11249: in PyType_FromSpec, copy tp_doc slot since it usually will point to a static string literal which should not be deallocated together with the type.
    
    (...)
    diff --git a/Objects/typeobject.c b/Objects/typeobject.c
    index e9c7591b81..b1fe44ebe4 100644
    --- a/Objects/typeobject.c
    +++ b/Objects/typeobject.c
    @@ -2347,6 +2347,17 @@ PyObject* PyType_FromSpec(PyType_Spec *spec)
                goto fail;
            }
            *(void**)(res_start + slotoffsets[slot->slot]) = slot->pfunc;
    +
    +        /* need to make a copy of the docstring slot, which usually
    +           points to a static string literal */
    +        if (slot->slot == Py_tp_doc) {
    +            ssize_t len = strlen(slot->pfunc)+1;
    +            char *tp_doc = PyObject_MALLOC(len);
    +            if (tp_doc == NULL)
    +               goto fail;
    +            memcpy(tp_doc, slot->pfunc, len);
    +            res->ht_type.tp_doc = tp_doc;
    +        }
         }
     
         return (PyObject*)res;

    I propose to accept tp_doc=NULL in PyType_FromSpec(), as we do in static types.

    I noticed this difference while reviewing PR 22220 which convert _lsprof extension static types to heap types.

    @vstinner vstinner added 3.10 interpreter-core labels Sep 22, 2020
    @shihai1991
    Copy link
    Member

    @shihai1991 shihai1991 commented Nov 2, 2020

    I will take a look in this weekend :)

    @encukou
    Copy link
    Member

    @encukou encukou commented Nov 3, 2020

    The PyType_Slot documentation says that the pointer may not be NULL: https://docs.python.org/3/c-api/type.html?highlight=pytype_fromspec#c.PyType_Slot.PyType_Slot.pfunc

    If you change this, why do it only for tp_doc, but for all the slots? NULL should *always* mean that the slot is set to NULL instead of inherited. (Except maybe in cases where this is dangerous; then it should result in an error?)
    See: https://bugs.python.org/issue26979

    If you want to only change this for tp_doc, please also update the PyType_Slot documentation to cover the exception.

    @shihai1991
    Copy link
    Member

    @shihai1991 shihai1991 commented Nov 4, 2020

    If you change this, why do it only for tp_doc, but for all the slots? NULL should *always* mean that the slot is set to NULL instead of inherited. (Except maybe in cases where this is dangerous; then it should result in an error?

    IMO, it's a proper user case.
    From bpo-26978: "I'll also try making an explicit {Py_tp_dealloc, NULL} override the inherited value, as per Serhiy's suggestion."
    Petr, do you have a PR about it?

    If you want to only change this for tp_doc, please also update the PyType_Slot documentation to cover the exception.

    Copy that, I will update it soon.

    @vstinner
    Copy link
    Member Author

    @vstinner vstinner commented Nov 6, 2020

    New changeset 88c2cfd by Hai Shi in branch 'master':
    bpo-41832: PyType_FromModuleAndSpec() now accepts NULL tp_doc (GH-23123)
    88c2cfd

    @vstinner
    Copy link
    Member Author

    @vstinner vstinner commented Nov 6, 2020

    Thanks Hai Shi.

    @shihai1991
    Copy link
    Member

    @shihai1991 shihai1991 commented Nov 7, 2020

    Thank you all:)

    @encukou
    Copy link
    Member

    @encukou encukou commented Nov 10, 2020

    The PR entirely removed the note that a slot's value "May not be NULL".
    In fact, only tp_doc may be NULL. AFAIK, the restriction is still there for other slots.
    Can that note be added back?

    @encukou encukou reopened this Nov 10, 2020
    @encukou encukou reopened this Nov 10, 2020
    @shihai1991
    Copy link
    Member

    @shihai1991 shihai1991 commented Nov 11, 2020

    The PR entirely removed the note that a slot's value "May not be NULL".
    In fact, only tp_doc may be NULL. AFAIK, the restriction is still there > for other slots.
    Can that note be added back?

    May not be NULL means everthing is possible, so I decided removed it:(
    Before, I updated an old commit in PR23123 to describe this fact in cca7bc7.

    @encukou
    Copy link
    Member

    @encukou encukou commented Nov 11, 2020

    May not be NULL means everthing is possible

    No, it does not. It only means a slot's value may not be NULL, which is an important difference between slots and entries in the PyTypeObject structure.

    It's OK that it's mentioned elsewhere, but can we please put it back in the docs of PyType_Slot?

    @encukou
    Copy link
    Member

    @encukou encukou commented Nov 14, 2020

    New changeset 2b39da4 by Hai Shi in branch 'master':
    bpo-41832: Restore note about NULL in PyType_Slot.pfunc (GH-23243)
    2b39da4

    @shihai1991
    Copy link
    Member

    @shihai1991 shihai1991 commented Nov 15, 2020

    Hi, petr. If there is no other doubt about this bpo, I suggest close this bpo.

    @vstinner
    Copy link
    Member Author

    @vstinner vstinner commented Nov 15, 2020

    Sorry Petr, I didn't notice that the note was fully removed in the first change. Thanks for restoring the note!

    @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
    Labels
    3.10 interpreter-core
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants