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

[wdoekes]

> 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.

> 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".


- wdoekes


-----------------------------------------------------------
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/6571b887/attachment-0001.htm>