Thank you for the answer. We are doing some tests and if they prove meaningful I'll drop a line about this corex_lib append_branch.
Have a nice day,
Lucian
On 13.07.2018 15:09, Daniel-Constantin Mierla wrote:
Hello,
well, not sure at this moment because would take some time to analyze what execution paths can end up there. I did look and I saw that path_vec field is cloned in shm, so doing pkg_free() on that value will fail.
If it ends up to that code when the value is not the cloned one, then it should be ok. On the other hand, the shm clone flag can be set even when path_vec points to a pkg address to due execution on a faked request.
Because of these cases, I just added a helper function to check if a pointer is inside shared memory, see:
* https://github.com/kamailio/kamailio/commit/1a20bcaa35db4aa80d6460dfb0fb9c70...
Maybe this is safer overall and we can change in other parts where we rely on some tricks to see if it is pkg or shm -- in many cases related to processing in context of transaction/tm module, pkg needs to be free and shm is cloned in a continuous block to be freed at once.
If you find this solution acceptable, then you can go ahead and push a commit using shm_address_in(). That might be safe to be added in reset_path_vector() as well, to avoid troubles if someone calls it for a shm cloned shm structure.
Cheers, Daniel
On 13.07.18 13:06, Lucian Balaceanu wrote:
Hi guys,
Seeing the https://github.com/kamailio/kamailio/pull/1144 about fixing a memory leak in the tm module due to incorrectly using FL_SHM_CLONE when freing path_vec memory, isn't there a case that a similar patch should be applied to corex/corex_lib.c ?
Inside corex_append_branch(..) function:
/* if this is a cloned message, don't free the path vector as it was copied into shm memory and will be freed as contiguous block*/ if (!(msg->msg_flags&FL_SHM_CLONE)) { if(msg->path_vec.s!=0) pkg_free(msg->path_vec.s); msg->path_vec.s = 0; msg->path_vec.len = 0; }
Maybe we should switch to:
if(msg->path_vec.s!=0) pkg_free(msg->path_vec.s); msg->path_vec.s = 0; msg->path_vec.len = 0;
If you agree this is the case, I will prepare a pull request.
Thank you, Lucian
Kamailio (SER) - Development Mailing List sr-dev@lists.kamailio.org https://lists.kamailio.org/cgi-bin/mailman/listinfo/sr-dev