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 OpenSSL binding C preprocesor hack failing on absence of headers #5973

Merged
merged 2 commits into from
Apr 20, 2018
Merged

Fix OpenSSL binding C preprocesor hack failing on absence of headers #5973

merged 2 commits into from
Apr 20, 2018

Conversation

LVMBDV
Copy link
Contributor

@LVMBDV LVMBDV commented Apr 20, 2018

The C preprocessor hack to determine OpenSSL & LibreSSL version numbers set them to 0 on failure now. Also I redirected stderr to stdoutput and redirected that to /dev/null to silence errors on the second commit but I'm not sure if a Unix compatible shell is guaranteed, we can omit that if you like.

@LVMBDV
Copy link
Contributor Author

LVMBDV commented Apr 20, 2018

Oops, forgot to mention this resolves #4676.

@LVMBDV LVMBDV changed the title Fix/4676 libressl support Fix OpenSSL binding C preprocesor hack failing on absence of headers Apr 20, 2018
@RX14
Copy link
Member

RX14 commented Apr 20, 2018

Yeah this is still going to break things. Because of openssl and libressl's breaking API changes, assuming the oldest possible version on failure just won't work. It'll fail to link.

Actually, why are we doing this at all. We should just revert the original PR back to the original well-tested version detection that worked for openssl. We then shell out to the preprocessor only for detecting libressl, and if we can't shell out or the headers don't exist or something similar we just assume openssl instead of breaking everything

@RX14
Copy link
Member

RX14 commented Apr 20, 2018

And in parallel we should bug libressl upstream to do something actually sane, and explain our (and rust's) issues here.

Copy link
Member

@asterite asterite left a comment

Choose a reason for hiding this comment

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

It fixes the issue for me. Probably not the definitive solution, but still an improvement. Thank you! 👍

@LVMBDV
Copy link
Contributor Author

LVMBDV commented Apr 20, 2018

Yeah this is still going to break things. Because of openssl and libressl's breaking API changes, assuming the oldest possible version on failure just won't work. It'll fail to link.
Actually, why are we doing this at all. We should just revert the original PR back to the original well-tested version detection that worked for openssl.

But that's what the original version detection did in the first place, check for 1.0.2 and 1.1.0 if it can (using pkg-config) and assume it's an older version otherwise. If it fails to link with this solution, it would have failed with that too.

And in parallel we should bug libressl upstream to do something actually sane, and explain our (and rust's) issues here.

I agree but I wouldn't hold my breath on it :)

@RX14
Copy link
Member

RX14 commented Apr 20, 2018

But that's what the original version detection did in the first place, check for 1.0.2 and 1.1.0 if it can (using pkg-config) and assume it's an older version otherwise. If it fails to link with this solution, it would have failed with that too.

But we no-longer check pkg-config --modversion at all. We previously did and it worked fine, now we don't and it doesn't work fine.

@LVMBDV
Copy link
Contributor Author

LVMBDV commented Apr 20, 2018

We previously did and it worked fine, now we don't and it doesn't work fine.

I think the current solution is fine and every hickup we had with it was due to trivial implementation errors but I am not against the idea of trying pkg-config --modversion first, headers later. I would give it a go but I am swamped with my bachelor's project at the moment.

Copy link
Member

@RX14 RX14 left a comment

Choose a reason for hiding this comment

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

Let's merge this now but revert if we hear any more issues with this stuff.

@RX14 RX14 added this to the Next milestone Apr 20, 2018
@RX14 RX14 added kind:bug A bug in the code. Does not apply to documentation, specs, etc. community:in-progress topic:stdlib and removed community:in-progress labels Apr 20, 2018
@RX14 RX14 merged commit af2e23d into crystal-lang:master Apr 20, 2018
@oprypin
Copy link
Member

oprypin commented May 1, 2018

(@sdogruyol noticed this)

After this commit, on Arch Linux:

$ make clean std_spec
Using /usr/bin/llvm-config [version=6.0.0]
rm -rf .build
rm -rf ./docs
rm -rf src/llvm/ext/llvm_ext.o
rm -rf src/ext/sigfault.o src/ext/libcrystal.a
g++ -c  -o src/llvm/ext/llvm_ext.o src/llvm/ext/llvm_ext.cc -I/usr/include -march=x86-64 -mtune=generic -O2 -pipe -fstack-protector-strong -fno-plt -fPIC -fvisibility-inlines-hidden -Werror=date-time -std=c++11 -Wall -W -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wno-maybe-uninitialized -Wdelete-non-virtual-dtor -Wno-comment -ffunction-sections -fdata-sections -O3 -DNDEBUG  -fno-exceptions -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS
cc -fPIC    -c -o src/ext/sigfault.o src/ext/sigfault.c
ar -rcs src/ext/libcrystal.a src/ext/sigfault.o
./bin/crystal build  -o .build/std_spec spec/std_spec.cr
_main.o: In function `__crystal_main':
/home/blaxpirit/repos/crystal/src/openssl/lib_ssl.cr:218: undefined reference to `SSL_library_init'
/home/blaxpirit/repos/crystal/src/openssl/lib_ssl.cr:218: undefined reference to `SSL_load_error_strings'
/home/blaxpirit/repos/crystal/src/openssl/lib_ssl.cr:218: undefined reference to `OPENSSL_add_all_algorithms_noconf'
/home/blaxpirit/repos/crystal/src/openssl/lib_ssl.cr:218: undefined reference to `ERR_load_crypto_strings'
_main.o: In function `~procProc(Pointer(Void), Nil)@src/openssl/ssl/hostname_validation.cr:66':
/home/blaxpirit/repos/crystal/src/raise.cr:202: undefined reference to `sk_free'
_main.o: In function `~proc2Proc(Pointer(Void), Nil)@src/openssl/ssl/hostname_validation.cr:66':
/home/blaxpirit/repos/crystal/src/raise.cr:202: undefined reference to `sk_free'
_main.o: In function `~proc3Proc(Pointer(Void), Nil)@src/openssl/ssl/hostname_validation.cr:66':
/home/blaxpirit/repos/crystal/src/raise.cr:202: undefined reference to `sk_free'
_main.o: In function `~proc4Proc(Pointer(Void), Nil)@src/openssl/ssl/hostname_validation.cr:66':
/home/blaxpirit/repos/crystal/src/raise.cr:202: undefined reference to `sk_free'
_main.o: In function `~proc5Proc(Pointer(Void), Nil)@src/openssl/ssl/hostname_validation.cr:66':
/home/blaxpirit/repos/crystal/src/raise.cr:202: undefined reference to `sk_free'
_main.o:/home/blaxpirit/repos/crystal/src/raise.cr:202: more undefined references to `sk_free' follow
O-penS-S-L-5858D-igest.o: In function `new_evp_mt_ctx':
/home/blaxpirit/repos/crystal/src/openssl/digest/digest.cr:23: undefined reference to `EVP_MD_CTX_create'
O-penS-S-L-5858D-igest.o: In function `finalize':
/home/blaxpirit/repos/crystal/src/openssl/digest/digest.cr:38: undefined reference to `EVP_MD_CTX_destroy'
O-penS-S-L-5858D-igest.o: In function `clone':
/home/blaxpirit/repos/crystal/src/openssl/digest/digest.cr:42: undefined reference to `EVP_MD_CTX_create'
/home/blaxpirit/repos/crystal/src/openssl/digest/digest.cr:44: undefined reference to `EVP_MD_CTX_destroy'
O-penS-S-L-5858S-S-L-5858C-ontext.o: In function `default_method':
/home/blaxpirit/repos/crystal/src/openssl/ssl/context.cr:4: undefined reference to `SSLv23_method'
O-penS-S-L-5858S-S-L-5858H-ostnameV-alidation.o: In function `matches_subject_alternative_name':
/home/blaxpirit/repos/crystal/src/openssl/ssl/hostname_validation.cr:66: undefined reference to `sk_pop_free'
/home/blaxpirit/repos/crystal/src/openssl/ssl/hostname_validation.cr:66: undefined reference to `sk_pop_free'
/home/blaxpirit/repos/crystal/src/openssl/ssl/hostname_validation.cr:34: undefined reference to `sk_num'
/home/blaxpirit/repos/crystal/src/openssl/ssl/hostname_validation.cr:35: undefined reference to `sk_value'
/home/blaxpirit/repos/crystal/src/openssl/ssl/hostname_validation.cr:66: undefined reference to `sk_pop_free'
/home/blaxpirit/repos/crystal/src/openssl/ssl/hostname_validation.cr:66: undefined reference to `sk_pop_free'
/home/blaxpirit/repos/crystal/src/openssl/ssl/hostname_validation.cr:66: undefined reference to `sk_pop_free'
/home/blaxpirit/repos/crystal/src/openssl/ssl/hostname_validation.cr:66: undefined reference to `sk_pop_free'
/home/blaxpirit/repos/crystal/src/openssl/ssl/hostname_validation.cr:66: undefined reference to `sk_pop_free'
collect2: error: ld returned 1 exit status
Error: execution of command failed with code: 1: `cc "${@}" -o '/home/blaxpirit/repos/crystal/.build/std_spec'  -rdynamic  -lyaml -lreadline `command -v pkg-config > /dev/null && pkg-config --libs --silence-errors libssl || printf %s '-lssl -lcrypto'` `command -v pkg-config > /dev/null && pkg-config --libs --silence-errors libcrypto || printf %s ' -lcrypto'` /home/blaxpirit/repos/crystal/src/llvm/ext/llvm_ext.o `/usr/bin/llvm-config --libs --system-libs --ldflags 2> /dev/null` -lstdc++ -lgmp -lz -lxml2 -lpcre -lm -lgc -lpthread /home/blaxpirit/repos/crystal/src/ext/libcrystal.a -levent -lrt -ldl -L/usr/lib -L/usr/local/lib`
make: *** [Makefile:112: .build/std_spec] Error 1

$ pacman -Q | grep ^openssl
openssl 1.1.0.h-1
openssl-1.0 1.0.2.o-1

Before this commit the same command passes.

@sdogruyol
Copy link
Member

@oprypin
Copy link
Member

oprypin commented May 1, 2018

That's a link to this very pull request :p

@sdogruyol
Copy link
Member

Gotta love recursion 😄

@LVMBDV
Copy link
Contributor Author

LVMBDV commented May 2, 2018

My fault again, I found an error in the second commit where I redirect stderr to stdout then redirect both to /dev/null to suppress any errors from cc. This is obviously a silly thing to do that since we need stdout output to determine OPENSSL_VERSION. This can be fixed by reverting that commit but I think @RX14's suggestion to revert to the original solution with this hack as the backup seems saner.

@LVMBDV LVMBDV deleted the fix/4676-libressl-support branch May 2, 2018 16:58
chris-huxtable pushed a commit to chris-huxtable/crystal that referenced this pull request Jun 6, 2018

Verified

This commit was signed with the committer’s verified signature. The key has expired.
chris-huxtable Chris Huxtable
…rystal-lang#5973)

* Fixed openssl/lib_crypto to not fail when openssl headers are not present on the system

* Fixed openssl/lib_crypto to not output errors if the C preprocessor hack fails
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:stdlib
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants