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

Crash on module unload #239

Closed
dwmw2 opened this issue Sep 20, 2016 · 3 comments
Closed

Crash on module unload #239

dwmw2 opened this issue Sep 20, 2016 · 3 comments
Labels
Milestone

Comments

@dwmw2
Copy link
Contributor

dwmw2 commented Sep 20, 2016

I tried using the SoftHSMv2 module with 'openssl rsautl'...

 SOFTHSM2_CONF=softhsm2.conf PKCS11_MODULE_PATH=/home/dwmw2/git/SoftHSMv2/src/lib/.libs/libsofthsm2.so \
  gdb --args openssl rsautl -engine pkcs11 -keyform engine \
      -inkey 'pkcs11:token=openconnect-test;id=%01;pin-value=1234' -sign -in foo -out bar 

It works, but then dies on exit:

engine "pkcs11" set.

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff547e990 in ?? ()
(gdb) bt
#0  0x00007ffff547e990 in ?? ()
#1  0x00007ffff6e0c312 in CRYPTO_THREADID_current (id=0x7fffffffd490) at cryptlib.c:493
#2  0x00007ffff6ec6468 in ERR_remove_thread_state (id=id@entry=0x0) at err.c:997
#3  0x0000000000419d42 in main (Argc=<optimized out>, Argv=<optimized out>) at openssl.c:437

Environment is Fedora 24 (OpenSSL 1.0.2), with updated libp11/engine_pkcs11 and SoftHSMv2 from latest git. Not that the latter seem to make any difference.

@dwmw2
Copy link
Contributor Author

dwmw2 commented Sep 20, 2016

Adding a CRYPTO_set_id_callback(NULL) to OSSLCryptoFactory::~OSSLCryptoFactory() seems to fix this, although I'm not sure any of that is really well-behaved given the the application may be using OpenSSL and its own threading model.

@dwmw2
Copy link
Contributor Author

dwmw2 commented Sep 21, 2016

It looks like CRYPTO_set_id_callback() has been deprecated since OpenSSL 1.0.0 and shouldn't be needed at all. Since 1.0.0 (actually commit openssl/openssl@4c32969 in 2008) OpenSSL has had its own fallbacks which include using the address of errno. Surely that's going to work on any platform with pthreads?
And since we are only calling CRYPTO_set_id_callback() when HAVE_PTHREAD_H is set, that makes it entirely redundant?

dwmw2 pushed a commit to dwmw2/SoftHSMv2 that referenced this issue Sep 21, 2016
We use CRYPTO_set_id_callback() to set a callback, but we don't ever
remove it again on unload. So OpenSSL crashes the next time it needs a
thread-id.

CRYPTO_set_id_callback() has been deprecated since OpenSSL 1.0.0, the
oldest we support. And redundant too, since OpenSSL has fallbacks which
include the address of errno. Which is going to work on any platform
with pthreads... and we were only calling CRYPTO_set_id_callback() on
platforms with pthreads.

So just rip it out.
dwmw2 pushed a commit to dwmw2/SoftHSMv2 that referenced this issue Sep 21, 2016
We use CRYPTO_set_id_callback() to set a callback, but we don't ever
remove it again on unload. So OpenSSL crashes the next time it needs a
thread-id.

CRYPTO_set_id_callback() has been deprecated since OpenSSL 1.0.0, the
oldest we support. And redundant too, since OpenSSL has fallbacks which
include the address of errno. Which is going to work on any platform
with pthreads... and we were only calling CRYPTO_set_id_callback() on
platforms with pthreads.

So just rip it out.
bellgrim added a commit that referenced this issue Sep 23, 2016
Issue #239: Crash on module unload with OpenSSL
@bellgrim
Copy link
Contributor

Thanks, the patch has been merged.

@bellgrim bellgrim added the bug label Sep 23, 2016
@bellgrim bellgrim added this to the 2.2.0 milestone Sep 23, 2016
@bellgrim bellgrim removed the bug label Sep 23, 2016
@bellgrim bellgrim removed this from the 2.2.0 milestone Sep 23, 2016
@bellgrim bellgrim added the bug label Sep 23, 2016
@bellgrim bellgrim added this to the 2.2.0 milestone Sep 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants