[asterisk-dev] [Code Review]: Fix possible memory leak with SIP realtime regs

[Terry Wilson]

> On Sept. 29, 2011, 4:03 p.m., wdoekes wrote:
> > /branches/1.8/channels/chan_sip.c, line 4713
> > <https://reviewboard.asterisk.org/r/1465/diff/2/?file=20922#file20922line4713>
> >
> >     once more. if we get sipregs entry but no sippeers entry, then we have a leak.

same as above.


> On Sept. 29, 2011, 4:03 p.m., wdoekes wrote:
> > /branches/1.8/channels/chan_sip.c, lines 4665-4666
> > <https://reviewboard.asterisk.org/r/1465/diff/2/?file=20922#file20922line4665>
> >
> >     this is still leaky if there is a sipregs entry without matching sippeers entry.
> >     
> >     simply moving this statement down a bit will help. no one is using the varregs yet.

Every single return in the function is preceeded by an ast_variables_destroy(varregs). I don't see how this could still leak. Am I missing something?


> On Sept. 29, 2011, 4:03 p.m., wdoekes wrote:
> > /branches/1.8/channels/chan_sip.c, lines 4745-4752
> > <https://reviewboard.asterisk.org/r/1465/diff/2/?file=20922#file20922line4745>
> >
> >     peerlist and var should be freed. 
> >     peerlist and varregs get freed => segfault on the latter (and a leak of 'var'.. but the segfault should prevent that ;) )
> >     
> >     (see my quick and dirty ast_variables_clone, it was there for a reason)

Ugh. I just noticed that even varregs is sometimes returned via the get_insecure_variable_from_config() function. So it isn't always safe to free, either. What a mess. I'll stare at things for a little longer and see if I can come up with something not ugly and definitely not behavior changing.

I like the way r1395 looks and it really may be the way to go, but the part where you detail the differences (checking ipaddr when we didn't used to and not checking sipregs for insecure) just worries me a little bit. Something being dumb doesn't preclude someone from doing it or relying on it.

With trunk, there is a res_config_sqlite3 and a built-in sqlite3 database for astdb. I plan on converting astdb over to just using the realtime API so that the astdb database can be used for realtime stuff as well. This would allow all kinds of fun things, but would also allow us to start writing tests that cover realtime as well. Tests make refactoring much less scary.


- Terry


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/1465/#review4456
-----------------------------------------------------------


On Sept. 29, 2011, 12:53 p.m., Terry Wilson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1465/
> -----------------------------------------------------------
> 
> (Updated Sept. 29, 2011, 12:53 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> It was possible that if realtime sipregs are used, that the varregs variable returned by ast_load_realtime() would not be freed. This patch aims to correct that.
> 
> 
> This addresses bug ASTERISK-17792.
>     https://issues.asterisk.org/jira/browse/ASTERISK-17792
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/channels/chan_sip.c 338223 
> 
> Diff: https://reviewboard.asterisk.org/r/1465/diff
> 
> 
> Testing
> -------
> 
> It compiles. We return NULL in every case that ast_variables_destroy(varregs) is called, so there should be no chance of causing a problem with destroying the variable multiple times, etc.
> 
> 
> Thanks,
> 
> Terry
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20110929/2aab7a36/attachment.htm>