Hi,
Recently I found a bug in ts_append() function of the tsilo module. It works for most users until they start using some functions in branch routes of newly created branches. The root of the problem is that static buffer from pv_get_buffer() can be overwritten by some branch_route actions. In the end in the ts_append() lock_entry_by_ruri locks one mutex and unlock_entry_by_ruri unlocks another one. You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/1138
-- Commit Summary --
* Fix memory leak in ldap module that happens after calling ldap_result_next() * Merge branch 'master' of https://github.com/kamailio/kamailio * tsilo: fix deadlock in ts_append()
-- File Changes --
M src/modules/tsilo/tsilo.c (18)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/1138.patch https://github.com/kamailio/kamailio/pull/1138.diff
@vitalikvoip pushed 1 commit.
9c9faeb ipops: new naptr_query function
grumvalski commented on this pull request.
@@ -250,15 +250,25 @@ static int fixup_ts_append(void** param, int param_no)
*/ static int w_ts_append(struct sip_msg* _msg, char *_table, char *_ruri) { - str ruri = {0}; + str tmp = STR_NULL;
I think this change should be done into ts_append, not in the wrapper. What can be changed by branch routes is the ruri that you pass to ts_append, so saving the original one at this level wouldn't solve the issue, if I'm not wrong.
There are two not related commits here. Can you make two different PRs?
vitalikvoip commented on this pull request.
@@ -250,15 +250,25 @@ static int fixup_ts_append(void** param, int param_no)
*/ static int w_ts_append(struct sip_msg* _msg, char *_table, char *_ruri) { - str ruri = {0}; + str tmp = STR_NULL;
Well, ts_append() receives "$tu" from the incoming REGISTER requests. "_ruri" parameter name is a bit confusing in this sense. What about the branch route, it works with an outgoing INVITE, while ts_append() is called for REGISTER. It can be moved to ts_append() to be closer to a new branch creation, but this fix works in both cases.
Just FYI, here is what happens in details: REGISTER comes and we call ts_append(). w_ts_append() calls fixup_get_svalue() which saves the real value to a static buffer from pv_get_buffer(). Then we create a new branch for the previously saved INVITE transaction and its branch route performs script actions and some of them can internally use the same static buffer of this tcp worker process. When branch route returns, we get back to our REGISTER processing and expect that variable "ruri" has the same value, but it's not... So, in a nutshell, the first script invocation (REGISTER) was interrupted by the second script invocation (branch_route of a new INVITE branch) though is wasn't reentrant.
Strange. When I did a PR the second commit with ipops changes wasn't done yet. So I thought I can safely push to github and create a new PR for it. Will read github's docs a bit.
grumvalski commented on this pull request.
@@ -250,15 +250,25 @@ static int fixup_ts_append(void** param, int param_no)
*/ static int w_ts_append(struct sip_msg* _msg, char *_table, char *_ruri) { - str ruri = {0}; + str tmp = STR_NULL;
Ok, I understand now, thanks :) Please do a clean PR and I think it can be merged.
If you push to the same branch for which you did a PR, github will update the PR itself.
Closed #1138.
-
Federico Cabiddu -
vitalikvoip