Summary:	ASTERISK-12345: [patch] Janitor project to add channel locks
Reporter:	Patrick Putman (pputman)	Labels:
Date Opened:	2008-07-09 06:14:30	Date Closed:	2011-06-07 14:07:27
Priority:	Minor	Regression?	No
Status:	Closed/Complete	Components:	Channels/chan_sip/General
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) chan_sip_chanlockjanitor.patch ( 1) chan_sip_chanlockjanitor2.patch
Description:	Uses ast_channel_lock and ast_channel_unlock to make chan_sip.c more threadsafe.
Comments:	By: Mark Michelson (mmichelson) 2008-07-09 11:29:59 Hi Patrick, thanks for the patch. There are four places where you have added channel locking. The first three are unnecessary since the channel will be locked prior to reaching that point. 1. Lock added to try_suggested_sip_codec. The channel will be locked either in sipsock_read or in ast_answer prior to reaching this point. 2 and 3. Locks added in get_refer_info and get_also_info. In both cases, the channel is already locked in sipsock_read. The final set of locks added in local_attended_transfer is necessary, however there is a typo in the patch. Instead of unlocking the channels you lock them again. By: Patrick Putman (pputman) 2008-07-09 15:36:19 Thanks for the feeback, I uploaded a second patch that only adds it to the last part, and corrected the typos. By: Tilghman Lesher (tilghman) 2008-08-06 12:56:15 Uh, well, here's two more problems with the patch: 1) the variable declaration is now no longer the first thing in a block, which violates C99. 2) Locking two channels like this is a very dangerous operation.  You could very well have introduced a deadlock.  When locking two channels (which have no defined lock order), deadlock avoidance must be used. By: Patrick Putman (pputman) 2008-08-06 14:48:40 It looks like this code has already been modified and this patch isn't necessary in the newest svn, so I guess this can be closed.