[Home]

Summary:ASTERISK-16141: [patch] Race conditition in app_meetme leads to crash
Reporter:Vincent Sweeney (vince)Labels:
Date Opened:2010-05-24 17:42:26Date Closed:2010-07-13 14:01:29
Priority:CriticalRegression?No
Status:Closed/CompleteComponents:Applications/app_meetme
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) bt-full.20100524.txt
( 1) Issue17390_meetme-fix_1.6.patch
( 2) Issue17390_meetme-fix_new_1.6.patch
( 3) patch.meetme-fix
Description:I have been tracking down random crashes on a number of Asterisk servers that rely heavily on app_meetme conference rooms. Depending on the server load they can crash upto several times a week.

The majority of core dumps seem to be related to insufficient locking on the conference 'userlist' linked list structure.

I have attached to patch to fix the locations where I think thread locks are missing. Applies against 1.4.31. I have quickly glanced at the 1.6.x branch and this looks to be required there too but I have no servers to test that version on.

****** ADDITIONAL INFORMATION ******

Debug Extract
----
Program terminated with signal 11, Segmentation fault.
#0  0x00002aaab9f2e7ad in conf_run (chan=0x18605350, conf=0x2aaacc0395a0, confflags=33620256, optargs=0x4386cb50) at app_meetme.c:1637
1637                    user->user_no = AST_LIST_LAST(&conf->userlist)->user_no + 1;

(gdb) list
   if (AST_LIST_EMPTY(&conf->userlist))
       user->user_no = 1;
   else
->      user->user_no = AST_LIST_LAST(&conf->userlist)->user_no + 1;

(gdb) print conf->userlist
$3 = {first = 0x0, last = 0x0}

"AST_LIST_LAST(&conf->userlist)" is NULL so Asterisk crashes when referenced. In between the "if (AST_LIST_EMPTY(&conf->userlist))" check and accessing the list, the data has been modified. Classic thread race.
Comments:By: Lott Caskey (lottc) 2010-05-25 11:55:19

I am also experiencing similar issues with 1.6.2.7.

Can you please post a full backtrace.
By: Leif Madsen (lmadsen) 2010-05-25 13:37:19

Agreed. I think a backtrace would be useful here. Thanks for the patch!
By: Lott Caskey (lottc) 2010-05-25 14:30:26

I have added a patch for the 1.6 branch, which I will be testing.
By: Vincent Sweeney (vince) 2010-05-25 20:00:08

Backtrace added.
By: Lott Caskey (lottc) 2010-05-25 23:13:48

The patch has been built into our 1.6.2.8-rc1 system and I will report back in 5 days.  If it can survive that long, I will consider it a success.
By: Lott Caskey (lottc) 2010-06-01 10:17:36

I haven't had another crash since patching this into 1.6.2.8-rc1 about a week ago.
By: Lott Caskey (lottc) 2010-06-02 16:31:59

Uploaded Issue17390_meetme-fix_new_1.6.patch which was missing Vince's removal of the 'const' declaration of the conf struct in the conf_queue_dtmf function.
By: Jeff Peeler (jpeeler) 2010-06-22 15:24:13

Vince - could you also upload a "thread apply all bt"?
By: Vincent Sweeney (vince) 2010-06-23 07:34:38

Unfortunately not as I no longer have the core files and my patch has fixed all the crashing I had been experiencing so I have no recent ones either.
By: Digium Subversion (svnbot) 2010-07-12 15:34:51

Repository: asterisk
Revision: 275773

U   branches/1.4/apps/app_meetme.c

------------------------------------------------------------------------
r275773 | jpeeler | 2010-07-12 15:34:50 -0500 (Mon, 12 Jul 2010) | 12 lines

Make user removals and traversals thread safe in meetme.

Race conditions present in meetme involving the user list where a lack of
locking has the potential for a user to be removed during a traversal or as in
the case of the reporter after checking if the list is empty could cause a
crash. Fixing this was done by convering the userlist to an ao2 container.

(closes issue ASTERISK-16141)
Reported by: Vince

Review: https://reviewboard.asterisk.org/r/746/

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=275773
By: Digium Subversion (svnbot) 2010-07-13 12:37:40

Repository: asterisk
Revision: 276074

_U  trunk/
U   trunk/apps/app_meetme.c

------------------------------------------------------------------------
r276074 | jpeeler | 2010-07-13 12:37:39 -0500 (Tue, 13 Jul 2010) | 19 lines

Merged revisions 275773 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.4

........
 r275773 | jpeeler | 2010-07-12 15:34:51 -0500 (Mon, 12 Jul 2010) | 12 lines
 
 Make user removals and traversals thread safe in meetme.
 
 Race conditions present in meetme involving the user list where a lack of
 locking has the potential for a user to be removed during a traversal or as in
 the case of the reporter after checking if the list is empty could cause a
 crash. Fixing this was done by convering the userlist to an ao2 container.
 
 (closes issue ASTERISK-16141)
 Reported by: Vince
 
 Review: https://reviewboard.asterisk.org/r/746/
........

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=276074
By: Digium Subversion (svnbot) 2010-07-13 14:01:28

Repository: asterisk
Revision: 276121

_U  branches/1.6.2/
U   branches/1.6.2/apps/app_meetme.c

------------------------------------------------------------------------
r276121 | jpeeler | 2010-07-13 14:01:28 -0500 (Tue, 13 Jul 2010) | 26 lines

Merged revisions 276074 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

................
 r276074 | jpeeler | 2010-07-13 12:37:40 -0500 (Tue, 13 Jul 2010) | 19 lines
 
 Merged revisions 275773 via svnmerge from
 https://origsvn.digium.com/svn/asterisk/branches/1.4
 
 ........
   r275773 | jpeeler | 2010-07-12 15:34:51 -0500 (Mon, 12 Jul 2010) | 12 lines
   
   Make user removals and traversals thread safe in meetme.
   
   Race conditions present in meetme involving the user list where a lack of
   locking has the potential for a user to be removed during a traversal or as in
   the case of the reporter after checking if the list is empty could cause a
   crash. Fixing this was done by convering the userlist to an ao2 container.
   
   (closes issue ASTERISK-16141)
   Reported by: Vince
   
   Review: https://reviewboard.asterisk.org/r/746/
 ........
................

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=276121