[asterisk-dev] [Code Review]: Make ast_unload_resource actually remove the module from the module list when it is unloaded

[Terry Wilson]

> On Feb. 20, 2012, 2:33 p.m., Mark Michelson wrote:
> > I think a better idea is to call both ast_unload_resource() and ast_module_unregister() in res_calendar.c.
> 
> Terry Wilson wrote:
>     I'd have to pass around pointers to the ast_module_info instead of just using AST_MODULE. The weird thing is that 1.8 doesn't seem to have the problem at all. Something has changed between there and trunk that is screwing things up.
> 
> Tilghman Lesher wrote:
>     The module shutdown routine has a loop that permits dependency ordering, if the usecount is set greater than zero.  It may be helpful to increment the usecount of res_calendar for each dependent module registered.  That would allow you to delay the unload of res_calendar until after all dependent modules are unloaded.  There's no good way of doing this currently, however, as usecount is defined within loader.c, and it is not accessible without providing a channel for sending a softhangup to at the appropriate moment.
> 
> wdoekes wrote:
>     > But who does the free'ing of mod then?
>     
>     Ok, I was a little quick there. I didn't see that there are two places where destruction of mod is legit.
>     
>     > Something has changed between there and trunk that is screwing things up.
>     
>     That it crashes in 10 has everything to do with the load order -- the new sorting -- but I can't figure out why:
>     
>     ----
>     
>     [modules]
>     load => func_strings.so
>     load => res_calendar.so
>     load => res_calendar_caldav.so
>     load => res_calendar_ews.so
>     
>     *CLI> module show
>     Module                         Description                              Use Count 
>     res_calendar.so                Asterisk Calendar integration            0         
>     func_strings.so                String handling dialplan functions       0         
>     res_calendar_caldav.so         Asterisk CalDAV Calendar Integration     0         
>     res_calendar_ews.so            Asterisk MS Exchange Web Service Calenda 0         
>     4 modules loaded
>     
>     >>> attempting unload of res_calendar.so (0/0)
>     >>> attempting unload of func_strings.so (0/0)
>     (no crash)
>     
>     ----
>     
>     [modules]
>     load => res_calendar.so
>     load => res_calendar_caldav.so
>     load => res_calendar_ews.so
>     load => func_strings.so
>     
>     *CLI> module show
>     Module                         Description                              Use Count 
>     res_calendar.so                Asterisk Calendar integration            0         
>     res_calendar_caldav.so         Asterisk CalDAV Calendar Integration     0         
>     res_calendar_ews.so            Asterisk MS Exchange Web Service Calenda 0         
>     func_strings.so                String handling dialplan functions       0         
>     4 modules loaded
>     
>     >>> attempting unload of res_calendar.so (0/0)
>     >>> attempting unload of res_calendar_caldav.so (0/0)
>     loader.c line 503 (ast_module_shutdown): Error: attempt to destroy invalid mutex '&(&mod->users)->lock'.
>     WARNING: Freeing unused memory at 0x7f37506f1738, in ast_module_shutdown of loader.c, line 504
>     (CRASH)
>     
>     ----
>     
>     If I use your patch, I get this:
>     
>     >>> attempting unload of res_calendar.so (0/0)
>     >>> attempting unload of res_calendar_caldav.so (0/0)
>     (no crash)
>     
>     That was a listing of mod->resource. If it worked like it's supposed to
>     I'd expect to see "func_strings.so", not "res_calendar_caldav.so".

Yeah, in 10+ it crashes because we sort the module list. It only seems to crash when a calendar tech module immediately follows res_calendar.so. If you put one module in between them, no crash. I'm assuming it has to do with an interaction with the list macros and ->next disappearing. I'll keep looking.


- Terry


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


On Feb. 18, 2012, 1:31 p.m., Terry Wilson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1752/
> -----------------------------------------------------------
> 
> (Updated Feb. 18, 2012, 1:31 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> res_calendar calls ast_unload_resource for the related tech modules when it is unloaded. If this happens through a 'core stop gracefully', then it will be unloading the tech modules that are already in the list that is being traversed (supposedly safely) for unloading, eventually causing a double free. The problem seems to be that ast_unload_resource, while it calls the unload() callback function for the module, does not actually unlink the module from the list of modules. So the AST_LIST_TRAVERSE_SAFE_BEGIN {} still iterates over the unloaded module.
> 
> This patch causes ast_unload_resource to call AST_LIST_REMOVE on successfully unloaded modules.
> 
> 
> Diffs
> -----
> 
>   /trunk/main/loader.c 355783 
> 
> Diff: https://reviewboard.asterisk.org/r/1752/diff
> 
> 
> Testing
> -------
> 
> Scenario: Start Asterisk with res_calendar and assorted calendar tech modules loaded. Run 'core stop gracefully'.
> Before patch: Crash.
> After patch: No crash.
> 
> 
> Thanks,
> 
> Terry
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20120221/509d0506/attachment.htm>