[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 don't know what you mean by delete. The complicated thing about
 > the PBX core is that it's a cumulative collection of things from
 > different sources, and in the case of pbx_config any changes there
 > should be merged with everything else - but that shouldn't impact
 > other contexts/extensions outside of its realm.
 > 
 > I agree though that going down the road of fixing that is
 > complicated, but we need to at least document this.

ast_merge_contexts_and_delete calls context_merge on all existing contexts.  The comment for context_merge:

/* the purpose of this routine is to duplicate a context, with all its substructure,
   except for any extens that have a matching registrar */

So step 1 when called by pbx_config, make a duplicate copy of all contexts not registered by pbx_config.  Then it merges the new pbx_config extensions with the copy of everything else.  Then it replaces the existing contexts_table with a new one, finally it deletes all contexts from the old table.

Using AO2 for the ast_context and ast_exten could solve this, but it could also introduce new problems.  This way we'd copy a reference of everyone elses contexts/extens to a new list, merge in the load from pbx_config, replace the global list, then clear the old list, unreferencing everything.  But even that could be a mess if one context had extensions from multiple registrars.

As it is with the current code I think ast_context_delete with a non-NULL context must be done within a wrlock on the context table, and only with a context pointer retrieved within the same lock.  So the use in func_periodic_hook is unsafe.  There are other situations where ast_context_find / ast_context_delete are used, this will have to be tweaked to run within a single lock instead of using the locking versions of both functions.  In fact ast_context_find is unsafe for this reason.  The return value can be freed by another thread as soon as ast_context_find runs ast_unlock_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