[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 11:16 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?
====================================================================== 

---------------------------------------------------------------------- 
 (0122789) tim_ringenbach (reporter) - 2010-06-02 11:16
 https://issues.asterisk.org/view.php?id=17420#c122789 
---------------------------------------------------------------------- 
Oh you're right. For some reason I thought wait_for_answer was called with
some locks held.

As for as ast_assert_locked, I'm pretty sure it's impossible to implement
without storing the pthread_t of the thread holding the lock. So it's
something that could be implemented when DEBUG_THREADS is on and expand to
nothing otherwise.

The rational section of 
http://www.opengroup.org/onlinepubs/009695399/functions/pthread_mutex_lock.html
says "Mutex objects are intended to serve as a low-level primitive from
which other thread synchronization functions can be built." and "Likewise,
while being able to extract the thread ID of the owner of a mutex might be
desirable, it would require storing the current thread ID when each mutex
is locked, and this could incur unacceptable levels of overhead. Similar
arguments apply to a mutex_tryunlock operation.".

If you only checked the use count, the pthread_mutex does have that
internally for recursive locks, but accessing it is both nonportable and
nasty.

Since asterisk uses recursive locks, you could just try locking the lock
again. But that would usually work even when lock wasn't locked. 

Issue History 
Date Modified    Username       Field                    Change               
====================================================================== 
2010-06-02 11:16 tim_ringenbach Note Added: 0122789                          
======================================================================