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

Fix SSL detection. #147

Closed
wants to merge 4 commits into from
Closed

Fix SSL detection. #147

wants to merge 4 commits into from

Conversation

comel
Copy link

@comel comel commented Jan 10, 2014

SSL detection is broken in 7.19.3 on some systems, because curl-config --libs doesn't display all depended libraries (e.g. ssl) in all cases, see REQUIRE_LIB_DEPS in https://github.com/bagder/curl/blob/master/curl-config.in.

So, for SSL detection curl-config --libs --static-libs should be used. Also, HAVE_CURL_SSL should be defined only when one of the SSL libraries is detected, because:

"libcurl was compiled with SSL support, but configure could not determine which library was used; thus no SSL crypto locking callbacks will be set, which may cause random crashes on SSL requests"

i.e. in most cases you get library which doesn't work with SSL requests.

@p
Copy link
Member

p commented Jan 10, 2014

Unconditionally using --libs --static-libs together was removed in #52. Apparently on some systems those two options together simply don't work, as opposed to do not accurately identify used SSL library.

Secondly, if libcurl was built with --disable-static then asking curl-config for static libs results in an error. If --static-libs is used at all it would have to be a separate invocation.

Furthermore, I do not understand what giving --libs --static-libs together is supposed to mean. I thought those options were mutually exclusive.

Here is what I would suggest. libcurl does not currently expose which ssl library it is built against in a convenient manner. The SSL feature only says whether some ssl library is used. So please send a feature request to libcurl to have a curl-config option to say which ssl library is used, which pycurl would then be able to trivially use.

@comel
Copy link
Author

comel commented Jan 10, 2014

As you can see at https://github.com/bagder/curl/blob/master/curl-config.in options --libs and --static-libs are not mutually exclusive, i.e. on Debian and derivatives the output is e.g.:

$ curl-config --libs
-L/usr/lib/x86_64-linux-gnu -lcurl

$ curl-config --static-libs
/usr/lib/x86_64-linux-gnu/libcurl.a -Wl,-z,relro -Wl,--as-needed -lidn -lssh2 \
-lssl -lcrypto -llber -lldap -lrt -Wl,-z,relro -lgssapi_krb5 -lkrb5 -lk5crypto \
-lcom_err -lssl -lcrypto -lz -lrtmp -lz -lgnutls

$ curl-config --libs --static-libs 
-L/usr/lib/x86_64-linux-gnu -lcurl
/usr/lib/x86_64-linux-gnu/libcurl.a -Wl,-z,relro -Wl,--as-needed -lidn -lssh2 \
-lssl -lcrypto -llber -lldap -lrt -Wl,-z,relro -lgssapi_krb5 -lkrb5 -lk5crypto \
-lcom_err -lssl -lcrypto -lz -lrtmp -lz -lgnutls

I agree that SSL info should be added to curl-config, but this change will not end in distributions for a long time, so it's easier to fix it here (and e.g. install pycurl with pip).

I've now separated the curl-config call to --libs and --libs-static.

raise ConfigurationError(msg)
for feature in split_quoted(stdout.decode()):
if feature == 'SSL':
# this means any ssl library, not just openssl
Copy link
Member

Choose a reason for hiding this comment

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

This part should be restored. If libcurl has ssl support and pycurl does not handle the ssl library libcurl uses, I want to retain the compile-time warning.

@comel
Copy link
Author

comel commented Jan 11, 2014

OK, I've added the code with the same functionality.

@p
Copy link
Member

p commented Jan 11, 2014

Thanks, this looks pretty good now. Found some more issues though.

# Check for SSL in all library linking information (static and shared)
libs_all = []
try:
libs_all.extend(subprocess.check_output([CURL_CONFIG, '--libs']).split())
Copy link
Member

Choose a reason for hiding this comment

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

check_output was added in python 2.7. There are already calls earlier in setup.py to run curl-config with --libs and --static-libs so you should be able to simply use optbuf.

@comel
Copy link
Author

comel commented Jan 12, 2014

I've added config_check_output() which could be used also in other parts of configure_unix().

@p
Copy link
Member

p commented Jan 13, 2014

Please check current master.

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

2 participants