aremily commented on issue #92: OpenSSL 1.1.0 updates with backward compatibility for OpenSSL 1.0.2 and 1.0.1 URL: https://github.com/apache/commons-crypto/pull/92#issuecomment-462469860 Completely spaced off that that was a compile time check. I'll swap it out with a dynamic one. What diff tool are you using? I should clean up the formatting stuff before I check it in. On Mon, Feb 11, 2019 at 1:59 PM Marcelo Vanzin <notificati...@github.com> wrote: > *@vanzin* commented on this pull request. > > Looks almost there. I still noticed a couple of places where it seems you > have a compile-time check disguised as a runtime check. > ------------------------------ > > In .gitignore > <https://github.com/apache/commons-crypto/pull/92#discussion_r255640544>: > > > @@ -1,37 +0,0 @@ > -*~ > > Don't delete this file. > ------------------------------ > > In .travis.yml > <https://github.com/apache/commons-crypto/pull/92#discussion_r255640619>: > > > @@ -24,7 +24,6 @@ matrix: > - "curl -L --cookie 'oraclelicense=accept-securebackup-cookie;' http://download.oracle.com/otn-pub/java/jce/8/jce_policy-8.zip -o /tmp/policy.zip && sudo unzip -j -o /tmp/policy.zip *.jar -d `jdk_switcher home oraclejdk8`/jre/lib/security && rm /tmp/policy.zip" > after_success: > - mvn clean test jacoco:report coveralls:report > - > > Undo. > ------------------------------ > > In Makefile > <https://github.com/apache/commons-crypto/pull/92#discussion_r255640785>: > > > @@ -18,7 +18,7 @@ > include Makefile.common > > COMMONS_CRYPTO_OUT:=$(TARGET)/$(commons-crypto)-$(os_arch) > -COMMONS_CRYPTO_OBJ:=$(addprefix $(COMMONS_CRYPTO_OUT)/,OpenSslCryptoRandomNative.o OpenSslNative.o OpenSslInfoNative.o) > +COMMONS_CRYPTO_OBJ:=$(addprefix $(COMMONS_CRYPTO_OUT)/,OpenSslInfoNative.o OpenSslCryptoRandomNative.o OpenSslNative.o) > > Nothing is changing here, right? If not, undo. > ------------------------------ > > In src/main/java/org/apache/commons/crypto/jna/OpenSslJnaCryptoRandom.java > <https://github.com/apache/commons-crypto/pull/92#discussion_r255642430>: > > > @@ -103,10 +103,9 @@ public void nextBytes(byte[] bytes) { > //to support multithreading https://wiki.openssl.org/index.php/Manual:Threads(3) needs to be done > > if(rdrandEnabled && OpenSslNativeJna.RAND_get_rand_method().equals(OpenSslNativeJna.RAND_SSLeay())) { > - close(); > > Why do you need to remove this? > ------------------------------ > > In src/main/native/org/apache/commons/crypto/OpenSslInfoNative.c > <https://github.com/apache/commons-crypto/pull/92#discussion_r255644566>: > > > @@ -135,3 +96,35 @@ JNIEXPORT jstring JNICALL Java_org_apache_commons_crypto_OpenSslInfoNative_Nativ > { > return (*env)->NewStringUTF(env, PROJECT_NAME); > } > + > +JNIEXPORT jlong JNICALL Java_org_apache_commons_crypto_OpenSslInfoNative_OpenSSL > + (JNIEnv *env, jclass clazz) > +{ > + if (!load_library(env)) { > + return 0; > + } > + if(OPENSSL_VERSION_NUMBER > VERSION_1_1_X){ > > nit: space after if (also in some other places) > > Where does OPENSSL_VERSION_NUMBER come from? A quick google search seems > to indicate it's a macro in an OpenSSL header. Which makes this a compile > time check, basically. > ------------------------------ > > In src/main/native/org/apache/commons/crypto/cipher/OpenSslNative.c > <https://github.com/apache/commons-crypto/pull/92#discussion_r255646682>: > > > @@ -716,3 +677,52 @@ JNIEXPORT void JNICALL Java_org_apache_commons_crypto_cipher_OpenSslNative_clean > EVP_CTX_Wrapper *wrapper = CTX_WRAPPER(ctx); > free_context_wrapper(wrapper); > } > + > +static int check_update_max_output_len(JNIEnv *env, EVP_CIPHER_CTX *context, jlong ctx, int input_len, int max_output_len) > > All these arguments feel a little redundant. e.g. you don't need context > since you can just use wrapper->context. > > With some small changes you could also just pass the wrapper directly from > the callers, instead of passing the jni env + the pointer address. > ------------------------------ > > In src/main/native/org/apache/commons/crypto/cipher/OpenSslNative.c > <https://github.com/apache/commons-crypto/pull/92#discussion_r255646781>: > > > + } > + } > + return 0; > + } > +} > + > +static int check_doFinal_max_output_len(JNIEnv *env, EVP_CIPHER_CTX *context, int max_output_len) > +{ > + if (dlsym_EVP_CIPHER_CTX_test_flags(context, EVP_CIPH_NO_PADDING) == EVP_CIPH_NO_PADDING) { > + return 1; > + } else { > + int b = dlsym_EVP_CIPHER_CTX_block_size(context); > + if (max_output_len >= b) { > + return 1; > + } > + return 0; > > indentation > ------------------------------ > > In > src/main/native/org/apache/commons/crypto/random/OpenSslCryptoRandomNative.c > <https://github.com/apache/commons-crypto/pull/92#discussion_r255647218>: > > > { > - locks_setup(); > + if (OPENSSL_VERSION_NUMBER < VERSION_1_1_X) { > > Looks like another compile-time check, if OPENSSL_VERSION_NUMBER really > comes from the openssl headers. > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <https://github.com/apache/commons-crypto/pull/92#pullrequestreview-202272520>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AGg8fuSJSBQMSRapPjIN5Fra5YMR15rtks5vMb17gaJpZM4ZgACn> > . >
---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
