[Asterisk-code-review] Fix potential crash after unload of func periodic hook or te... (asterisk[13])

[Corey Farrell]

Corey Farrell has posted comments on this change.

Change subject: Fix potential crash after unload of func_periodic_hook or test_message.
......................................................................


Patch Set 2:

> I dread saying this because it's pbx.c but I think the real bug
 > here is that the context is replaced. There's a reasonable
 > expectation in the PBX core that the context returned will remain
 > valid. Does this impact the features.c code as well or does
 > ordering make it so it doesn't?

Isn't the delete done so that all the contexts can be merged into a new container, then replace the old one - making dialplan changes from pbx_config reload atomic?

features.c has no calls to ast_context_destroy, I think they were moved to res_parking?  All calls in res_parking use the NULL parameter so they are fine.  All other uses of non-null context to ast_context_destroy are using contexts that were retrieved the line before.  This is still vulnerable to a race, but only for the time between the two (not the entire time the module is open).

The problem here is that when func_periodic_hook fails to unregister it's context, it leaves behind 'struct ast_exten' with 'registrar' pointing to AST_MODULE - a string that was part of memory free'd by dlclose on func_periodic_hook.

This is probably a bug in pbx.c, but that's a bigger ordeal.  For the immediate this patch does fix a potential crash while simplifying the cleanup for these two modules.  In my opinion any module that might add contexts should run ast_context_destroy(NULL, AST_MODULE) in unload_module, instead of deleting individual contexts.

-- 
To view, visit https://gerrit.asterisk.org/467
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6a00ec8e38046058f97dc703e1adcde9bf517835
Gerrit-PatchSet: 2
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-HasComments: No