[kamailio/kamailio] http_async_client: memory mismanagement (#1407)
#### 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 -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/pull/1407
@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. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/pull/1407#issuecomment-359874351
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. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/pull/1407#issuecomment-360041430
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. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/pull/1407#issuecomment-360094489
@dragos-oancea, @grumvalski - did this one ended in some conclusion? Merging or not? -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/pull/1407#issuecomment-381318126
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. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/pull/1407#issuecomment-381539603
Pinging again to see if anything else was left to be addressed here. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/pull/1407#issuecomment-403415451
@giavac - any updates on this? Were you able to reproduce this with a newer 5.1.x version? -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/pull/1407#issuecomment-425073994
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. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/pull/1407#issuecomment-425080622
@grumvalski , @giavac , @dragos-oancea - can this be closed or there is still something here to work on? -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/pull/1407#issuecomment-446511625
I am closing it, long time no activity and no similar reports. Reopen if still needs to be addressed. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/pull/1407#issuecomment-546318778
Closed #1407. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/pull/1407#event-2743896810
participants (5)
-
Daniel-Constantin Mierla -
Dragos Oancea
-
Federico Cabiddu
-
Giacomo Vacca
-
Henning Westerholt