[OpenSIPS-Devel] [ opensips-Bugs-3411434 ] lack of free_to_params in many places

[SourceForge.net]

Bugs item #3411434, was opened at 2011-09-19 14:28
Message generated for change (Comment added) made by vladut-paiu
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=1086410&aid=3411434&group_id=232389

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: core
Group: trunk
>Status: Closed
Resolution: None
Priority: 5
Private: No
Submitted By: Walter Doekes (wdoekes)
Assigned to: Vladut-Stefan Paiu (vladut-paiu)
Summary: lack of free_to_params in many places

Initial Comment:
Hi,

free_to_params is not called (implicitly) nearly as often as it should.

See attached patch.

Note: I removed the free_to_params from parse_to_params as it could cause double frees. Convention seems to be that one should call it.
Note: I only tested the parse_to functionality. Other parts of the patch are checked for compilation errors only.

Version 1.6 is also affected (as probably others too).

Regards,
Walter Doekes
OSSO B.V.

See also: http://sip-router.org/tracker/index.php?do=details&task_id=155

----------------------------------------------------------------------

>Comment By: Vladut-Stefan Paiu (vladut-paiu)
Date: 2011-09-29 13:34

Message:
Hello,

Sorry for forgetting a part of the patch while trimming it. As we have
talked, everything is fine now in trunk and 1.7.
I have also applied the patch in the 1.6 branch.

Thanks and Regards,
Vlad

----------------------------------------------------------------------

Comment By: Walter Doekes (wdoekes)
Date: 2011-09-21 21:52

Message:

Why do I have to double check these things?

You forgot half of the functionality. I can still get current svn to run
out of memory. 

$ sudo opensips -V | grep svnrevision
svnrevision: 2:8377
$ pidof opensips || echo nothing running
nothing running
$ sudo opensips >/dev/null && echo started
started
$ sipsak -s sip:localhost && echo ok
ok

$ time ./my-modified-sipsak -Fs sip:localhost 
warning: redirects are not expected in flood. disableing
^C
real	0m5.251s
user	0m3.430s
sys	0m1.410s

$ tail /var/log/syslog | grep memory
Sep 21 20:43:57 walter-laptop opensips[7307]: ERROR:core:get_hdr_field:
out of pkg memory
Sep 21 20:43:57 walter-laptop opensips[7307]: ERROR:core:get_hdr_field:
out of pkg memory
Sep 21 20:43:57 walter-laptop opensips[7307]: ERROR:core:get_hdr_field:
out of pkg memory

$ sipsak -s sip:localhost || echo not ok
not ok


I'm sorry, but this is not the first time that people with commit powers
"fix" my patch and break it instead. You might understand the frustration.

Walter

----------------------------------------------------------------------

Comment By: Vladut-Stefan Paiu (vladut-paiu)
Date: 2011-09-21 18:36

Message:
Hello,

I have just committed a fix for this issue in trunk and in the 1.7 branch.
The parameters are freed internally in case the parsing of the TO header
fails, as before, but the param freeing function sets the pointers to NULL
upon freeing.

Thank you for reporting this.
Regards.
Vlad


----------------------------------------------------------------------

Comment By: Walter Doekes (wdoekes)
Date: 2011-09-21 16:10

Message:
Sure. I don't mind either way.

However, for the sake of arguing I could say the following: on success,
it's still your own memory, but free_to() free's it. What's up with that?
;)

What I do care about is that if you *do* free_to_params on error, I think
param_lst should be (re)set to 0. That way those that *do* call free_to,
won't get a double free.

Regards,
Walter Doekes

----------------------------------------------------------------------

Comment By: Vladut-Stefan Paiu (vladut-paiu)
Date: 2011-09-21 14:13

Message:
Hello Walter,

Thank you for the patch, but I have to disagree a little.
I find it more logical that, when parse_to returns error, it still does
free_to_params() internally. So, in case of error, parse_to will free the
memory that it allocated, and it is your responsability to deallocate your
own memory.
In case of success, it is your responsability to free both your memory,
and the memory allocated on the fly by parse_to.

Still, some elements in your patch remain valid ( leak in osp and b2b ). I
will take a closer look and fix those.

Regards,
Vlad

Regards,
Vlad


----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=1086410&aid=3411434&group_id=232389