#47 crash loading tls in threads

[Jeff Rogers]

I am reliably seeing a crash when loading tls in a lot of threads.

My test script is:
package require Thread

for {set x 0} {$x < 1000000} {incr x} {
thread::create {
package require tls
}
}

I'm using linux 2.6.32 (i386), tclsh8.6b3, Thread 2.7b1, tls 1.6.3, and openssl 0.9.8k

[Jeff Rogers]

I think the issue may be that initialization of the SSL library only needs to be done once per application, not once per thread. The attached patch seems to fix the problem for me.

[Jeff Rogers]

patch to initialize SSL library only once

[Jeffrey Hobbs]

This patch isn't correctly written for the way core Tcl thread commands are defined since at least Tcl 8.4. You should not put Tcl_MutexLock inside the #ifdef TCL_THREADS - it is defined with or without threads. This means TlsLibInit should be streamlined, and LibInit can just be inlined. I'd like to see an updated patch that reflects that.

Also, I'm curious if just the use of the CRYPTO_mem_set_functions is enough to prevent the segfaults, since Tcl in threaded mode will have thread-safe malloc, whereas that may not be the case without setting that.

[Jeff Rogers]

LibInit is inlined in the current version of the code, as part of Tls_Init, which is called in every interpreter the package is loaded into. I broke it out in an attempt to separate the things that need only be called once from the things that should be called in every interp. I believe some of the pieces in LibInit there can safely be called multiple times (such as SSL_load_error_strings), but it is not necessary to do so.

The main problem as I understand it is SSL_library_init(), which the documentation explicitly states is not reentrant.
https://www.openssl.org/docs/ssl/SSL_library_init.html#NOTES

[Jeffrey Hobbs]

I meant inlining into TlsLibInit, not Tls_Init. There is no need for the extra function otherwise.

[Jeff Rogers]

I think I understand what you mean. Is the attached patch more in line with current standards?

[Jeffrey Hobbs]

This looks good. The only change I would make (debatable) is to move initialized=1 to be done first thing after the 2nd !initialized check. This means it will be marked true even for errors, but I think that's correct in this use case.

[Andreas Kupries]

Patch applied, with Jeff's suggestion regarding the location of initialization=1.