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

configure: fix the -ldl check for openssl, add -lpthread check #1427

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@bagder
Member

bagder commented Apr 18, 2017

The check for if -ldl is needed to build with (a statically built)
openssl was broken. This repairs the check, and adds a check for
-lpthread as well since OpenSSL 1.1.0+ does in fact require -lpthread so
only adding -ldl for a static openssl build is no longer enough.

Reported-by: Jay Satiro
Fixes #1426

configure: fix the -ldl check for openssl, add -lpthread check
The check for if -ldl is needed to build with (a statically built)
openssl was broken. This repairs the check, and adds a check for
-lpthread as well since OpenSSL 1.1.0+ does in fact require -lpthread so
only adding -ldl for a static openssl build is no longer enough.

Reported-by: Jay Satiro
Fixes #1426

@bagder bagder requested a review from jay Apr 18, 2017

@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Apr 18, 2017

@bagder, thanks for your PR! By analyzing the history of the files in this pull request, we identified @yangtse, @dfandrich and @gknauf to be potential reviewers.

mention-bot commented Apr 18, 2017

@bagder, thanks for your PR! By analyzing the history of the files in this pull request, we identified @yangtse, @dfandrich and @gknauf to be potential reviewers.

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Apr 20, 2017

Member

Turns out this PR doesn't fix the #1426 case but it did fix my test builds using a static OpenSSL 1.1.0 devel version (from git master) so unless someone objects I intend to merge this soon. It removes the need to specify LIBS="-ldl -lpthread" ./configure

Member

bagder commented Apr 20, 2017

Turns out this PR doesn't fix the #1426 case but it did fix my test builds using a static OpenSSL 1.1.0 devel version (from git master) so unless someone objects I intend to merge this soon. It removes the need to specify LIBS="-ldl -lpthread" ./configure

@jay

I'm not sure I follow this, it looks like you've changed the -ldl check to only occur when HMAC_Init_ex link fails. Also at this point there may already be -lssl -lcrypto from pkg-config which seems like we could skip these crypto checks then, like in other words imagine
-lpthread -ldl -lcrypto -lssl -lcrypto
but -ldl should come after -lssl shouldn't it?

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Apr 21, 2017

Member

it looks like you've changed the -ldl check to only occur when HMAC_Init_ex link fails

Yes. When building with a static openssl that requires -ldl, trying to link the library to use HMAC_Init_ex fails. The previous (current) way to check for -ldl is pointless since it will fail to detect openssl completely before even considering the -ldl issue.

Also at this point there may already be -lssl -lcrypto from pkg-config which seems like we could skip these crypto checks then

Maybe, but I would prefer to have the -ldl and -lpthread checks only get tried when it fails without those libs and I only want to add those specific libs to try to fix the build and not make conclusions or guesses about the other libs that are used or not used.

Hence the flow with this patch goes like this:

  1. openssl is detected fine so far
  2. verifty that can link fine with HMAC_Init_ex, if yes, then we're good. if not
  3. retry the link but now with -ldl added to LIBS. If it succeeds we're good, if not
  4. retry the link but now also add -lpthread to LIBS. If it succeeds we're good, if not we fail the libcrypto detection

I've tested this with a static openssl from their git master and with this fix, it works without any custom LIBS set, but without this patch it will fail to detect a static only openssl completely.

Member

bagder commented Apr 21, 2017

it looks like you've changed the -ldl check to only occur when HMAC_Init_ex link fails

Yes. When building with a static openssl that requires -ldl, trying to link the library to use HMAC_Init_ex fails. The previous (current) way to check for -ldl is pointless since it will fail to detect openssl completely before even considering the -ldl issue.

Also at this point there may already be -lssl -lcrypto from pkg-config which seems like we could skip these crypto checks then

Maybe, but I would prefer to have the -ldl and -lpthread checks only get tried when it fails without those libs and I only want to add those specific libs to try to fix the build and not make conclusions or guesses about the other libs that are used or not used.

Hence the flow with this patch goes like this:

  1. openssl is detected fine so far
  2. verifty that can link fine with HMAC_Init_ex, if yes, then we're good. if not
  3. retry the link but now with -ldl added to LIBS. If it succeeds we're good, if not
  4. retry the link but now also add -lpthread to LIBS. If it succeeds we're good, if not we fail the libcrypto detection

I've tested this with a static openssl from their git master and with this fix, it works without any custom LIBS set, but without this patch it will fail to detect a static only openssl completely.

@bagder bagder closed this in c68fed8 Apr 24, 2017

@bagder bagder deleted the bagder/configure-openssl-ldl branch Apr 24, 2017

@lock lock bot locked as resolved and limited conversation to collaborators May 14, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.