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

Jump to bottom

Convert _hashlib to PEP 489 multiphase initialization #84848

Closed
vstinner opened this issue May 18, 2020 · 6 comments
Closed

Convert _hashlib to PEP 489 multiphase initialization #84848

vstinner opened this issue May 18, 2020 · 6 comments
Assignees
@tiran
Labels
3.9 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@vstinner
Copy link
Member

BPO 40671
Nosy @vstinner, @tiran, @encukou, @corona10, @miss-islington, @shihai1991
PRs
  • bpo-40671: Prepare _hashlib for PEP 489 (GH-20180) #20180
  • [3.9] bpo-40671: Prepare _hashlib for PEP 489 (GH-20180) #20378
  • [3.9] bpo-40671: Prepare _hashlib for PEP 489 (GH-20180) #20381
    		•

    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/tiran'
closed_at = <Date 2020-10-19.17:04:31.425>
created_at = <Date 2020-05-18.13:07:20.001>
labels = ['type-feature', 'library', '3.9']
title = 'Convert _hashlib to PEP 489 multiphase initialization'
updated_at = <Date 2020-10-19.17:04:31.425>
user = 'https://github.com/vstinner'

    bugs.python.org fields:

    activity = <Date 2020-10-19.17:04:31.425>
actor = 'corona10'
assignee = 'christian.heimes'
closed = True
closed_date = <Date 2020-10-19.17:04:31.425>
closer = 'corona10'
components = ['Library (Lib)']
creation = <Date 2020-05-18.13:07:20.001>
creator = 'vstinner'
dependencies = []
files = []
hgrepos = []
issue_num = 40671
keywords = ['patch']
message_count = 6.0
messages = ['369211', '369216', '369221', '369229', '369862', '369874']
nosy_count = 6.0
nosy_names = ['vstinner', 'christian.heimes', 'petr.viktorin', 'corona10', 'miss-islington', 'shihai1991']
pr_nums = ['20180', '20378', '20381']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue40671'
versions = ['Python 3.9']
    @vstinner
    Copy link
    Member Author

    In Modules/_hashopenssl.c:PyInit__hashlib(), PyModule_AddObject() can be replaced with PyModule_AddType() to add the 3 types: this function does the Py_INCREF for us, which reduces errors if the function fails.

    The results of other PyModule_AddObject() calls should also be checked to handle errors.

    The following _hashlibstate_global macro is no longer used:

    #define _hashlibstate_global ((_hashlibstate *)PyModule_GetState(PyState_FindModule(&_hashlibmodule)))

    EVPnew() got a module parameter, so it doesn't need _hashlibstate_global anymore.

    Dong-hee, Hai: Maybe it's time to convert this module to PEP-489 multiphase initialization?

    Hai: do you want to work on a PR?

    --

    Question: do EVPobject and HMACobject rely on a global state somewhere in OpenSSL? Or is it safe to use them in subinterpreters, with multiple instances of _hashlib?

    EVPobject and HMACobject have "lock" member.

    Even if converting the module to PEP-489 is not enough to make it compatible with subinterpreters (I don't know, maybe it's enought, see my question), it should help for that :-)

    Note: the current code is already quite clean, I like it :-)

    cc Petr & Christian.

    @vstinner vstinner added 3.9 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels May 18, 2020
    @tiran
    Copy link
    Member

    tiran commented May 18, 2020

    OpenSSL's EVP and HMAC API depend on global state in OpenSSL, but they are still safe to be used in subinterpreters. OpenSSL ensures that the internal data structures are initialized in a thread-safe manner. The internal state cannot leak across subinterpreters. OpenSSL has process global configuration, but Python's ssl and hashlib module do not expose these features.

    The Python wrappers EVPobject, EVPXOFobject and HMACobject are **not** safe to be transfered across subinterpreters. The lock shouldn't matter here. It's used to release the GIL and block other threads from feeding into an object at the same time.

    @vstinner
    Copy link
    Member Author

    The Python wrappers EVPobject, EVPXOFobject and HMACobject are **not** safe to be transfered across subinterpreters.

    No Python object must be shared between two interpreter and no object should be transfered form one interpreter to another. That's not specific to _hashlib, but a general guidelines for subinterpreters ;-)

    Converting _hashlib should allow to have one _hashlib module instance per interpreter ;-) (Rather than sharing the same instance between all interpereters which caused so many issues!)

    @shihai1991
    Copy link
    Member

    Hai: do you want to work on a PR?

    Oh, Looks like I am late, Christian have crated a PR ;)

    @tiran
    Copy link
    Member

    tiran commented May 25, 2020

    New changeset 20c22db by Christian Heimes in branch 'master':
    bpo-40671: Prepare _hashlib for PEP-489 (GH-20180)
    20c22db

    @miss-islington
    Copy link
    Contributor

    New changeset 1fe1a14 by Miss Islington (bot) in branch '3.9':
    bpo-40671: Prepare _hashlib for PEP-489 (GH-20180)
    1fe1a14

    @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

    @tiran tiran

    Labels
    3.9 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    5 participants
    @vstinner @tiran @corona10 @shihai1991 @miss-islington