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

C API: Add PyModule_Add() function #86493

Closed
serhiy-storchaka opened this issue Nov 11, 2020 · 12 comments
Closed

C API: Add PyModule_Add() function #86493

serhiy-storchaka opened this issue Nov 11, 2020 · 12 comments
Labels
3.13 new features, bugs and security fixes topic-C-API type-feature A feature request or enhancement

Comments

@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented Nov 11, 2020

BPO 42327
Nosy @vstinner, @ambv, @serhiy-storchaka, @brandtbucher
PRs
  • bpo-42327: Add PyModule_Add(). #23240
  • bpo-42327: Add PyModule_Add() (smaller). #23443
  • [3.9] bpo-44050: Extension modules can share state when they don't support sub-interpreters. (GH-27794) #28741
  • 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 = None
    created_at = <Date 2020-11-11.20:35:14.453>
    labels = ['expert-C-API', 'type-feature', '3.10']
    title = 'Add PyModule_Add()'
    updated_at = <Date 2021-10-05.17:30:16.409>
    user = 'https://github.com/serhiy-storchaka'

    bugs.python.org fields:

    activity = <Date 2021-10-05.17:30:16.409>
    actor = 'lukasz.langa'
    assignee = 'none'
    closed = False
    closed_date = None
    closer = None
    components = ['C API']
    creation = <Date 2020-11-11.20:35:14.453>
    creator = 'serhiy.storchaka'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 42327
    keywords = ['patch']
    message_count = 9.0
    messages = ['380794', '380795', '380818', '380819', '380820', '380821', '380829', '380833', '384812']
    nosy_count = 4.0
    nosy_names = ['vstinner', 'lukasz.langa', 'serhiy.storchaka', 'brandtbucher']
    pr_nums = ['23240', '23443', '28741']
    priority = 'normal'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue42327'
    versions = ['Python 3.10']

    Linked PRs

    @serhiy-storchaka
    Copy link
    Member Author

    There is a design flaw in PyModule_AddObject(). It steals reference of its value only if the result is success. To avoid leaks it should be used in the following form:

        PyObject *tmp = <new reference>;
        if (PyModule_AddObject(name, name, tmp) < 0) {
            Py_XDECREF(tmp);
            goto error;
        }

    It is inconvenient and many code forgot to use a temporary variable and call Py_XDECREF().

    It was not intention, but it is too late to change this behavior now, because some code calls Py_XDECREF() if PyModule_AddObject() fails. Fixing PyModule_AddObject() now will break hard such code.

    There was an idea to make the change gradual, controlled by a special macro (see bpo-26871). But it still has significant risk.

    I propose to add new function PyModule_Add() which always steals reference to its argument. It is more convenient and allows to get rid of temporary variable:

        if (PyModule_Add(name, name, <new reference>) < 0) {
            goto error;
        }

    I choose name PyModule_Add because it is short, and allow to write the call in one line with moderately long expression <new reference> (like PyFloat_FromDouble(...) or PyLong_FromUnsignedLong(...)).

    @serhiy-storchaka serhiy-storchaka added 3.10 only security fixes topic-C-API type-feature A feature request or enhancement labels Nov 11, 2020
    @brandtbucher
    Copy link
    Member

    See also:

    https://bugs.python.org/issue38823

    #17298

    @vstinner
    Copy link
    Member

    Oh, I just rejected PR 17298. Copy of my message:
    ---
    I added PyModule_AddObjectRef() which uses strong references, rather than only stealing a reference on success.

    I also enhanced the documentation to show concrete examples:
    https://docs.python.org/dev/c-api/module.html#c.PyModule_AddObjectRef

    I modified a few extension to use PyModule_AddObjectRef(). Sometimes, PyModule_AddObject() is more appropriate. Sometimes, PyModule_AddObjectRef() is more appropriate. Both functions are relevant, and I don't see a clear winner.

    I agree than fixing existing code is painful, but I hope that new code using mostly PyModule_AddObjectRef() would be simpler to review. I'm not sure that it's simpler to write new code using PyModule_AddObjectRef(), since you might need more Py_DECREF() calls.

    My intent is to have more "regular" code about reference counting. See also: https://bugs.python.org/issue42294

    Since you wrote that this API is a band aid on a broken API, I consider that you are fine with rejecting it and move on to the new PyModule_AddObjectRef().

    Anyway, thanks for you attempt to make the C API less broken :-)
    ---

    I added PyModule_AddObjectRef() in bpo-163574.

    @vstinner
    Copy link
    Member

    I'm using more and more often such macro:

    #define MOD_ADD(name, expr) \
        do { \
            PyObject *obj = (expr); \
            if (obj == NULL) { \
                return -1; \
            } \
            if (PyModule_AddObjectRef(mod, name, obj) < 0) { \
                Py_DECREF(obj); \
                return -1; \
            } \
            Py_DECREF(obj); \
        } while (0)

    Would PyModule_Add() replace such macro?

    @vstinner
    Copy link
    Member

    If PyModule_Add() is added, I would suggest to rename PyModule_AddObjectRef() to PyModule_AddRef() for consistency.

    IMHO PyModule_AddObjectRef() remains useful even if PyModule_Add() is added.

    @vstinner
    Copy link
    Member

    In practice, PyModule_AddRef(mod, obj) behaves as PyModule_Add(mod, Py_NewRef(obj)) if I understood correctly.

    @serhiy-storchaka
    Copy link
    Member Author

    PyModule_Add() allows to make such macro much simpler:

    #define MOD_ADD(name, expr) \
        do { \
            if (PyModule_Add(mod, name, expr) < 0) { \
                return -1; \
            } \
        } while (0)

    PyModule_AddObjectRef() is just Py_XINCREF() followed by PyModule_Add(). But since most values added to the module are new references, Py_XINCREF() is usually not needed. The PyModule_Add* API is a convenient API. It is not necessary, you can use PyModule_GetDict() + PyDict_SetItemString(), but with this API it is easier. And PyModule_Add() is a correct PyModule_AddObject() (which was broken a long time ago and cannot be fixed now) and is more convenient than PyModule_AddObjectRef().

    PyModule_AddIntConstant() and PyModule_AddStringConstant() can be easily expressed in terms of PyModule_Add():

        PyModule_Add(m, name, PyLong_FromLong(value))
        PyModule_Add(m, name, PyUnicode_FromString(value))

    And it is easy to combine it with other functions: PyLong_FromLongLong(), PyLong_FromUnsignedLong(), PyLong_FromVoidPtr(), PyFloat_FromDouble(), PyCapsule_New(), PyType_FromSpec(), etc.

    @vstinner
    Copy link
    Member

    PyModule_AddObjectRef() is just Py_XINCREF() followed by PyModule_Add(). But since most values added to the module are new references, Py_XINCREF() is usually not needed.

    There is no general rule. I saw two main cases.

    (A) Create an object only to add it into the module. PyModule_Add() and PyModule_AddObject() are good for that case.

    Example in the array module:

        PyObject *typecodes = PyUnicode_DecodeASCII(buffer, p - buffer, NULL);
        if (PyModule_AddObject(m, "typecodes", typecodes) < 0) {
            Py_XDECREF(typecodes);
            return -1;
        }

    This code can be rewritten with PyModule_Add():

        PyObject *typecodes = PyUnicode_DecodeASCII(buffer, p - buffer, NULL);
        if (PyModule_Add(m, "typecodes", typecodes) < 0) {
            return -1;
        }

    Py_XDECREF(typecodes) is no longer needed using PyModule_Add().

    (B) Add an existing object into the module, but the objet is already stored elsewhere. PyModule_AddObjectRef() is good for that case. It became common to have this case when an object is also stored in the module state.

    Example in _ast:

        state->AST_type = PyType_FromSpec(&AST_type_spec);
        if (!state->AST_type) {
            return 0;
        }
        (...)
        if (PyModule_AddObjectRef(m, "AST", state->AST_type) < 0) {
            return -1;
        }

    state->AST_type and module attribute both hold a strong reference to the type.

    @vstinner
    Copy link
    Member

    Note for myself: I added PyModule_AddObjectRef() to https://github.com/pythoncapi/pythoncapi_compat If it's renamed, pythoncapi_compat must also be updated.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @vstinner
    Copy link
    Member

    @serhiy-storchaka: So what's the status of this API?

    @iritkatriel iritkatriel added 3.13 new features, bugs and security fixes and removed 3.10 only security fixes labels May 13, 2023
    serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Jul 15, 2023
    serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Jul 15, 2023
    serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Jul 15, 2023
    Fix _ssl, _stat, _testinternalcapi, _threadmodule, cmath, math, posix, time.
    serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Jul 15, 2023
    Fix _ssl, _stat, _testinternalcapi, _threadmodule, cmath, math, posix, time.
    @serhiy-storchaka serhiy-storchaka changed the title Add PyModule_Add() C API: Add PyModule_AddNew() function Jul 15, 2023
    @serhiy-storchaka
    Copy link
    Member Author

    I extracted some changes in two separate PRs: #106767 fixes some bugs by using PyModule_AddObjectRef(), and #106768 fixes other bugs by using new private function _PyModule_AddNew(). Both these PRs can be backported. I hope there are no objections against this. Adding a new public function PyModule_AddNew() has been left to other PR.

    serhiy-storchaka added a commit that referenced this issue Jul 18, 2023
    serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Jul 18, 2023
    …_curses_panel, _decimal, posix, xxsubtype (pythonGH-106767).
    
    (cherry picked from commit 7454923)
    
    Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
    serhiy-storchaka added a commit that referenced this issue Jul 18, 2023
    …s_panel, _decimal, posix, xxsubtype (GH-106767) (#106849)
    
    (cherry picked from commit 7454923)
    serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Jul 18, 2023
    …ation: _curses_panel, _decimal, posix, xxsubtype (pythonGH-106767) (pythonGH-106849)
    
    (cherry picked from commit 7454923).
    (cherry picked from commit 970cb8e)
    
    Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
    serhiy-storchaka added a commit that referenced this issue Jul 18, 2023
    Fix _ssl, _stat, _testinternalcapi, _threadmodule, cmath, math, posix, time.
    serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Jul 18, 2023
    …ion (pythonGH-106768)
    
    Fix _ssl, _stat, _testinternalcapi, _threadmodule, cmath, math, posix, time..
    (cherry picked from commit 3e65bae)
    
    Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
    @serhiy-storchaka serhiy-storchaka changed the title C API: Add PyModule_AddNew() function C API: Add PyModule_Add() function Jul 18, 2023
    serhiy-storchaka added a commit that referenced this issue Jul 18, 2023
    …s_panel, _decimal, posix, xxsubtype (GH-106767) (GH-106849) (GH-106851)
    
    (cherry picked from commit 7454923)
    (cherry picked from commit 970cb8e)
    serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Jul 18, 2023
    Use PyModule_Add() or PyModule_AddObjectRef() instead of soft deprecated
    PyModule_AddObject().
    serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Jul 18, 2023
    serhiy-storchaka added a commit that referenced this issue Jul 18, 2023
    …H-106768) (GH-106855)
    
    Fix _ssl, _stat, _testinternalcapi, _threadmodule, cmath, math, posix, time.
    (cherry picked from commit 3e65bae)
    serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Jul 18, 2023
    …ialization (pythonGH-106768) (pythonGH-106855)
    
    Fix _ssl, _stat, _testinternalcapi, _threadmodule, cmath, math, posix, time.
    (cherry picked from commit 3e65bae).
    (cherry picked from commit a423ddb)
    
    Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
    serhiy-storchaka added a commit that referenced this issue Jul 19, 2023
    …H-106768) (GH-106855) (GH-106863)
    
    [3.11] [3.12] gh-86493: Fix possible leaks in some modules initialization (GH-106768) (GH-106855)
    
    Fix _ssl, _stat, _testinternalcapi, _threadmodule, cmath, math, posix, time.
    (cherry picked from commit 3e65bae).
    (cherry picked from commit a423ddb)
    serhiy-storchaka added a commit that referenced this issue Jul 25, 2023
    Use PyModule_Add() or PyModule_AddObjectRef() instead of soft deprecated
    PyModule_AddObject().
    @vstinner
    Copy link
    Member

    Nice job, thanks.

    jtcave pushed a commit to jtcave/cpython that referenced this issue Jul 27, 2023
    Use PyModule_Add() or PyModule_AddObjectRef() instead of soft deprecated
    PyModule_AddObject().
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.13 new features, bugs and security fixes topic-C-API type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants