Fix build failure against OpenSSL 1.1 built with no-deprecated #160

markwright · 2017-10-14T11:06:38Z

The proposed changes remove the use of the OpenSSL 1.1.0 deprecated API. Which fixes the following build errors that occur on Gentoo when building ruby 2.4.2 with openssl 1.1.0f built with no-deprecated:

compiling ossl.c
ossl.c: In function ‘Init_openssl’:
ossl.c:1014:5: warning: implicit declaration of function ‘OpenSSL_add_all_algorithms’; did you mean ‘OpenSSL_add_ssl_algorithms’? [-Wimplicit-function-declaration]
OpenSSL_add_all_algorithms();
^~~~~~~~~~~~~~~~~~~~~~~~~~
OpenSSL_add_ssl_algorithms
ossl.c:1015:5: warning: implicit declaration of function ‘ERR_load_crypto_strings’; did you mean ‘ERR_load_OCSP_strings’? [-Wimplicit-function-declaration]
ERR_load_crypto_strings();
^~~~~~~~~~~~~~~~~~~~~~~
ERR_load_OCSP_strings
ossl.c:1016:5: warning: implicit declaration of function ‘SSL_load_error_strings’; did you mean ‘ERR_lib_error_string’? [-Wimplicit-function-declaration]
SSL_load_error_strings();
^~~~~~~~~~~~~~~~~~~~~~
ERR_lib_error_string
In file included from ../.././include/ruby/ruby.h:36:0,
from ../.././include/ruby.h:33,
from ossl.h:17,
from ossl.c:10:
ossl.c:1051:67: warning: implicit declaration of function ‘SSLeay_version’; did you mean ‘SSL_version’? [-Wimplicit-function-declaration]
rb_define_const(mOSSL, "OPENSSL_LIBRARY_VERSION", rb_str_new2(SSLeay_version(SSLEAY_VERSION)));
^
../.././include/ruby/defines.h:94:53: note: in definition of macro ‘RB_GNUC_EXTENSION_BLOCK’
#define RB_GNUC_EXTENSION_BLOCK(x) extension ({ x; })
^
../.././include/ruby/intern.h:857:21: note: in expansion of macro ‘rb_str_new_cstr’
#define rb_str_new2 rb_str_new_cstr
^~~~~~~~~~~~~~~
ossl.c:1051:55: note: in expansion of macro ‘rb_str_new2’
rb_define_const(mOSSL, "OPENSSL_LIBRARY_VERSION", rb_str_new2(SSLeay_version(SSLEAY_VERSION)));
^~~~~~~~~~~
ossl.c:1051:82: error: ‘SSLEAY_VERSION’ undeclared (first use in this function); did you mean ‘SSL2_VERSION’?
rb_define_const(mOSSL, "OPENSSL_LIBRARY_VERSION", rb_str_new2(SSLeay_version(SSLEAY_VERSION)));
^
../.././include/ruby/defines.h:94:53: note: in definition of macro ‘RB_GNUC_EXTENSION_BLOCK’
#define RB_GNUC_EXTENSION_BLOCK(x) extension ({ x; })
^
../.././include/ruby/intern.h:857:21: note: in expansion of macro ‘rb_str_new_cstr’
#define rb_str_new2 rb_str_new_cstr
^~~~~~~~~~~~~~~
ossl.c:1051:55: note: in expansion of macro ‘rb_str_new2’
rb_define_const(mOSSL, "OPENSSL_LIBRARY_VERSION", rb_str_new2(SSLeay_version(SSLEAY_VERSION)));
^~~~~~~~~~~
ossl.c:1051:82: note: each undeclared identifier is reported only once for each function it appears in
rb_define_const(mOSSL, "OPENSSL_LIBRARY_VERSION", rb_str_new2(SSLeay_version(SSLEAY_VERSION)));
^
../.././include/ruby/defines.h:94:53: note: in definition of macro ‘RB_GNUC_EXTENSION_BLOCK’
#define RB_GNUC_EXTENSION_BLOCK(x) extension ({ x; })
^
../.././include/ruby/intern.h:857:21: note: in expansion of macro ‘rb_str_new_cstr’
#define rb_str_new2 rb_str_new_cstr
^~~~~~~~~~~~~~~
ossl.c:1051:55: note: in expansion of macro ‘rb_str_new2’
rb_define_const(mOSSL, "OPENSSL_LIBRARY_VERSION", rb_str_new2(SSLeay_version(SSLEAY_VERSION)));
^~~~~~~~~~~

The Gentoo bug is:
https://bugs.gentoo.org/614760

rhenium

This is an issue that has to be fixed anyway before OpenSSL 1.2.0 is out. Thanks for working on this!

rhenium · 2017-10-14T16:00:40Z

ext/openssl/ossl.c

    OpenSSL_add_ssl_algorithms();
    OpenSSL_add_all_algorithms();
    ERR_load_crypto_strings();
    SSL_load_error_strings();
+#endif


Suffix L and the parentheses around OPENSSL_VERSION_NUMBER >= ~~ are redundant.

The return value from OPENSSL_init_ssl() should be checked.

if (!OPENSSL_init_ssl(0, NULL)) rb_raise(rb_eRuntimeError, "OPENSSL_init_ssl");

While you're at it, can you remove the obsolete comments above and below?

rhenium · 2017-10-14T16:02:31Z

ext/openssl/ossl.h

+#  include <openssl/dsa.h>
+#  include <openssl/evp.h>
+#  include <openssl/dh.h>
+#endif


They can be unconditionally included since they all exist in OpenSSL 1.0.1 (which is the minimum supported) and LibreSSL.

rhenium · 2017-10-14T16:02:43Z

ext/openssl/ossl_cipher.c

    if (EVP_CIPHER_CTX_flags(ctx) & EVP_CIPH_FLAG_AEAD_CIPHER)
+#endif


EVP_CIPHER_CTX_cipher() and EVP_CIPHER_flags() exist in OpenSSL 1.0.1.

rhenium · 2017-10-14T16:04:04Z

ext/openssl/ossl_engine.c

    OSSL_ENGINE_LOAD_IF_MATCH(dynamic);
 #endif
+#endif


This is comparing the string with "DYNAMIC" rather than "dynamic". The return value from OPENSSL_init_crypto() should be checked.

rhenium · 2017-10-14T16:04:30Z

ext/openssl/ossl_x509cert.c

    if (!X509_set_notBefore(x509, asn1time)) {
+#endif


I'd define macro in ext/openssl/openssl_missing.h. It's actually done for the getter functions, but apparently I forgot to do that for the setters.

#define X509_set1_notBefore(x, t) X509_set_notBefore(x, t)

rhenium · 2017-10-29T13:03:42Z

ext/openssl/ossl_engine.c

 #define OSSL_ENGINE_LOAD_IF_MATCH(x) \
 do{\
  if(!strcmp(#x, RSTRING_PTR(name))){\
    ENGINE_load_##x();\
    return Qtrue;\
  }\
 }while(0)
+#endif


strcasecmp() would be overkill. I was expecting something like:

#define OSSL_ENGINE_LOAD_IF_MATCH(engine_name, x) do { \ ... if (!strcmp(#engine_name, RSTRING_PTR(name)))... ... OSSL_ENGINE_LOAD_IF_MATCH(dynamic, DYNAMIC);

The OpenSSL < 1.1.0 version can have the same signature so the #ifs below can be removed.

Additionally, (this is not documented anywhere, but) ossl_raise() that rewinds "error"s in the OpenSSL error queue should be used with eEngineError instead. ossl.c had to use the raw rb_raise() because OpenSSL wouldn't be initialized at that time.

rhenium · 2017-10-29T13:04:37Z

ext/openssl/ossl_engine.c

@@ -160,7 +198,9 @@ ossl_engine_s_load(int argc, VALUE *argv, VALUE klass)
 static VALUE
 ossl_engine_s_cleanup(VALUE self)
 {
+#if defined(LIBRESSL_VERSION_NUMBER) || (OPENSSL_VERSION_NUMBER < 0x10100000L)


Please remove () and L.

rhenium · 2017-10-29T13:04:45Z

ext/openssl/ossl_x509crl.c

    if (!X509_CRL_set_lastUpdate(crl, asn1time)) {
+#endif


Can you add X509_CRL_set1_{last,next}Update() to openssl_missing.h, too?

rhenium

Overall this looks good to me, aside from a typo (see inline comment). Can you fix it, and squash the commits into one?

rhenium · 2017-11-03T15:22:08Z

ext/openssl/ossl_engine.c

+    if (OPENSSL_init_crypto(OPENSSL_INIT_ENGINE_##x, NULL))\
+      return Qtrue;\
+    else\
+      ossl_raise(eEngineError, "OPENSSL_init_ssl"); \


s/_ssl/_crypto/

Thanks rhenium for the code review and fixes.

markwright · 2017-11-04T03:10:49Z

Thanks I fixed the typo s/_ssl/_crypto/ and squashed all the commits.

rhenium · 2017-11-04T03:29:15Z

Thanks!

markwright force-pushed the master branch 2 times, most recently from 2f4a312 to e103636 Compare October 14, 2017 14:34

rhenium requested changes Oct 14, 2017

View reviewed changes

markwright force-pushed the master branch 3 times, most recently from 3eb532f to badffb6 Compare October 28, 2017 06:59

rhenium requested changes Oct 29, 2017

View reviewed changes

markwright force-pushed the master branch 2 times, most recently from 53bea13 to 8fb6759 Compare November 3, 2017 01:35

rhenium approved these changes Nov 3, 2017

View reviewed changes

Fix build failure against OpenSSL 1.1 built with no-deprecated

7af8081

Thanks rhenium for the code review and fixes.

markwright force-pushed the master branch from 8fb6759 to 7af8081 Compare November 4, 2017 02:52

rhenium merged commit 37106e3 into ruby:master Nov 4, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix build failure against OpenSSL 1.1 built with no-deprecated #160

Fix build failure against OpenSSL 1.1 built with no-deprecated #160

markwright commented Oct 14, 2017

rhenium left a comment

rhenium Oct 14, 2017

rhenium Oct 14, 2017

rhenium Oct 14, 2017

rhenium Oct 14, 2017

rhenium Oct 14, 2017

rhenium Oct 29, 2017

rhenium Oct 29, 2017

rhenium Oct 29, 2017

rhenium left a comment

rhenium Nov 3, 2017

markwright commented Nov 4, 2017

rhenium commented Nov 4, 2017

		if (EVP_CIPHER_CTX_flags(ctx) & EVP_CIPH_FLAG_AEAD_CIPHER)
		#endif

Fix build failure against OpenSSL 1.1 built with no-deprecated #160

Fix build failure against OpenSSL 1.1 built with no-deprecated #160

Conversation

markwright commented Oct 14, 2017

rhenium left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhenium left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markwright commented Nov 4, 2017

rhenium commented Nov 4, 2017