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

Simplify hashlib implementation #82313

Closed
tiran opened this issue Sep 12, 2019 · 7 comments
Closed

Simplify hashlib implementation #82313

tiran opened this issue Sep 12, 2019 · 7 comments
Assignees
@tiran
Labels
3.8 only security fixes 3.9 only security fixes extension-modules C modules in the Modules dir type-feature A feature request or enhancement

Comments

@tiran
Copy link
Member

tiran commented Sep 12, 2019

BPO 38132
Nosy @gpshead, @tiran, @matrixise, @stratakis, @miss-islington
PRs
  • bpo-38132: Simplify _hashopenssl code #16023
  • [3.8] bpo-38132: Simplify _hashopenssl code (GH-16023) #16040
  • bpo-38132: Check EVP_DigestUpdate for error #16041
  • [3.8] bpo-38132: Check EVP_DigestUpdate for error (GH-16041) #16048
    		•

    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 = None
created_at = <Date 2019-09-12.10:44:02.780>
labels = ['extension-modules', 'type-feature', '3.8', '3.9']
title = 'Simplify hashlib implementation'
updated_at = <Date 2019-09-12.13:50:49.858>
user = 'https://github.com/tiran'

    bugs.python.org fields:

    activity = <Date 2019-09-12.13:50:49.858>
actor = 'miss-islington'
assignee = 'christian.heimes'
closed = False
closed_date = None
closer = None
components = ['Extension Modules']
creation = <Date 2019-09-12.10:44:02.780>
creator = 'christian.heimes'
dependencies = []
files = []
hgrepos = []
issue_num = 38132
keywords = ['patch']
message_count = 6.0
messages = ['352099', '352100', '352146', '352151', '352160', '352164']
nosy_count = 5.0
nosy_names = ['gregory.p.smith', 'christian.heimes', 'matrixise', 'cstratak', 'miss-islington']
pr_nums = ['16023', '16040', '16041', '16048']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue38132'
versions = ['Python 3.8', 'Python 3.9']
    @tiran
    Copy link
    Member Author

    tiran commented Sep 12, 2019

    The hashlib module uses complicated macros and some pointless optimization. The code can be simplified to make maintenance easier. Cleanup also makes it easier to get rid of static global state and to add "usedforsecurity" feature.

    • The EVPobject contains a PyObject* with the name of the hashing algorithm as PyUnicode object. The name is rarely used and can be efficiently calculated from a const char* of the EVP_MD_CTX.

    • The module caches pre-generated EVP_MD_CTX objects for commonly used hashes like sha256. Tests with timeit has shown that generating a EVP_MD_CTX from a EVP constructor is as fast as copying and reinitializing a EVP_MD_CTX.

    • The pre-calculated constructs can be replaced with argument clinic to make the code more readable.

    @tiran tiran added 3.8 only security fixes 3.9 only security fixes labels Sep 12, 2019
    @tiran tiran self-assigned this Sep 12, 2019
    @tiran tiran added extension-modules C modules in the Modules dir type-feature A feature request or enhancement labels Sep 12, 2019
    @stratakis
    Copy link
    Mannequin

    stratakis mannequin commented Sep 12, 2019

    Yes that would be awesome!

    Indeed hashlib has been a bit of a pain to work with especially with FIPS related modifications, simplifying it would help a ton.

    @matrixise
    Copy link
    Member

    New changeset 5a4f82f by Stéphane Wirtel (Christian Heimes) in branch 'master':
    bpo-38132: Simplify _hashopenssl code (GH-16023)
    5a4f82f

    @matrixise
    Copy link
    Member

    New changeset 67b90a0 by Stéphane Wirtel (Miss Islington (bot)) in branch '3.8':
    bpo-38132: Simplify _hashopenssl code (GH-16023) (bpo-16040)
    67b90a0

    @matrixise
    Copy link
    Member

    New changeset 8c74574 by Stéphane Wirtel (Christian Heimes) in branch 'master':
    bpo-38132: Check EVP_DigestUpdate for error (GH-16041)
    8c74574

    @miss-islington
    Copy link
    Contributor

    New changeset 0d7cb5b by Miss Islington (bot) in branch '3.8':
    bpo-38132: Check EVP_DigestUpdate for error (GH-16041)
    0d7cb5b

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @erlend-aasland
    Copy link
    Contributor

    AFAICS, all three suggested items was implemented by gh-16023:

    • The EVPobject name is now fetched from the EVP_MD_CTX on demand
    • The pre-generated static objects were removed from the extension module
    • The extension module was adapted to use Argument Clinic

    Closing as completed.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees

    @tiran tiran

    Labels
    3.8 only security fixes 3.9 only security fixes extension-modules C modules in the Modules dir type-feature A feature request or enhancement
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    4 participants
    @matrixise @tiran @erlend-aasland @miss-islington