classification
Title: Convert _hashlib to PEP 489 multiphase initialization
Type: enhancement Stage: patch review
Components: Library (Lib) Versions: Python 3.9
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: christian.heimes Nosy List: christian.heimes, corona10, miss-islington, petr.viktorin, shihai1991, vstinner
Priority: normal Keywords: patch

Created on 2020-05-18 13:07 by vstinner, last changed 2020-05-25 12:19 by miss-islington.

Pull Requests
URL Status Linked Edit
PR 20180 merged christian.heimes, 2020-05-18 13:52
PR 20378 closed miss-islington, 2020-05-25 08:45
PR 20381 merged miss-islington, 2020-05-25 11:57
Messages (6)
msg369211 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-05-18 13:07
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.
msg369216 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2020-05-18 13:25
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.
msg369221 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-05-18 13:52
> 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!)
msg369229 - (view) Author: hai shi (shihai1991) * Date: 2020-05-18 14:17
> Hai: do you want to work on a PR?

Oh, Looks like I am late, Christian have crated a PR ;)
msg369862 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2020-05-25 08:44
New changeset 20c22db602bf2a51f5231433b9054290f8069b90 by Christian Heimes in branch 'master':
bpo-40671: Prepare _hashlib for PEP 489 (GH-20180)
https://github.com/python/cpython/commit/20c22db602bf2a51f5231433b9054290f8069b90
msg369874 - (view) Author: miss-islington (miss-islington) Date: 2020-05-25 12:19
New changeset 1fe1a14703001ec8076412ca28a3fbdf1f5c0735 by Miss Islington (bot) in branch '3.9':
bpo-40671: Prepare _hashlib for PEP 489 (GH-20180)
https://github.com/python/cpython/commit/1fe1a14703001ec8076412ca28a3fbdf1f5c0735
History
Date User Action Args
2020-05-25 12:19:08miss-islingtonsetmessages: + msg369874
2020-05-25 11:57:29miss-islingtonsetpull_requests: + pull_request19645
2020-05-25 08:45:05miss-islingtonsetnosy: + miss-islington
pull_requests: + pull_request19641
2020-05-25 08:44:55christian.heimessetmessages: + msg369862
2020-05-18 14:17:11shihai1991setmessages: + msg369229
2020-05-18 13:52:59vstinnersetmessages: + msg369221
2020-05-18 13:52:29christian.heimessetkeywords: + patch
stage: needs patch -> patch review
pull_requests: + pull_request19479
2020-05-18 13:25:23christian.heimessetmessages: + msg369216
2020-05-18 13:13:35christian.heimessetassignee: christian.heimes
stage: needs patch
2020-05-18 13:07:20vstinnercreate