Hello,
There seems to be a design problem in proxy.c mk_proxy(str* name, unsigned short port, unsigned short proto, int is_sips) function. The purpose of this function is to create a proxy and return a pointer to the created structure.
The issue arises from the fact that the name (type struct str) member of the proxy structure is not deep copied from the given parameters(refer to the str* name ) (the hostent structure is instead deep copied). This isn't a problem for now but I have worked on a small patch that caches proxies (using add_proxy() and find_proxy()) and ,because of this shallow copy, things are broken.
I said that this is a design problem because we can let the shallow copy happen (performance is better), and when needed the caller should provide a buffer that doesn't change (let him do the copy instead). This is not clearly documented but done from /modules(_k)/utils/conf.c. But it this way we may have a memory leak when the proxy is deallocated, because I doubt that the caller keeps track of the allocated buffers(the code in proxy.c doesn't take ownership of the given pointer).
This affects functions mk_proxy and mk_shm_proxy in both kamailio(1.5 to speak of) and sip-router.
I have created a patch that also does a deep copy of the name, thus removing the need for the caller to bother about the lifetime of the name buffer.
Any ideas?!
Cheers Marius
Hello,
On 1/26/10 5:02 PM, marius zbihlei wrote:
Hello,
There seems to be a design problem in proxy.c mk_proxy(str* name, unsigned short port, unsigned short proto, int is_sips) function. The purpose of this function is to create a proxy and return a pointer to the created structure.
The issue arises from the fact that the name (type struct str) member of the proxy structure is not deep copied from the given parameters(refer to the str* name ) (the hostent structure is instead deep copied). This isn't a problem for now but I have worked on a small patch that caches proxies (using add_proxy() and find_proxy()) and ,because of this shallow copy, things are broken.
how is it going to be? Each structure will have a reference counter to ensure is not deleted while still in use? Is the structure in shm?
I said that this is a design problem because we can let the shallow copy happen (performance is better), and when needed the caller should provide a buffer that doesn't change (let him do the copy instead). This is not clearly documented but done from /modules(_k)/utils/conf.c. But it this way we may have a memory leak when the proxy is deallocated, because I doubt that the caller keeps track of the allocated buffers(the code in proxy.c doesn't take ownership of the given pointer).
This affects functions mk_proxy and mk_shm_proxy in both kamailio(1.5 to speak of) and sip-router.
I have created a patch that also does a deep copy of the name, thus removing the need for the caller to bother about the lifetime of the name buffer.
Any ideas?!
Seems ok for me, maybe Andrei can comment better in relation with tm and other core functions.
Did you (intend to) attach the patch or you just mentioned it.
Cheers, Daniel
Hello, Sorry again for top-posting, web client Agrrr....
I will push the patch to upstream tomorrow, if nobody has anything against it. Along with the proxy caching functionality that I described in the previous mail today, they are for now in a single patch, so i need a little time to break it in two, and only commit this atm.
I am for a deep copy of the name. My reason is that also the mk_shm_proxy funtion requires (even if documentation says nothing) the name parameter to point to something in shared memory(as to be addressable by other processes)
As I said, I think that the proxy object abstractization should have ownership of all it's members.
Greetings Marius
-----Original Message----- From: Daniel-Constantin Mierla [mailto:miconda@gmail.com] Sent: Tue 1/26/2010 7:11 PM To: Marius Zbihlei Cc: sr-dev@lists.sip-router.org; users@lists.kamailio.org Subject: Re: [Kamailio-Users] core:proxy.c problem
Hello,
On 1/26/10 5:02 PM, marius zbihlei wrote:
Hello,
There seems to be a design problem in proxy.c mk_proxy(str* name, unsigned short port, unsigned short proto, int is_sips) function. The purpose of this function is to create a proxy and return a pointer to the created structure.
The issue arises from the fact that the name (type struct str) member of the proxy structure is not deep copied from the given parameters(refer to the str* name ) (the hostent structure is instead deep copied). This isn't a problem for now but I have worked on a small patch that caches proxies (using add_proxy() and find_proxy()) and ,because of this shallow copy, things are broken.
how is it going to be? Each structure will have a reference counter to ensure is not deleted while still in use? Is the structure in shm?
I said that this is a design problem because we can let the shallow copy happen (performance is better), and when needed the caller should provide a buffer that doesn't change (let him do the copy instead). This is not clearly documented but done from /modules(_k)/utils/conf.c. But it this way we may have a memory leak when the proxy is deallocated, because I doubt that the caller keeps track of the allocated buffers(the code in proxy.c doesn't take ownership of the given pointer).
This affects functions mk_proxy and mk_shm_proxy in both kamailio(1.5 to speak of) and sip-router.
I have created a patch that also does a deep copy of the name, thus removing the need for the caller to bother about the lifetime of the name buffer.
Any ideas?!
Seems ok for me, maybe Andrei can comment better in relation with tm and other core functions.
Did you (intend to) attach the patch or you just mentioned it.
Cheers, Daniel
Hello,
On 1/26/10 8:10 PM, Marius Zbihlei wrote:
Hello, Sorry again for top-posting, web client Agrrr....
I will push the patch to upstream tomorrow, if nobody has anything against it.
I would wait a bit, not sure what are the exact changes, but I prefer to have most of updates from 3.0 branches integrated first, to avoid conflicts (Andrei just resumed the process). Besides that the function is pretty used across the code, should be reviewed carefully.
Thanks, Daniel
Along with the proxy caching functionality that I described in the previous mail today, they are for now in a single patch, so i need a little time to break it in two, and only commit this atm.
I am for a deep copy of the name. My reason is that also the mk_shm_proxy funtion requires (even if documentation says nothing) the name parameter to point to something in shared memory(as to be addressable by other processes)
As I said, I think that the proxy object abstractization should have ownership of all it's members.
Greetings Marius
-----Original Message----- From: Daniel-Constantin Mierla [mailto:miconda@gmail.com] Sent: Tue 1/26/2010 7:11 PM To: Marius Zbihlei Cc: sr-dev@lists.sip-router.org; users@lists.kamailio.org Subject: Re: [Kamailio-Users] core:proxy.c problem
Hello,
On 1/26/10 5:02 PM, marius zbihlei wrote:
Hello,
There seems to be a design problem in proxy.c mk_proxy(str* name, unsigned short port, unsigned short proto, int is_sips) function. The purpose of this function is to create a proxy and return a pointer to the created structure.
The issue arises from the fact that the name (type struct str) member of the proxy structure is not deep copied from the given parameters(refer to the str* name ) (the hostent structure is instead deep copied). This isn't a problem for now but I have worked on a small patch that caches proxies (using add_proxy() and find_proxy()) and ,because of this shallow copy, things are broken.
how is it going to be? Each structure will have a reference counter to ensure is not deleted while still in use? Is the structure in shm?
I said that this is a design problem because we can let the shallow copy happen (performance is better), and when needed the caller should provide a buffer that doesn't change (let him do the copy instead). This is not clearly documented but done from /modules(_k)/utils/conf.c. But it this way we may have a memory leak when the proxy is deallocated, because I doubt that the caller keeps track of the allocated buffers(the code in proxy.c doesn't take ownership of the given pointer).
This affects functions mk_proxy and mk_shm_proxy in both kamailio(1.5 to speak of) and sip-router.
I have created a patch that also does a deep copy of the name, thus removing the need for the caller to bother about the lifetime of the name buffer.
Any ideas?!
Seems ok for me, maybe Andrei can comment better in relation with tm and other core functions.
Did you (intend to) attach the patch or you just mentioned it.
Cheers, Daniel
-- Daniel-Constantin Mierla
Kamailio (OpenSER) - Users mailing list Users@lists.kamailio.org http://lists.kamailio.org/cgi-bin/mailman/listinfo/users http://lists.openser-project.org/cgi-bin/mailman/listinfo/users
-
Daniel-Constantin Mierla -
marius zbihlei