[asterisk-dev] [Code Review] 3209: Crash in ast_format_cmp on shutdown
Matt Jordan
reviewboard at asterisk.org
Thu Mar 6 15:16:46 CST 2014
> 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>
More information about the asterisk-dev
mailing list