Bug#870018: [Pkg-openssl-devel] Bug#870018: openssl: SIGSEGV/coredump on process stop when TLS is enabled in kamailio

[Kurt Roeckx]

reassign 870018 kamailio
thanks

On Wed, Aug 02, 2017 at 05:01:01PM +0200, Guillem Jover wrote:
> Control: tags -1 patch
> 
> Hi!
> 
> On Sat, 2017-07-29 at 09:11:53 +0200, Kurt Roeckx wrote:
> > On Sat, Jul 29, 2017 at 12:12:16AM +0200, Michael Prokop wrote:
> > > Kurt, do you have any ideas what might go wrong in OPENSSL_cleanup
> > > here and how this could be fixed? We'd appreciate any hints. Thanks!
> 
> > I don't see anything obvious wrong. From what I understand it
> > calls exit(0) from a SIGTERM handler.
> > 
> > The only suggestion I have is that you try to run this under
> > valgrind or something.
> > 
> > Also feel free to open a github issue about this.
> 
> I was checking this, and my initial hypothesis which I've not yet
> confirmed, but seems completely spot on, given the documentation I've
> just read about OPENSSL_cleanup() is that something within the pthreads
> library is releasing the memory pool for the affected variable that is
> segfaulting in OpenSSL, and when the OpenSSL atexit() handler gets
> called the pthreads variable is already gone.
> 
> The attached patch fixes the segfault for me, and seems to be in line
> with the recommendations in the OPENSSL_cleanup() docs. I should
> probably update the comment further. And I guess this report should be
> reassigned to the kamailio package then.
> 
> I'm not sure whether calling OPENSSL_thread_stop() would be more
> correct here, as I don't know how threads are being used, and how the
> TLS module interacts with the rest of the codebase, etc.

>From reading the documentation, I think OPENSSL_thread_stop()
should have been called because otherwise it doesn't know it's
variable already got removed. So I'm reassigning it.


Kurt

Bug#870018: [Pkg-openssl-devel] Bug#870018: openssl: SIGSEGV/coredump on process stop when TLS is enabled in kamailio

[Guillem Jover]

Control: tags -1 patch

Hi!

On Sat, 2017-07-29 at 09:11:53 +0200, Kurt Roeckx wrote:
> On Sat, Jul 29, 2017 at 12:12:16AM +0200, Michael Prokop wrote:
> > Kurt, do you have any ideas what might go wrong in OPENSSL_cleanup
> > here and how this could be fixed? We'd appreciate any hints. Thanks!

> I don't see anything obvious wrong. From what I understand it
> calls exit(0) from a SIGTERM handler.
> 
> The only suggestion I have is that you try to run this under
> valgrind or something.
> 
> Also feel free to open a github issue about this.

I was checking this, and my initial hypothesis which I've not yet
confirmed, but seems completely spot on, given the documentation I've
just read about OPENSSL_cleanup() is that something within the pthreads
library is releasing the memory pool for the affected variable that is
segfaulting in OpenSSL, and when the OpenSSL atexit() handler gets
called the pthreads variable is already gone.

The attached patch fixes the segfault for me, and seems to be in line
with the recommendations in the OPENSSL_cleanup() docs. I should
probably update the comment further. And I guess this report should be
reassigned to the kamailio package then.

I'm not sure whether calling OPENSSL_thread_stop() would be more
correct here, as I don't know how threads are being used, and how the
TLS module interacts with the rest of the codebase, etc.

Thanks,
Guillem
Description: Explicitly call OPENSSL_cleanup() on TLS module destruction
 Kamailio is a threaded program, and it pulls OpenSSL via a dynamically
 linked plugin loaded at run-time via dlopen().
 .
 Older OpenSSL releases do not perform automatic cleanup on exit, newer ones
 do via an atexit() handler.
 .
 In addition the OpenSSL code leaks (on purpose) a DSO handler for itself so
 that it can guarantee that the library is still present when the atexit()
 handler is executed.
 .
 When the handler is called, a pthreads variable is still present but the
 memory it points to is not allocated anymore, which indicates something else
 released those pthreads pools. Most probably pthreads itself as part of its
 deconstructors, pthread_exit() or atexit() handlers while unwinding or
 being unloaded.
 .
 While that might be a bug that might need to be tracked independently,
 explicitly calling the OpenSSL cleanup code fixes the issue for newer
 OpenSSL releases. And the documentation for OPENSSL_cleanup() mentions
 that explicit cleanup might be needed in these conditions anyway.
Origin: other, Sipwise
Author: Guillem Jover 
Bug-Debian: https://bugs.debian.org/870018

---

--- a/src/modules/tls/tls_init.c
+++ b/src/modules/tls/tls_init.c
@@ -778,4 +778,8 @@ void destroy_tls_h(void)
 	tls_destroy_cfg();
 	tls_destroy_locks();
 	tls_ct_wq_destroy();
+
+#if OPENSSL_VERSION_NUMBER >= 0x01010L
+	OPENSSL_cleanup();
+#endif
 }

Bug#870018: [Pkg-openssl-devel] Bug#870018: openssl: SIGSEGV/coredump on process stop when TLS is enabled in kamailio

[Kurt Roeckx]

On Sat, Jul 29, 2017 at 12:12:16AM +0200, Michael Prokop wrote:
> 
> Kurt, do you have any ideas what might go wrong in OPENSSL_cleanup
> here and how this could be fixed? We'd appreciate any hints. Thanks!

I don't see anything obvious wrong. From what I understand it
calls exit(0) from a SIGTERM handler.

The only suggestion I have is that you try to run this under
valgrind or something.

Also feel free to open a github issue about this.


Kurt