> On Feb. 12, 2014, 6: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.
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. - Corey ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3209/#review10863 ----------------------------------------------------------- On Feb. 11, 2014, 2: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, 2: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
