1 Feb
2024
1 Feb
'24
4:16 a.m.
#### 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
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3740
You are receiving this because you are subscribed to this thread.
Message ID: &lt;kamailio/kamailio/pull/3740(a)github.com&gt;
1 Feb
1 Feb
9:03 a.m.
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.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3740#issuecomment-1920638330
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3740/c1920638330(a)github.com>
12:49 p.m.
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.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3740#issuecomment-1921039355
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3740/c1921039355(a)github.com>
1:07 p.m.
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?
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3740#issuecomment-1921071498
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3740/c1921071498(a)github.com>
1:38 p.m.
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.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3740#issuecomment-1921127455
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3740/c1921127455(a)github.com>
4:26 p.m.
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.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3740#issuecomment-1921459905
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3740/c1921459905(a)github.com>
4 Feb
4 Feb
2:31 a.m.
Closed #3740.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3740#event-11693780710
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3740/issue_event/11693780710(a)github.com>
2:31 a.m.
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.)
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3740#issuecomment-1925501489
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3740/c1925501489(a)github.com>
323
days inactive
326
days old
7 comments
3 participants
participants (3)
-
Daniel-Constantin Mierla -
Henning Westerholt
-
space88man