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

Make sure libcrypto threadsafety callbacks are properly set up #290

Closed
wants to merge 1 commit into from

Conversation

wulczer
Copy link
Contributor

@wulczer wulczer commented Feb 17, 2015

Multithreaded programs using libcrypto (part of OpenSSL) need to set up
callbacks to ensure safe execution. Both Python and libpq set up those
callbacks, which might lead to a conflict.

To avoid leaving dangling function pointers when being unloaded, libpq sets up
and removes the callbacks every time a SSL connection it opened and closed. If
another Python thread is performing unrelated SSL operations (like connecting
to a HTTPS server), this might lead to deadlocks, as described in
http://www.postgresql.org/message-id/871tlzrlkq.fsf@wulczer.org

Even if the problem will be remediated in libpq, it's still useful to have it
fixed in psycopg2. The solution is to use Python's own libcrypto callbacks and
completely disable handling them in libpq.

Multithreaded programs using libcrypto (part of OpenSSL) need to set up
callbacks to ensure safe execution. Both Python and libpq set up those
callbacks, which might lead to a conflict.

To avoid leaving dangling function pointers when being unloaded, libpq sets up
and removes the callbacks every time a SSL connection it opened and closed. If
another Python thread is performing unrelated SSL operations (like connecting
to a HTTPS server), this might lead to deadlocks, as described in
http://www.postgresql.org/message-id/871tlzrlkq.fsf@wulczer.org

Even if the problem will be remediated in libpq, it's still useful to have it
fixed in psycopg2. The solution is to use Python's own libcrypto callbacks and
completely disable handling them in libpq.
@wulczer
Copy link
Contributor Author

wulczer commented Feb 17, 2015

Here's a script to reproduce deadlock when one Python thread is doing libpq over SSL and another is doing unrelated SSL:

https://gist.github.com/wulczer/82b5b52a420ac7387694

Apart from the discussion in the linked pg-hackers thread these threads are relevant:

http://www.postgresql.org/message-id/CAHUL3dpWYFnUgdgo95OHYDQ4kugdnBKPTjq0mNbTuBhCMG4xvQ@mail.gmail.com

http://www.postgresql.org/message-id/48620925.6070806@pws.com.au

@fogzot
Copy link
Member

fogzot commented Feb 17, 2015

Patch seems fine to me. I don't have an environment to run the tests but I'll do that and merge tomorrow unless @dvarrazzo beats me to it. :D

@dvarrazzo
Copy link
Member

I'm totally fine with the patch (thank you Jan!) but I prefer to apply it to my branch, where I can make the tests run, and then merge it to master.

@wulczer
Copy link
Contributor Author

wulczer commented Feb 17, 2015

Sure thing! Feel free to close the PR and apply the patch via different means :)

@dvarrazzo
Copy link
Member

It's fine as it is: I don't really like the way github makes the merge (with an extra commit) but it detects automatically if the changeset is pushed "manually" and closes the requests itself. We can leave everything as it is now ;-)

@wulczer
Copy link
Contributor Author

wulczer commented Mar 30, 2015

Hi! Just checking in, where are we on this?

@dvarrazzo
Copy link
Member

Right, I forgot to include this in the last release :\ sorry, I only got through the bug list and skipped the pull request.

I'll try to release it soon in a 2.6.1.

@wulczer
Copy link
Contributor Author

wulczer commented Mar 31, 2015

No problem at all, I actually submitted this after 2.6 got released.

Just wanted to make sure it doesn't fall through the cracks, thanks!

@dvarrazzo
Copy link
Member

Absolutely no, thank you: it must have been a tricky nut to crack!

@dvarrazzo dvarrazzo added this to the psycopg 2.6.1 milestone Mar 31, 2015
@dvarrazzo
Copy link
Member

Merged, thank you very much.

@dvarrazzo dvarrazzo closed this May 3, 2015
@wulczer
Copy link
Contributor Author

wulczer commented May 4, 2015

Yay! Thank you!

@wulczer wulczer deleted the setup-libcrypto-locking branch May 4, 2015 06:03
@bhagany
Copy link

bhagany commented Jun 12, 2015

Is there any chance of getting this into a stable release soon? We get bitten by this relatively frequently.

@dvarrazzo
Copy link
Member

I was aiming for a release the past week but I couldn't. I'll try to have it out before Monday.

@bhagany
Copy link

bhagany commented Jun 12, 2015

Great! Thanks very much.

@dvarrazzo
Copy link
Member

Still have to write the release notes to -announce but 2.6.1 is already tagged on github and released on PyPI.

@bhagany
Copy link

bhagany commented Jun 15, 2015

Thanks!

@KarelJakubec
Copy link

@wulczer I was happy as fuck when I saw this issue - debugging our app for two days, strange freezes in connect, thinking I am the only one who is expereincing it. Seeking for some wrong initialization of SSL and in the end, all I needed was to update the psycopg2. :)

Thanks a lot, you just made my day.

@wulczer
Copy link
Contributor Author

wulczer commented Jun 29, 2015

@KarelJakubec Yeah! I love the sound of bugs squashed in the morning 🤘

@danhnguyen0902
Copy link

You are my savior, @wulczer
I have been tracking down the crashes on my server for almost a month, and I came across this issue. All I did was to update psycopg2 to the latest, and no crashes since then.

@wulczer
Copy link
Contributor Author

wulczer commented Jul 23, 2015

Glad to help @danhnguyen0902 :)

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.

None yet

6 participants