[asterisk-dev] [Code Review] Deadlocks dealing with dialplan hints during reload.

[David Vossel]

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


This is the last comment I have for now.


/branches/1.8/main/pbx.c
<https://reviewboard.asterisk.org/r/1313/#comment7754>

    The statecbs container is protected by the statecbs lock.  When this lock is held in ast_extension_state_add() the data pointer for a element already existing within the container can change.  I notice we do not protect that data pointer during the callback here.  Could that be an issue?
    
    


- David


On July 14, 2011, 8:20 p.m., rmudgett wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1313/
> -----------------------------------------------------------
> 
> (Updated July 14, 2011, 8:20 p.m.)
> 
> 
> Review request for Asterisk Developers, Russell Bryant, David Vossel, and irroot.
> 
> 
> Summary
> -------
> 
> There are two remaining different deadlocks reported dealing with dialplan
> hints.
> 
> The deadlock in ASTERISK-17666 is caused by invalid locking order in
> ast_remove_hint().  The hints container must be locked before the hint
> object.
> 
> The deadlock in ASTERISK-17760 is caused by a catch-22 situation in
> handle_statechange().  The deadlock is caused by not having the conlock
> before calling the watcher callbacks.  Unfortunately, having that lock
> causes a different deadlock as reported in ASTERISK-16961.
> 
> * Fixed ast_remove_hint() locking order.
> 
> * Made handle_statechange() no longer call the watcher callbacks holding
> any locks that matter.
> 
> * Made hint ao2 destructor do the watcher callbacks for extension
> deactivation to guarantee that they get called.
> 
> * Fixed hint reference leak in ast_add_hint() if the callback container
> constructor failed.
> 
> * Fixed hint reference leak in complete_core_show_hint() for every hint it
> found for CLI tab completion.
> 
> * Adjusted locking in ast_merge_contexts_and_delete() for safety.
> 
> * Added context_merge_lock to prevent ast_merge_contexts_and_delete() and
> handle_statechange() from interfering with each other.
> 
> * Fixed ast_change_hint() not taking into account that the extension is
> used for the hash key.
> 
> 
> This addresses bugs ASTERISK-17666 and ASTERISK-17760.
>     https://issues.asterisk.org/jira/browse/ASTERISK-17666
>     https://issues.asterisk.org/jira/browse/ASTERISK-17760
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/main/pbx.c 328355 
> 
> Diff: https://reviewboard.asterisk.org/r/1313/diff
> 
> 
> Testing
> -------
> 
> * Did some calls that changed the state of some hints.
> * Reloaded the dialplan.
> 
> 
> Thanks,
> 
> rmudgett
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20110721/20c5c765/attachment-0001.htm>