[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 13:19 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.
====================================================================== 

---------------------------------------------------------------------- 
 (0118042) tim_ringenbach (reporter) - 2010-02-12 13:19
 https://issues.asterisk.org/view.php?id=16678#c118042 
---------------------------------------------------------------------- 
It looks like I already had added a chanspy_ds_free(&chanspy_ds); in the
same place you have it, and it was still crashing with that.

I'll try testing your patch and post something to the review board.

I suspect that I will still be able to make it crash though.

If you look at the code in channel.c:
       while ((datastore = AST_LIST_REMOVE_HEAD(&chan->datastores,
entry)))
                /* Free the data store */
                ast_channel_datastore_free(datastore);

After it removes the datastore from the linked list, but before it calls
ast_channel_datastore_free(), chanspy might decide it's done, and call
chanspy_ds_free(). chanspy_ds_free() won't find anything to remove, but it
is ok with that. I assumed since it didn't find anything to remove, it's
safe to exit and let the stack memory go away. channel.c can't know to tell
it otherwise because it's not on the list anymore. But the other thread is
going about to call the destroy method, which is going to access invalid
stack memory and crash. 

Issue History 
Date Modified    Username       Field                    Change               
====================================================================== 
2010-02-12 13:19 tim_ringenbach Note Added: 0118042                          
======================================================================