#### 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 --> - [ ] PR should be backported to stable branches - [ ] Tested changes locally - [ ] Related to issue #1208
#### Description p->headers.t is dynamically allocated header_list_add() in and should be freed with free_async_query().
You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/1407
-- Commit Summary --
* http_async_client: memory mismanagement
-- File Changes --
M src/modules/http_async_client/async_http.c (2)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/1407.patch https://github.com/kamailio/kamailio/pull/1407.diff
@grumvalski , set_query_params() is called in 3 places, for us kamailio is crashing at startup, and I suspect header_list_add(&ah_params.headers, &tval->rs); is sometimes called before set_query_params(&ah_params) in ah_set_req() , thus leading to a kamailio crash at startup, under traffic.
set_quey_params is called in mod_init by init_query_params (https://github.com/kamailio/kamailio/blob/master/src/modules/http_async_clie...), so whenever ah_set_req is called the query_params struct should already have been initialized.
headers.t is a double pointer, the allocated memory is freed later by free_async_query, we are not freeing it inset_query_params. If we don't set it to NULL we end up with a growing headers list (from different request) and we go toward a crash being the memory freed elsewhere.
This being said, do you have a coredump of the crashes? Which OS/libcurl version are you using? Recently a crash at startup had been reported which was due to a combination of libcurl/OS libs: https://github.com/kamailio/kamailio/issues/1391.
is it possible is being set to NULL before headers.t being freed in some case ? I noticed there are checks inside set_query_params() for other already allocated memory buffers . It's crashing randomly at startup, in this scenario, I dont have backtraces anymore.
@dragos-oancea, @grumvalski - did this one ended in some conclusion? Merging or not?
FYI this was kamailio `5.1.0` on debian 8, with stock libcurl (`7.38`). I'm trying to reproduce the issue with `5.1.2` and both libcurl `7.38` and `7.59` (still on debian 8) (including with restarts under load), and I will update here with any relevant information.
Pinging again to see if anything else was left to be addressed here.
@giavac - any updates on this? Were you able to reproduce this with a newer 5.1.x version?
I've been able to reproduce the crash that lead to Dragos analysis and this PR, both in debian 8 and in CentOS 7.
But I tracked it down to a different cause, which is `libcurl` crashing when using the threaded resolver for DNS lookups. This happens even with latest `libcurl`.
The work around this was building `libcurl` with `c-ares` resolver, and that proved to solve the issue. This included tests under heavy load (hundreds of requests per second).
I meant to write my findings to the libcurl mailing list and ask for corroboration or indications, but I haven't had time yet, as bringing up the failing and working scenarios in a clean way to be shared with that ml is very time consuming.
For what concerns this specific Pull Request I haven't done any test/verification. I don't have it in the environments with the c-ares fix and there are no crashes, so perhaps it's not needed. The only thing I can suggest is that next time I do the load testing, I use the patch in this PR to verify there are no side effects.
@grumvalski , @giavac , @dragos-oancea - can this be closed or there is still something here to work on?
I am closing it, long time no activity and no similar reports. Reopen if still needs to be addressed.
Closed #1407.
-
Daniel-Constantin Mierla -
Dragos Oancea
-
Federico Cabiddu
-
Giacomo Vacca
-
Henning Westerholt