[sr-dev] Remove check for msg->msg_flags inside corex module ?

[Lucian Balaceanu]

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/1a20bcaa35db4aa80d6460dfb0fb9c70026248c9
>
> 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 at lists.kamailio.org
>> https://lists.kamailio.org/cgi-bin/mailman/listinfo/sr-dev