[kamailio/kamailio] tm:t_lookup_callid use only trans Call-ID header value to match with … (PR #3393)
…callid param
<!-- Kamailio Pull Request Template -->
<!-- IMPORTANT: - for detailed contributing guidelines, read: https://github.com/kamailio/kamailio/blob/master/.github/CONTRIBUTING.md - pull requests must be done to master branch, unless they are backports of fixes from master branch to a stable branch - backports to stable branches must be done with 'git cherry-pick -x ...' - code is contributed under BSD for core and main components (tm, sl, auth, tls) - code is contributed GPLv2 or a compatible license for the other components - GPL code is contributed with OpenSSL licensing exception -->
#### 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 --> - [ ] Commit message has the format required by CONTRIBUTING guide - [ ] Commits are split per component (core, individual modules, libs, utils, ...) - [ ] Each component has a single commit (if not, squash them into one commit) - [ ] 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 --> - [x ] PR should be backported to stable branches - [x ] Tested changes locally - [ ] Related to issue #XXXX (replace XXXX with an open issue number)
#### Description <!-- Describe your changes in detail --> Using kamailio 5.4 version, we have had an issue with a carrier that sent a CANCEL with to-tag. To be able to process that request, we used the function t_cancel_callid, to convert that abnormal CANCEL to a regular one cancelling the current transaction for that call. But we saw that it was not working since they sent the callid header named as CALL-ID. Seems the matching between the stored callid header in memory and the param passed for the callid lookup matching, is being done in case sensitive way with a generated Call-ID: XXXXXXXX based on the called param of the function.
we have tested a wasy to do the comparision only with the callid header value, so we strip from the callid header stored at memory the Call-ID: (or CALL-ID:) part.
maybe there is another way to do that in other part of the code, but seems at least this worked for the tests
thanks a lot and regards david
You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/3393
-- Commit Summary --
* tm:t_lookup_callid use only trans Call-ID header value to match with callid param
-- File Changes --
M src/modules/tm/t_lookup.c (10) M src/modules/tm/t_msgbuilder.c (9) M src/modules/tm/t_msgbuilder.h (1)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/3393.patch https://github.com/kamailio/kamailio/pull/3393.diff
Thanks for the PR. The patch uses standard malloc, which is not the right choice in the tm module. So I understand the problem is that the header is stored inside the transaction as "Call-ID", but the provider sends back it as "CALL-ID"? What about just using then strncasecmp (similar as for the CSEQ) for the Call-Id field? Are you able to test a patch?
Answering to myself, comparing the Call-Id case-insensitive is not allowed from the standard: "Call-IDs are case-sensitive and are simply compared byte-by-byte." The header field name is case-insensitive.
hello Henning
thanks for the update. Yes i actually was not sure if 2 different calls can have 2 callid which only differs by having capital or lowercase letters. Also i added the line to remove the spaces, to make the function work in cases were the header has an space or not after "Call-ID:" part.
About the memory manager, if it's used shm_malloc instead of malloc only, `char* cid_value = (char*)shm_malloc(sizeof(char) * (cid_value_len + 1));` would be ok?
Or would be better something like ``` char* cid_value = (char*)pkg_malloc(sizeof(char) * (cid_value_len + 1)); ..... pkg_free(cid_value); (after retrieving only_callid value) ``` ?
thanks a lot and regards david
The exiting code seems inflexible indeed, but also the PR does not feel clean. I will think about if there would be a better way to update.
Thanks for the update Daniel
Are the src/modules/tm/t_lookup.c conflicts dued to changes you did? I see a new callid_val.s struct field I think, I guess to do the callid match not depending on the header name.
best regards david
Indeed, I tried to come with an alternative that does not depend on header names. I haven't had the time to test yet, maybe you can do and report the results.
Hello.
i will try. Are all the changes in master branch, or in another one?
@descartin Yes, usually this kind of changes are put into master branch, you can use this or cherry-pick them to your test branch etc..
Closed #3393.
Closing this one, if the alternative implementation has problems, open in issue on the tracker.
thanks Daniel
I will try the changes you made as soon as I can and I will let you know if i have any issue. Meanwhile I could find a workaround for this without using the patch i proposed, by rewritting the Call-ID header for just some particular calls. But this solution is just a temporary fix. I will try your changes
regards david
Hello Daniel
sorry about the delay, just one question the changes you made about this issue, are all ot these ``` commit c7e228eae76a432ea93fac7e95f50fe50979d79e Author: Daniel-Constantin Mierla miconda@gmail.com Date: Fri Mar 31 18:34:00 2023 +0200
pua: updates for the new t field names
commit c321f28ff657c921b8de2b84e66908f281d35d78 Author: Daniel-Constantin Mierla miconda@gmail.com Date: Fri Mar 31 18:22:23 2023 +0200
tm: use header attribute shortcuts to match inside t_lookup_callid()
commit 2bd1227b2440dd376c18e6c964f569e0e9ba4730 Author: Daniel-Constantin Mierla miconda@gmail.com Date: Fri Mar 31 15:18:13 2023 +0200
tm: keep shortcut the cseq header method
commit 734fd2910cd437205da870ad1e329eaefe2f043a Author: Daniel-Constantin Mierla miconda@gmail.com Date: Fri Mar 31 08:05:10 2023 +0200
sca: updates for the new t field names
commit ecd906dabbc5dc19d0fabb35d22320e2ba3fccf0 Author: Daniel-Constantin Mierla miconda@gmail.com Date: Fri Mar 31 08:04:59 2023 +0200
lcr: updates for the new t field names
commit 8f39d0ff741a3ba5819c6c0f96b8f575d30ab770 Author: Daniel-Constantin Mierla miconda@gmail.com Date: Fri Mar 31 08:04:43 2023 +0200
keepalive: updates for the new t field names
commit 927a2451e084e886d2548fdae3f5f4cd3abd0a4b Author: Daniel-Constantin Mierla miconda@gmail.com Date: Fri Mar 31 08:04:11 2023 +0200
dispatcher: updates for the new t field names
commit 8d69c0d68347198ccaea57a8ac4eeb040d19287a Author: Daniel-Constantin Mierla miconda@gmail.com Date: Fri Mar 31 08:02:10 2023 +0200
tm: rename shortcuts to from/to/callid/cseq headers
- reflect better that they are with header names - new fields to point to callid value and cseq number to facilitate use of them directly without new parsing or printing with header names ``` ? Apart of testing directly on master branch, if I tested on 5.4 for instance, should I cherry pick all of them? (asuming there are no conflicts) thanks a lot and regards david
-
Daniel-Constantin Mierla -
descartin
-
Henning Westerholt