756036 - Race condition in Ril.cpp

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Cervantes Yu [:cyu] [:cervantes]]

I encountered a crash gdb. Here is the debugging excerpt:

======================= webapi+apps.js =======================
[New Thread 4743.4750]

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 4743.4750]
0x40c76162 in mozilla::ipc::RilClient::OnFileCanWriteWithoutBlocking (this=0x2da0d0, fd=21) at /home/cervantes/git/mozilla-b2g/B2G/gecko/ipc/ril/Ril.cpp:333
333             while (mCurrentWriteOffset < mCurrentRilRawData->mSize) {
(gdb) bt
#0  0x40c76162 in mozilla::ipc::RilClient::OnFileCanWriteWithoutBlocking (this=0x2da0d0, fd=21) at /home/cervantes/git/mozilla-b2g/B2G/gecko/ipc/ril/Ril.cpp:333
#1  0x40c75f36 in mozilla::ipc::RilWriteTask::Run (this=<value optimized out>) at /home/cervantes/git/mozilla-b2g/B2G/gecko/ipc/ril/Ril.cpp:177
#2  0x40cbf706 in MessageLoop::RunTask (this=0x100ffdf4, task=0x0) at /home/cervantes/git/mozilla-b2g/B2G/gecko/ipc/chromium/src/base/message_loop.cc:318
#3  0x40cc0700 in MessageLoop::DeferOrRunPendingTask (this=0x0, pending_task=<value optimized out>) at /home/cervantes/git/mozilla-b2g/B2G/gecko/ipc/chromium/src/base/message_loop.cc:326
#4  0x40cc138e in MessageLoop::DoWork (this=0x100ffdf4) at /home/cervantes/git/mozilla-b2g/B2G/gecko/ipc/chromium/src/base/message_loop.cc:426
#5  0x40cd3048 in base::MessagePumpLibevent::Run (this=0xf130, delegate=0x100ffdf4) at /home/cervantes/git/mozilla-b2g/B2G/gecko/ipc/chromium/src/base/message_pump_libevent.cc:310
#6  0x40cbf6a2 in MessageLoop::RunInternal (this=0x0) at /home/cervantes/git/mozilla-b2g/B2G/gecko/ipc/chromium/src/base/message_loop.cc:208
#7  0x40cbf782 in MessageLoop::RunHandler (this=0x100ffdf4) at /home/cervantes/git/mozilla-b2g/B2G/gecko/ipc/chromium/src/base/message_loop.cc:201
#8  MessageLoop::Run (this=0x100ffdf4) at /home/cervantes/git/mozilla-b2g/B2G/gecko/ipc/chromium/src/base/message_loop.cc:175
#9  0x40cc86ce in base::Thread::ThreadMain (this=0xf0f8) at /home/cervantes/git/mozilla-b2g/B2G/gecko/ipc/chromium/src/base/thread.cc:156
#10 0x40cd3542 in ThreadFunc (closure=0x0) at /home/cervantes/git/mozilla-b2g/B2G/gecko/ipc/chromium/src/base/platform_thread_posix.cc:27
#11 0x40087c28 in __thread_entry (func=0x40cd3539 <ThreadFunc>, arg=0xf0f8, tls=<value optimized out>) at bionic/libc/bionic/pthread.c:217
#12 0x4008777c in pthread_create (thread_out=<value optimized out>, attr=0xbe836910, start_routine=0x40cd3539 <ThreadFunc>, arg=0xf0f8) at bionic/libc/bionic/pthread.c:357
#13 0x00000000 in ?? ()
(gdb) p mCurrentRilRawData
$6 = {mRawPtr = 0x0}

Since mCurrentRilRawData is assigned from popping mOutgoingQ. This could be that the queue contains a NULL pointer.

The mOutgoingQ is popped on IO thread, but is pushed from ril worker.
In SenRilRawData() mOutgoingQ mutex is locked before pushing, but in RilClient::OnFileCanWriteWithoutBlocking() it doesn't lock the mutex when reading and popping the queue. This is equivalent to locking at all. Adding MutexAutoLock lock(mMutex); before reading and popping the queue should fix the crash.

Comment 1

[Cervantes Yu [:cyu] [:cervantes]]

Attachment: Fix the race condition between rilworker and IO thread

Comment 2

[Cervantes Yu [:cyu] [:cervantes]]

Attachment: Test code to verify the race condition is fixed

Comment 3

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Looks good. Do you need me to land it, or do you have rights to?

Comment 4

[Cervantes Yu [:cyu] [:cervantes]]

Please land it. Thanks.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d917d7b92fc5

Comment 6

[Ryan VanderMeulen [:RyanVM]]

I assumed that the test code was not intended to land. Let me know if I was wrong.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d917d7b92fc5