[asterisk-dev] [Code Review] 3209: Crash in ast_format_cmp on shutdown

[Matt Jordan]

> On Feb. 12, 2014, 5:12 p.m., Mark Michelson wrote:
> > I'm not a fan of this patch for two reasons:
> > 1) We're preventing a crash by introducing a memory leak. 
> > 2) The condition of undestroyed channels doesn't seem like enough to guarantee that the interfaces container is not currently being referenced
> > 
> > A more graceful fix is to replace the interfaces ao2_container in format.c with an ao2 global object. This way, each time a piece of code attempts to get the container, they will need to get a reference using ao2_global_obj_ref()  (and check the function for a NULL return). This protects access to the container and guarantees that the container will not be freed as long as someone may be trying to use it.
> 
> Corey Farrell wrote:
>     I don't agree with #1, since this patch really only applies to SHUTDOWN_FAST (signal INT, TERM or HUP). It might rarely apply to SHUTDOWN_NORMAL (core stop now).  Both of these modes skip shutdown of all modules, and SHUTDOWN_FAST doesn't even hangup active channels.  Anyone testing for memory leaks would use 'core stop gracefully'.
>     
>     You're probaby right about #2.  This patch makes it far less likely for the issue to happen, but in theory still possible.  Maybe it would be better to backport ast_register_cleanup, use that to register format_attr_shutdown?
>     
>     I had considered using an ao2_global_obj_ref, but that would add overhead to the already expensive interfaces container lookups.  Since the lookup is done multiple times per frame, I don't think more overhead is worth preventing a memory leak during SHUTDOWN_FAST.

Ugh.

Life was simpler when we didn't care and just leaked memory all the time on exit...

Josh's work in 12 may end up going with a linked list instead of a hash table for formats as well. So this might be a very temporary change in 11 anyway.

SO... while a global container is technically correct, I could live with the approach here, personally.

Mark: feel free to veto me :-)


- Matt


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


On Feb. 11, 2014, 1:37 p.m., Scott Griepentrog wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3209/
> -----------------------------------------------------------
> 
> (Updated Feb. 11, 2014, 1:37 p.m.)
> 
> 
> Review request for Asterisk Developers, Corey Farrell and n8ideas.
> 
> 
> Bugs: ASTERISK-23103
>     https://issues.asterisk.org/jira/browse/ASTERISK-23103
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This fixes a possible crash on shutdown by preventing ao2_unref of interfaces while channels are active.
> 
> Patch by cfarrell
> 
> 
> Diffs
> -----
> 
>   /branches/11/main/format.c 407957 
> 
> Diff: https://reviewboard.asterisk.org/r/3209/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Scott Griepentrog
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140306/b51b7cd0/attachment-0001.html>