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

bpo-38142: Updated _hashopenssl.c to be PEP 384 compliant #16071

Merged
merged 4 commits into from
Sep 25, 2019

Conversation

tiran
Copy link
Member

@tiran tiran commented Sep 12, 2019

The _hashlib OpenSSL wrapper extension module is now PEP 384 compliant.

Also remove refleak test from test_hashlib. The updated type no longer accepts random arguments to __init__.

https://bugs.python.org/issue38142

Copy link
Contributor

@eduardo-elizondo eduardo-elizondo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since EVP is now a heap allocated type, you should now properly refcount the type as well. You can read the details in: https://docs.python.org/3.8/whatsnew/3.8.html#changes-in-the-c-api, search for bpo-35810

Anyways, the TL;DR is that EVP_dealloc should now be:

static void
EVP_dealloc(EVPobject *self)
{
    PyTypeObject *tp = Py_TYPE(self);
    if (self->lock != NULL)
        PyThread_free_lock(self->lock);
    EVP_MD_CTX_free(self->ctx);
    PyObject_Del(self);
    Py_DECREF(tp);
}

@tiran tiran force-pushed the bpo-38142-hashlib-mod branch from 945e8e2 to aa25666 Compare September 13, 2019 16:10
@tiran
Copy link
Member Author

tiran commented Sep 13, 2019

Since EVP is now a heap allocated type, you should now properly refcount the type as well. You can read the details in: docs.python.org/3.8/whatsnew/3.8.html#changes-in-the-c-api, search for bpo-35810

Thanks! I missed the note. :)

Copy link
Contributor

@eduardo-elizondo eduardo-elizondo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect! This LGTM now!

@eduardo-elizondo
Copy link
Contributor

Agh I forget I don't have the commit bit here. Let's ping someone else to merge, cc @ericsnowcurrently @DinoV

Signed-off-by: Christian Heimes <christian@python.org>
The updated type no longer accepts random arguments to __init__.

Signed-off-by: Christian Heimes <christian@python.org>
Signed-off-by: Christian Heimes <christian@python.org>
Signed-off-by: Christian Heimes <christian@python.org>
@tiran tiran force-pushed the bpo-38142-hashlib-mod branch from 6e95139 to a4b8323 Compare September 25, 2019 15:34
@tiran tiran merged commit df69e75 into python:master Sep 25, 2019
@tiran tiran deleted the bpo-38142-hashlib-mod branch September 25, 2019 21:03
@bedevere-bot
Copy link

@tiran: Please replace # with GH- in the commit message next time. Thanks!

miss-islington pushed a commit that referenced this pull request Sep 27, 2019
…H-16248)

As mentioned in the bpo ticket, this mistake came up on two reviews:
- #16127 (review)
- #16071 (review)

Would be nice to have it documented in a more permanent place than 3.8's whatsnew entry.


https://bugs.python.org/issue38206



Automerge-Triggered-By: @encukou
jacobneiltaylor pushed a commit to jacobneiltaylor/cpython that referenced this pull request Dec 5, 2019
* Updated _hashopenssl.c to be PEP 384 compliant
* Remove refleak test from test_hashlib. The updated type no longer accepts random arguments to __init__.
jacobneiltaylor pushed a commit to jacobneiltaylor/cpython that referenced this pull request Dec 5, 2019
…ythonGH-16248)

As mentioned in the bpo ticket, this mistake came up on two reviews:
- python#16127 (review)
- python#16071 (review)

Would be nice to have it documented in a more permanent place than 3.8's whatsnew entry.


https://bugs.python.org/issue38206



Automerge-Triggered-By: @encukou
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants