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

[Terry Wilson]

> On Feb. 22, 2012, 1:11 p.m., wdoekes wrote:
> > /trunk/res/res_calendar.c, lines 1811-1814
> > <https://reviewboard.asterisk.org/r/1752/diff/2/?file=24509#file24509line1811>
> >
> >     If it's now not possible to unload res_calendar before unloading those that depend on it, shouldn't this be replaced with an assert(AST_LIST_EMPTY(&techs)) ?
> 
> Terry Wilson wrote:
>     Bah. Indeed. The whole point originally was to be able to unload res_calendar and the dependent modules with "module unload res_calendar.so". Oh, well. At least this makes it obvious why it isn't unloading in that case and still allows it to shut down properly.
> 
> Terry Wilson wrote:
>     Actually, you can still successfully unload all of the modules with "module unload -h res_calendar.so" so I'll just leave this in.
> 
> wdoekes wrote:
>     successfully isn't the first word that springs to mind.
>     
>     *CLI> module unload -h res_calendar
>     [Feb 22 22:18:30] WARNING[15761]: loader.c:535 ast_unload_resource: Warning:  Forcing removal of module 'res_calendar' with use count 2
>     WARNING: Freeing unused memory at 0x7f9ea4548b88, in __ast_module_user_remove of loader.c, line 231
>     WARNING: Freeing unused memory at 0x7f9ea4548668, in __ast_module_user_remove of loader.c, line 231
>     Unloaded res_calendar
>
> 
> Terry Wilson wrote:
>     Looks like ast_unload_resource calls ast_module_user_hangup_all() (right before calling ->unload()) which frees the module_user as well. Having a function which goes in and frees a pointer that you have without giving you any way of knowing that has happened is kind of irritating. For now, I'll just take out the loop. In the future, it might be nice for there to be a pre-unload callback or something. For now, I give up.

Actually, I don't give up. Even if we do take out the code, a "module unload -h res_calendar.so" will still unload res_calendar without unloading the other modules. Then if those modules are unloaded, the same error will show up. Basically, using module unload -h is unsafe in any case, but I think it is less unsafe having the modules unloaded than it is having them stick around without res_calendar existing--which will almost certainly eventually cause a crash.


- Terry


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


On Feb. 22, 2012, 12:12 p.m., Terry Wilson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1752/
> -----------------------------------------------------------
> 
> (Updated Feb. 22, 2012, 12:12 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/include/asterisk/calendar.h 356213 
>   /trunk/main/loader.c 356213 
>   /trunk/res/res_calendar.c 356213 
> 
> 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/20120222/4e04c33b/attachment.htm>