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 locking callback #3226

Merged
merged 12 commits into from
Nov 13, 2016
Merged

C locking callback #3226

merged 12 commits into from
Nov 13, 2016

Conversation

alex
Copy link
Member

@alex alex commented Nov 8, 2016

No description provided.

aglasgall and others added 2 commits October 18, 2016 13:30
The Python OpenSSL locking callback is unsafe; if GC is triggered
during the callback's invocation, it can result in the callback being
invoked reentrantly, which can lead to deadlocks. This patch replaces
it with one in C that gets built at compile time via cffi along with
the rest of the OpenSSL binding.
@mention-bot
Copy link

@alex, thanks for your PR! By analyzing the history of the files in this pull request, we identified @public, @tiran and @Ayrx to be potential reviewers.

@reaperhulk
Copy link
Member

jenkins, retest this please.

Copy link
Member

@reaperhulk reaperhulk left a comment

Choose a reason for hiding this comment

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

Should we try to fail gracefully on pythons with no threading? I think I'd rather do it this way and see what happens. Still need to sort out the licensing before we can merge this though.

@reaperhulk reaperhulk added this to the Sixteenth Release milestone Nov 8, 2016
@alex
Copy link
Member Author

alex commented Nov 8, 2016

I don't think the previous implementation failed gracefully in such cases.

On Tue, Nov 8, 2016 at 12:47 AM, Paul Kehrer notifications@github.com
wrote:

@reaperhulk commented on this pull request.

Should we try to fail gracefully on pythons with no threading? I think I'd
rather do it this way and see what happens. Still need to sort out the
licensing before we can merge this though.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#3226 (review),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAADBJL9EtINWGS-L9Iiig86Jhgnl1K7ks5q8A0HgaJpZM4Kr88-
.

"I disapprove of what you say, but I will defend to the death your right to
say it." -- Evelyn Beatrice Hall (summarizing Voltaire)
"The people's good is the highest law." -- Cicero
GPG Key fingerprint: D1B3 ADC0 E023 8CA6

int _setup_ssl_threads(void) {
unsigned int i;

if (_ssl_locks == NULL) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to check for CRYPTO_get_locking_callback() == NULL as well?

Copy link
Member

Choose a reason for hiding this comment

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

It looks like in this version we call __import__("_ssl") but then we register our callback anyway. We should check to see if the callback is already registered before we register anything.

Copy link
Member Author

Choose a reason for hiding this comment

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

The if cls.lib.CRYPTO_get_locking_callback() != cls.ffi.NULL is still there.

Copy link
Member

Choose a reason for hiding this comment

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

Sigh, the problems with just looking at diffs. Yep, this is fine then.

@reaperhulk reaperhulk mentioned this pull request Nov 11, 2016
4 tasks
@@ -1,3 +1,5 @@
# -*- coding: utf8 -*-
Copy link
Member

Choose a reason for hiding this comment

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

This has unicode in it?

Copy link
Member Author

Choose a reason for hiding this comment

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

The copyright symbol. I copied it directly out o the license file.

@alex
Copy link
Member Author

alex commented Nov 13, 2016

Jenkins, retest this please.

@reaperhulk reaperhulk merged commit d862933 into pyca:master Nov 13, 2016
@alex alex deleted the c-locking-callback branch November 13, 2016 20:55
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants