[asterisk-bugs] [Asterisk 0017420]: app_queue's compare_weight can be called in contextes that don't lock the global queues lock

[Asterisk Bug Tracker]

A NOTE has been added to this issue. 
====================================================================== 
https://issues.asterisk.org/view.php?id=17420 
====================================================================== 
Reported By:                tim_ringenbach
Assigned To:                
====================================================================== 
Project:                    Asterisk
Issue ID:                   17420
Category:                   Applications/app_queue
Reproducibility:            unable to reproduce
Severity:                   minor
Priority:                   normal
Status:                     acknowledged
Asterisk Version:           1.4.31 
JIRA:                       SWP-1614 
Regression:                 No 
Reviewboard Link:            
SVN Branch (only for SVN checkouts, not tarball releases): N/A 
SVN Revision (number only!):  
Request Review:              
====================================================================== 
Date Submitted:             2010-05-28 15:37 CDT
Last Modified:              2010-06-02 10:37 CDT
====================================================================== 
Summary:                    app_queue's compare_weight can be called in
contextes that don't lock the global queues lock
Description: 
I haven't experienced any crashes from this issue but I thought I would
report it anyway.

I noticed in the code that ring_one() is called from both try_calling()
and wait_for_answer(). The former goes through a lot of trouble to lock the
queues lock if we're using weights. The queues lock is then unlocked right
before weight_for_answer() is called. But wait_for_answer() can also call
ring_one(), which calls ring_entry(), which calls compare_weight(). And
compare_weight() walks the queues list (in 1.4) or iterates the queue ao2
(in trunk).

As a side note: ever thought about creating an ast_assert_locked() macro
that could be used in places that compare_weight that assume locks, to
output a warning if that assumption is ever violated?
====================================================================== 

---------------------------------------------------------------------- 
 (0122779) mmichelson (administrator) - 2010-06-02 10:37
 https://issues.asterisk.org/view.php?id=17420#c122779 
---------------------------------------------------------------------- 
Not much to say here other than that you're definitely right that the
behavior is inconsistent and should be fixed. I'm fairly certain that the
calls to ring_one in wait_for_answer are done with no locks held, so it
would likely be easy enough to just grab the global queue container lock
prior to calling ring_one if weights are being used. No fancy deadlock
avoidance would be necessary.

Regarding ast_assert_locked, I certainly like the idea and it would be
helpful. The problem is that I have no idea how to implement such a thing.
A naive approach might be to try unlocking and see if that succeeds or
fails. The problem is that if it succeeds, we've just unlocked the lock.
While we would obviously re-lock before returning from the macro, the
operation has destroyed the atomicity of the operations we were trying to
accomplish. Such a macro could safely be made if DEBUG_THREADS is enabled,
since we use our own mutex structure that could be inspected to determine
its use count. However, this is not on by default and we'd need a way to
implement it with standard pthread_mutex_t types. Any ideas? 

Issue History 
Date Modified    Username       Field                    Change               
====================================================================== 
2010-06-02 10:37 mmichelson     Note Added: 0122779                          
======================================================================