> 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 > >
-- _____________________________________________________________________ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
