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

Test results #3

Closed
Jan-E opened this Issue Jan 31, 2019 · 18 comments

Comments

Projects
None yet
2 participants
@Jan-E
Copy link

Jan-E commented Jan 31, 2019

Can you post the test results?
\python27\python.exe win-tests.py --release --cleanup

The reason I am asking: does crypto-test.exe run at all? In my builds it starts, but never ends execution.

@nono303

This comment has been minimized.

Copy link
Owner

nono303 commented Feb 1, 2019

Hi @Jan-E ,

All 119 tests run at the end but for me crypto-test.exe is skipped… (don’t know why)

START: crypto-test.exe
SKIP: crypto-test 1: basic password encryption/decryption test
SKIP: crypto-test 2: password checktext generation/validation
END: crypto-test.exe
ELAPSED: crypto-test.exe 0:00:00.047000

  • All deps (including openssl) are set in the path

  • I launch win-tests.py --release –cleanup

  • Build conf are :

    • python gen-make.py --release -t vcproj --with-jdk=C:\jdk8\%ARCH% --with-serf=C:\httpd-sdk\install\include --with-sqlite=C:\httpd-sdk\src\sqlite-amalgamation --vsnet-version=2017 --with-openssl=C:\httpd-sdk\install --with-apr-util=C:\httpd-sdk\install --with-apr=C:\httpd-sdk\install --with-apr-iconv=C:\httpd-sdk\install --with-zlib=C:\httpd-sdk\install --with-apr_memcache=C:\httpd-sdk\install --with-httpd=C:\httpd-sdk\install

    • MSBuild.exe subversion_vcnet.sln /nowarn:C4702 /nowarn:LNK4087 /nowarn:C4703 /nowarn:C4132 /nowarn:C4389 /nowarn:C4244 /nowarn:C4245 /nowarn:C4267 /nowarn:C4018 /nowarn:C4334 /nowarn:C4189 /nowarn:C4312 /nowarn:C4090 /nowarn:C4152 /nowarn:C4146 /nologo /m:8 /t:Clean;__ALL__;__JAVAHL__;__ALL_TESTS__ /p:Configuration=Release /p:Platform=%archmsbuild% /p:DebugSymbols=true /p:DebugType=None

  • tests.zip detailled results

If you have any idea in what may be wrong in my test case, give me a feedback!

@nono303 nono303 self-assigned this Feb 1, 2019

@Jan-E

This comment has been minimized.

Copy link
Author

Jan-E commented Feb 1, 2019

What is in your apu.h (and/or apu.hw)? It should contain

#define APU_HAVE_CRYPTO         1

#ifndef APU_DSO_MODULE_BUILD
#define APU_HAVE_OPENSSL        1
#define APU_HAVE_NSS            0
#define APU_HAVE_COMMONCRYPTO   0
#endif
@nono303

This comment has been minimized.

Copy link
Owner

nono303 commented Feb 1, 2019

It was:

#define APU_HAVE_CRYPTO 1

#ifndef APU_DSO_MODULE_BUILD
#define APU_HAVE_OPENSSL 1
#define APU_HAVE_NSS 0
#define APU_HAVE_COMMONCRYPTO 0
#endif

change APU_HAVE_CRYPTO & APU_HAVE_OPENSSL to 1, build & launch crypto-test.exe which now,effectively, never end ; blocked and looping on this stack (1 full core cpu fully loaded):

ntoskrnl.exe!KiCpuId+0xaa
ntoskrnl.exe!KeReleaseSpinLock+0x612
ntoskrnl.exe!KeWaitForMutexObject+0x1a3
ntoskrnl.exe!KeQueryActiveProcessorCountEx+0x218
ntoskrnl.exe!RtlNumberOfSetBitsUlongPtr+0x10cd
ntoskrnl.exe!KiCpuId+0x2553
libcrypto-1_1-x64.dll!CRYPTO_malloc+0x20
libcrypto-1_1-x64.dll!CRYPTO_THREAD_lock_new+0x3a
libcrypto-1_1-x64.dll!OPENSSL_init_crypto+0xb2
apr_crypto_openssl-1.dll+0x104d
libaprutil-1.dll!apr_crypto_get_driver+0x106
crypto-test.exe!svn_crypto__context_create+0x127
crypto-test.exe!test_encrypt_decrypt_password+0x96
crypto-test.exe!do_test_num+0x18a
crypto-test.exe!svn_test_main+0xb07
crypto-test.exe!__scrt_common_main_seh+0x10c
kernel32.dll!BaseThreadInitThunk+0xd
ntdll.dll!RtlUserThreadStart+0x1d

@nono303 nono303 added the bug label Feb 1, 2019

@nono303

This comment has been minimized.

Copy link
Owner

nono303 commented Feb 1, 2019

will have a look on it but seems to be it looks like openssl/openssl#2865

@Jan-E

This comment has been minimized.

Copy link
Author

Jan-E commented Feb 1, 2019

Good to know that you were able to reproduce it and that it wasn't caused by my setup.

nono303 added a commit that referenced this issue Feb 1, 2019

@nono303

This comment has been minimized.

Copy link
Owner

nono303 commented Feb 1, 2019

I've just pushed apr_crypto_openssl-1.dll patched for bypassing this issue (see readme...)
Working for me for the 4 build type but @Jan-E could you try on your side and give me a feedback ?
Thanks!

@Jan-E

This comment has been minimized.

Copy link
Author

Jan-E commented Feb 1, 2019

In the OpenSSL issue openssl/openssl#2865 was a link to http://www.luke1410.de:8090/browse/MAXSVN-83. Quote:

Issue 4
Trying to run the Serf tests eventually leads to an endless loop.

The endless loop occurs in CRYPTO_malloc() where the "malloc_impl != CRYPTO_malloc"-check fails to detect that it's in the current function (presumably with OpenSSL being built in debug configuration this would trigger a stack overflow).
In particular malloc_impl is set to the statically linked CRYPTO_malloc function pointer, while CRYPTO_malloc points to the malloc function in the DLL.
With the support from xemdetia on the OpenSSL-IRC-channel, it was determined that Serf incorrectly issues SSL-calls prior to calling SSL_library_init() (which according to the OpenSSL-documentation must be called prior to any other OpenSSL-function - see: https://www.openssl.org/docs/man1.1.0/ssl/SSL_library_init.html ).
The attached patch resolves that flaw by moving the call to SSL_library_init() to before the other OpenSSL-calls.

Patch:

Index: buckets/ssl_buckets.c
===================================================================
--- buckets/ssl_buckets.c	(revision 1778230)
+++ buckets/ssl_buckets.c	(working copy)
@@ -1143,6 +1143,10 @@
         int i, numlocks;
 #endif
 
+        /* SSL_library_init() must be called prior to any other OpenSSL
+           action. */
+        SSL_library_init();
+
 #ifdef SSL_VERBOSE
         /* Warn when compile-time and run-time version of OpenSSL differ in
            major/minor version number. */
@@ -1163,7 +1167,6 @@
 #endif
         ERR_load_crypto_strings();
         SSL_load_error_strings();
-        SSL_library_init();
         OpenSSL_add_all_algorithms();
 
 #if APR_HAS_THREADS && !defined(USE_OPENSSL_1_1_API)
Index: test/server/test_sslserver.c
===================================================================
--- test/server/test_sslserver.c	(revision 1778230)
+++ test/server/test_sslserver.c	(working copy)
@@ -228,6 +228,8 @@
     /* Init OpenSSL globally */
     if (!init_done)
     {
+        /* SSL_library_init() must be called prior to any other OpenSSL action. */
+        SSL_library_init();
 #ifdef USE_OPENSSL_1_1_API
         OPENSSL_malloc_init();
 #else
@@ -235,7 +237,6 @@
 #endif
         ERR_load_crypto_strings();
         SSL_load_error_strings();
-        SSL_library_init();
         OpenSSL_add_all_algorithms();
         init_done = 1;
     }

Probably it is better to add a SSL_library_init(); before OPENSSL_malloc_init(); than to remove the malloc_init.

@Jan-E

This comment has been minimized.

Copy link
Author

Jan-E commented Feb 1, 2019

The strange thing is that mod_ssl.c in Apache 2.4.x does it the other way around:
http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/modules/ssl/mod_ssl.c?view=markup#l398

    /* We must register the library in full, to ensure our configuration
     * code can successfully test the SSL environment.
     */
#if MODSSL_USE_OPENSSL_PRE_1_1_API || defined(LIBRESSL_VERSION_NUMBER)
    (void)CRYPTO_malloc_init();
#else
    OPENSSL_malloc_init();
#endif
    ERR_load_crypto_strings();
    SSL_load_error_strings();
    SSL_library_init();
#if HAVE_ENGINE_LOAD_BUILTIN_ENGINES
    ENGINE_load_builtin_engines();
#endif
    OpenSSL_add_all_algorithms();
    OPENSSL_load_builtin_modules();

@Jan-E

This comment has been minimized.

Copy link
Author

Jan-E commented Feb 1, 2019

Could you try this patch for https://svn.apache.org/viewvc/apr/apr-util/branches/1.6.x/crypto/apr_crypto_openssl.c?view=markup#l127

--- crypto/apr_crypto_openssl.c.orig	2017-10-08 13:32:30.000000000 +0200
+++ crypto/apr_crypto_openssl.c	2019-02-01 23:04:13.083613500 +0100
@@ -30,6 +30,7 @@
 
 #if APU_HAVE_CRYPTO
 
+#include <openssl/ssl.h>
 #include <openssl/evp.h>
 #include <openssl/rand.h>
 #include <openssl/engine.h>
@@ -133,6 +134,8 @@
 #if APR_USE_OPENSSL_PRE_1_1_API
     (void)CRYPTO_malloc_init();
 #else
+    /* SSL_library_init() must be called prior to any other OpenSSL action. */
+    SSL_library_init();
     OPENSSL_malloc_init();
 #endif
     ERR_load_crypto_strings();

My results now:

START: crypto-test.exe
PASS: crypto-test 1: basic password encryption/decryption test
PASS: crypto-test 2: password checktext generation/validation
END: crypto-test.exe
ELAPSED: crypto-test.exe 0:00:00.219000

@Jan-E

This comment has been minimized.

Copy link
Author

Jan-E commented Feb 2, 2019

Log file of building 1.10.4 with VC14, OpenSSL 1.1.1a x86
buildvc14o111_x86.zip

Found apr 1.6.5
Found apr-util 1.6.1
Found apr_memcache 1.6.1
Found expat 2.2.6
Found httpd 2.4.38
Found mod_dav 2.4.38
Found openssl 1.1.1a
Found sasl 2.1.27
Found serf 1.3.9
Found sqlite 3.23.1
Found zlib 1.2.11
Using bundled lz4 1.7.5
Using bundled utf8proc 2.1.0

@nono303

This comment has been minimized.

Copy link
Owner

nono303 commented Feb 2, 2019

Hi @Jan-E,
Thanks for your search!
I built apr_crypto_openssl with your patch and everything is working well so far!
I also put the patched dll in my httpd exec path to see what's going on...
Please test on your side - from my master branch - and if you didn't see regress, I'll update tag 1.11.1

@Jan-E

This comment has been minimized.

Copy link
Author

Jan-E commented Feb 2, 2019

The server in this Apachelounge reply is running with the patched apr_crypto_openssl.dll:
https://www.apachelounge.com/viewtopic.php?p=37841#37841
Seems OK.

@nono303

This comment has been minimized.

Copy link
Owner

nono303 commented Feb 2, 2019

1.11.1 tag updated

@nono303 nono303 closed this Feb 2, 2019

@nono303

This comment has been minimized.

Copy link
Owner

nono303 commented Feb 2, 2019

The server in this Apachelounge reply is running with the patched apr_crypto_openssl.dll:
https://www.apachelounge.com/viewtopic.php?p=37841#37841
Seems OK.
@Jan-E so far so good ^^ https://www.ssllabs.com/ssltest/analyze.html?d=nono303.net

@Jan-E

This comment has been minimized.

Copy link
Author

Jan-E commented Feb 3, 2019

Jan-E added a commit to Jan-E/apr-util that referenced this issue Feb 3, 2019

BZ-63139: crypto_openssl endless loop OpenSSL 1.1
See https://bz.apache.org/bugzilla/show_bug.cgi?id=63139

When building Subversion with apr-util 1.6.1 and OpenSSL 1.1.1a on Windows the crypto-test.exe test ends in an endless loop.

Cause: OPENSSL_malloc_init() is called without preceding SSL_library_init();

Analysis of the bug: nono303/win-svn#3
Comparable report for Serf in the OpenSSL issues:
openssl/openssl#2865

Jan-E added a commit to Jan-E/apr-util that referenced this issue Feb 3, 2019

BZ-63139: endless loop in apr_crypto_openssl.dll
See https://bz.apache.org/bugzilla/show_bug.cgi?id=63139

When building Subversion with apr-util 1.6.1 and OpenSSL 1.1.1a on Windows the crypto-test.exe test ends in an endless loop.

Cause: OPENSSL_malloc_init() is called without preceding SSL_library_init();

Analysis of the bug: nono303/win-svn#3
Comparable report for Serf in the OpenSSL issues:
openssl/openssl#2865
@nono303

This comment has been minimized.

Copy link
Owner

nono303 commented Feb 4, 2019

Just pushed on master apr_crypto_openssl patched with OPENSSL_init_ssl(0, NULL); instead of SSL_library_init();
my httpd & crypto_test run fine with...
@Jan-E if you can test in your side and give me a feedback

@Jan-E

This comment has been minimized.

Copy link
Author

Jan-E commented Feb 7, 2019

openssl/openssl@ef45aa1
If I understand this commit correctly, it should work with the original, unpatched. apr_crypto_ssl (provided you build OpenSSL with the fix).

@nono303

This comment has been minimized.

Copy link
Owner

nono303 commented Feb 12, 2019

Hi @Jan-E,
Just pushed on master (1c5d882) new build with patched openssl 1.1.1a & original apr_util 1.6.1.
Sounds good for me without any patched deps, according to this header fix ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.