#### Pre-Submission Checklist <!-- Go over all points below, and after creating the PR, tick all the checkboxes that apply --> <!-- All points should be verified, otherwise, read the CONTRIBUTING guidelines from above--> <!-- If you're unsure about any of these, don't hesitate to ask on sr-dev mailing list --> - [X] Commit message has the format required by CONTRIBUTING guide - [X] Commits are split per component (core, individual modules, libs, utils, ...) - [X] Each component has a single commit (if not, squash them into one commit) - [X] No commits to README files for modules (changes must be done to docbook files in `doc/` subfolder, the README file is autogenerated)
#### Type Of Change - [X] Small bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds new functionality) - [ ] Breaking change (fix or feature that would change existing functionality)
#### Checklist: <!-- Go over all points below, and after creating the PR, tick the checkboxes that apply --> - [ ] PR should be backported to stable branches - [x] Tested changes locally - [ X] Related to issue #3738 (replace XXXX with an open issue number)
#### Description 5.7.4 has OpenSSL fixes for tls and outbound but missed out the db modules—the most common users of libssl
This PR updates db_mysql, db_unixodbc, db_postgres to init libssl in a thread to handle more configurations (tls + db module)
You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/3740
-- Commit Summary --
* core: backport OpenSSL thread-local fixes * db_mysql: backport OpenSSL thread-local fixes * db_postgres: backport OpenSSL thread-local fixes * db_unixodbc: backport OpenSSL thread-local fixes
-- File Changes --
A src/core/rthreads.h (100) M src/modules/db_mysql/km_dbase.c (9) M src/modules/db_postgres/km_dbase.c (19) M src/modules/db_unixodbc/dbase.c (11)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/3740.patch https://github.com/kamailio/kamailio/pull/3740.diff
Thanks for the PR. It adds a new core implementation, which are usually not introduced in stable releases. As most people are not using databases with SSL, I am not sure if we really need to introduce this in the stable branch. We already got some regressions (e.g. #3737) in the TLS module in the stable branch from this type of changes. Lets see what other developers think about it.
Unfortunately adapting to work properly with changes brought by libssl3+ was not easy, but it might not be a simpler way for avoiding those random crashes. I really appreciate the time and effort @space88man spent on it.
This backported PR is not doing any fundamental change, it just makes common wrappers that can be used from the modules directly, instead of having those wrappers in each module. Those functions are not used in core, just placed there to be available everywhere. For consistency with devel version (upcoming 5.8 as well) , I think it should be merged.
I of course also appreciate the work @space88man put here into that. Maybe I am wrong, but right now the db_* modules in stable branches are not using the pthread approach. My question was about if its really necessary to apply this pthread logic to the DB modules. If the db_* modules are not using TLS, I would expect no problems, or I am wrong?
Well, that's the problem actually, because the db module can use tls if, for example, connection to MySQL/MariaDB is done over TLS. That was reported for db_unixodbc when connecting to Microsoft SQL Server via TLS. Any other module that eventually ends up using libssl3 though chain of dependencies can have similar impact. Also in the past there were reports that can be related:
- https://github.com/kamailio/kamailio/issues/3115 - https://github.com/kamailio/kamailio/issues/2560
Solutions were found at the times for respective versions, but with libssl3 this might be the right approach, or at least the one we could explore now.
Anyhow, I expected those patches to be backported to have the fix completed, right now at least the db_unixodbc is reported to be broken.
I of course also appreciate the work @space88man put here into that. Maybe I am wrong, but right now the db_* modules in stable branches are not using the pthread approach. My question was about if its really necessary to apply this pthread logic to the DB modules. If the db_* modules are not using TLS, I would expect no problems, or I am wrong?
When troubleshooting #3727 the behaviour of the MySQL and MariaDB ODBC connectors is that they will initialise libssl/libcrypto even if not using TLS subsequently. However this will cascading effects on `tls.so` and other modules that use TLS since the thread-locals will be init'ed in process_no = 0.
I.e. the database modules, while not using SSL themselves, can contaminate the thread-locals of main process and the workers will be affected.
Closed #3740.
Thank you for your comments — I will proceed with these commits as they are necessary for client TLS (any module using libssl3) coexistence, i.e., even if not using kamailio as TLS server, the various client modules can contaminate the main thread-local state.
The commits are non-intrusive — they run the db_XXXX_init functions in a thread.
I have validated: 5.7.3/5.7.4 with tls.so and client SSL will fail intermittently using libssl1; using libssl3 the config will not startup (race condition GH #3727).
With these patches and on master GH #3727 (tls.so + dispatcher/unixodbc+SSL) now works cleanly (no more shm double free errors).
(Unfortunately the libssl3 db patches did not make it into 5.7.4 so this is to catch-up with the fixes on master.)
-
Daniel-Constantin Mierla -
Henning Westerholt
-
space88man