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

SSL: remove multi-OpenSSL sanity check. #3109

Merged
merged 2 commits into from May 31, 2017

Conversation

@mkrautz
Copy link
Member

commented May 31, 2017

This PR removes the sanity check for multiple copies of OpenSSL.

It also ensures Mumble always initializes its own copy of OpenSSL, even when Mumble and Qt are using distinct versions of OpenSSL.

See commit messages for more information.

mkrautz added 2 commits May 31, 2017
SSL: also initialize Mumble's copy of libssl/libcrypto.
Instead of only relying on Qt to initialize its copy of
OpenSSL, ensure Mumble also initializes its copy.

This is obviously only relevant if Mumble links against one
copy, and Qt dlopens another copy.
SSL: remove qsslSanityCheck.
Many distros are now shipping OpenSSL 1.1.

However, Qt 5 only supports OpenSSL 1.0 at present.
A Qt Project changeset implementing OpenSSL 1.1 support
is currently slated for Qt 5.10.

This leaves us in a situation where we're inevitably going
to be in situation where Mumble and Qt will be forced to use
different versions of OpenSSL on most Linux systems.

The previous commit in this PR has added proper initialization
to Mumble's copy of OpenSSL, instead of relying on Qt's only
initialization, which only works if Mumble and Qt are using the same
copy. This should fix the crashes people have reported when trying to
use Mumble/Murmur in a configuration where Mumble and Qt each use
their own copy of OpenSSL.

I'm still wary of allowing this, but it seems like the cleanest
approach.

An alternative would be to have Mumble/Murmur to try and
dynamically look up the symbols they need to run. However, we don't
really have the proper infrastructure to suport runtime-loaded
dependencies on Unix-like systems. Nor is it something we've done
in the past. Also, using OpenSSL 1.1 headers against OpenSSL 1.0
might also prove problematic anyway.

@mkrautz mkrautz requested a review from davidebeatrici May 31, 2017

@davidebeatrici
Copy link
Member

left a comment

Working fine on Debian Stretch.

@mkrautz mkrautz merged commit 13bc12d into mumble-voip:master May 31, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.