Make autotools check for all the used openssl functions #838

pull elichai wants to merge 2 commits into bitcoin-core:master from elichai:2020-10-openssl-m4 changing 2 files +31 −5

elichai commented at 11:50 AM on October 21, 2020: contributor

I added all the openssl functions that we call in tests.c and in bench_verify.c to the m4 check, that way if any of them are missing it won't enable openssl. I also modified it a little to prevent a segmentation fault when running that program (not that it really matters for autotools)

This should fix #836
real-or-random changes_requested
real-or-random commented at 4:45 PM on October 23, 2020: contributor

d2i_ECPrivateKey and EC_KEY_check_key (used in get_openssl_key in tests.c) are missing.
elichai commented at 11:18 AM on October 25, 2020: contributor

d2i_ECPrivateKey and EC_KEY_check_key (used in get_openssl_key in tests.c) are missing.

I have no idea how I missed this, I grepped ENABLE_OPENSSL_TESTS multiple times. Will add now, thanks!

Modify bitcoin_secp.m4's openssl check to call all the functions that we

use in the tests/benchmarks.
That way linking will fail if those symbols are missing

e6692778d3

elichai force-pushed on Oct 25, 2020
real-or-random approved
real-or-random commented at 2:40 PM on October 25, 2020: contributor

ACK e6692778d3f6507eb1325785cdd424073a945ff7

I've just realized that the summary of build options at the end of configure does not show whether tests are enabled, nor whether OpenSSLs are enabled. If you want, add a commit.
jonasnick commented at 10:58 AM on October 27, 2020: contributor

If you want, add a commit.

Do you @elichai ? :) I think that the the author of #836 might have noticed that this was a configure option if it was diplayed in the summary.
elichai commented at 11:11 AM on October 27, 2020: contributor
Is this what you meant? @real-or-random @jonasnick

It's also somewhat confusing that everyone calls the library openssl but we actually use libcrypto (which is a part of openssl), so the logs say:
```
checking for main in -lcrypto... yes
checking for EC functions in libcrypto... yes
```

in configure.ac:400 in bd25aac548 outdated

 396 | @@ -397,6 +397,7 @@ if test x"$use_tests" = x"yes"; then
 397 |    SECP_OPENSSL_CHECK
 398 |    if test x"$has_openssl_ec" = x"yes"; then
 399 |      if test x"$enable_openssl_tests" != x"no"; then
 400 | +    enable_openssl_tests=yes

jonasnick commented at 1:29 PM on October 27, 2020:

wrong indentation

jonasnick commented at 1:32 PM on October 27, 2020: contributor

@elichai Cool, that's what I meant at least. Seems that the we need to put enable_openssl_tests into one more branch, because afaics if use_tests = yes and has_openssl_ec = no then enable_openssl_tests would be auto instead of no.
Configure echo if openssl tests are enabled 3734b68200
elichai force-pushed on Oct 27, 2020
real-or-random approved
real-or-random commented at 3:42 PM on October 27, 2020: contributor

ACK 3734b68200ee37f5eea80f47d611e9b5a65548fe
sipa commented at 6:10 PM on October 27, 2020: contributor

ACK 3734b68200ee37f5eea80f47d611e9b5a65548fe
real-or-random merged this on Oct 27, 2020
real-or-random closed this on Oct 27, 2020
elichai deleted the branch on Oct 28, 2020
jasonbcox referenced this in commit ccf0cf3c66 on Oct 29, 2020
deadalnix referenced this in commit 00fa9d553a on Oct 30, 2020

Contributors

Linked (view graph)

#836 tests.c: undefined reference to `ECSDA_SIG_get0`