[asterisk-bugs] [Asterisk 0016678]: [patch] segfault on chanspy due to race in main/channel.c

[Asterisk Bug Tracker]

A NOTE has been added to this issue. 
====================================================================== 
https://issues.asterisk.org/view.php?id=16678 
====================================================================== 
Reported By:                tim_ringenbach
Assigned To:                dvossel
====================================================================== 
Project:                    Asterisk
Issue ID:                   16678
Category:                   Applications/app_chanspy
Reproducibility:            random
Severity:                   minor
Priority:                   normal
Status:                     feedback
Target Version:             1.4.31
Asterisk Version:           1.4.29 
JIRA:                       SWP-783 
Regression:                 No 
Reviewboard Link:            
SVN Branch (only for SVN checkouts, not tarball releases): N/A 
SVN Revision (number only!):  
Request Review:              
====================================================================== 
Date Submitted:             2010-01-22 17:52 CST
Last Modified:              2010-02-12 16:06 CST
====================================================================== 
Summary:                    [patch] segfault on chanspy due to race in
main/channel.c
Description: 
When channel.c destroys the datastore on the channel, it doesn't hold the
channel lock while calling the destroy callback. It really ought to,
because otherwise it's accessing the datastore list without locking. I've
gotten a segfault trying to lock the mutex in the ds destroy function in
app_chanspy because of this race.

Holding the channel lock during the destroy should be safe because it is
also held during the fixup callback, and app_chanspy has already been
patched to avoid the possible deadlock from that locking order issue.
====================================================================== 

---------------------------------------------------------------------- 
 (0118048) tim_ringenbach (reporter) - 2010-02-12 16:06
 https://issues.asterisk.org/view.php?id=16678#c118048 
---------------------------------------------------------------------- 
I applied your new patch to a stock 1.4.29, plus I added a "sleep(30)"
where I think the race is happening to try to bring it out. Now I am
running the same stress test I was using when I originally got the crash,
and I'll post the backtrace if I can get it to crash.

I don't see what good checking the return value of
ast_channel_datastore_remove() does. You know it didn't remove the channel
datastore and really should have, but what are you going to do about it?
You have to not free the memory (by returning) until the other thread
finishes calling your callback, and you have no way of knowing when that
will be. That's why I think you need to protect it from happening by making
channel.c whole the channel lock while doing that. 

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