data race on IncomingVideoStream::incoming_render_thread_

RESOLVED FIXED in Firefox 22

Status

()

P2
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jseward, Assigned: jesup)

Tracking

(Depends on: 1 bug)

unspecified
mozilla22
x86_64
Linux
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox21 affected, firefox22 fixed)

Details

(Whiteboard: [webrtc][blocking-webrtc+] [data-race] [helgrind][qa-])

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
media/webrtc/trunk/src/modules/video_render/main/source/incoming_video_stream.cc

IncomingVideoStream::IncomingVideoStreamProcess reads
incoming_render_thread_ without holding a lock at line 287.  By
contrast, IncomingVideoStream::Stop() reads and writes it only after
doing 'thread_critsect_.Enter()'.

How this can screw up is with the following interleaving:

T1 (in ::IncomingVideoStreamProcess()) vs T2 (::Stop())

T2:241   thread_critsect_.Enter();
T2:242  if (incoming_render_thread_) {

T1:287    if (incoming_render_thread_ == NULL) { /*NOT TAKEN*/
T1:290    }
T1 now believes we are not in a terminating state

T2:243    ThreadWrapper* thread = incoming_render_thread_;
T2:244    incoming_render_thread_ = NULL;
T2 sets it to 'terminating' state

If T1 now accesses incoming_render_thread_ again, we will segfault.

Line 292 is:
    thread_critsect_.Enter();

If this was pushed upwards to cover the test at line 287 I think
we'd be safe.
(Reporter)

Comment 1

6 years ago
The relevant complaint from Helgrind.  The 4th and 5th stacks are the main
race report.  Note how the writer holds 3 locks but the reader holds none.

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

Lock at 0x1F4367D8 was first observed
   at 0x4A0A0C9: pthread_mutex_init (hg_intercepts.c:429)
   by 0x8061802: webrtc::CriticalSectionPosix::CriticalSectionPosix() (critical_section_posix.cc:26)
   by 0x8061749: webrtc::CriticalSectionWrapper::CreateCriticalSection() (critical_section.cc:24)
   by 0x807F847: webrtc::IncomingVideoStream::IncomingVideoStream(int, unsigned int) (incoming_video_stream.cc:39)
   by 0x8082109: webrtc::ModuleVideoRenderImpl::AddIncomingRenderStream(unsigned int, unsigned int, float, float, float, float) (video_render_impl.cc:470
)
   by 0x80A82BC: webrtc::ViERenderer::Init(unsigned int, float, float, float, float) (vie_renderer.cc:120)
   by 0x80A841D: webrtc::ViERenderer::CreateViERenderer(int, int, webrtc::VideoRender&, webrtc::ViERenderManager&, unsigned int, float, float, float, flo
at) (vie_renderer.cc:31)
   by 0x80A899C: webrtc::ViERenderManager::AddRenderStream(int, void*, unsigned int, float, float, float, float) (vie_render_manager.cc:142)
   by 0x808F6F7: webrtc::ViERenderImpl::AddRenderer(int, webrtc::RawVideoType, webrtc::ExternalRenderer*) (vie_render_impl.cc:384)
   by 0x74CD429: mozilla::MediaEngineWebRTCVideoSource::Start(mozilla::SourceMediaStream*, int) (MediaEngineWebRTCVideo.cpp:269)
   by 0x7344A86: mozilla::MediaOperationRunnable::Run() (MediaManager.h:224)
   by 0x7C74686: nsThread::ProcessNextEvent(bool, bool*) (nsThread.cpp:627)

Lock at 0x1F436848 was first observed
   at 0x4A0A0C9: pthread_mutex_init (hg_intercepts.c:429)
   by 0x8061802: webrtc::CriticalSectionPosix::CriticalSectionPosix() (critical_section_posix.cc:26)
   by 0x8061749: webrtc::CriticalSectionWrapper::CreateCriticalSection() (critical_section.cc:24)
   by 0x807F850: webrtc::IncomingVideoStream::IncomingVideoStream(int, unsigned int) (incoming_video_stream.cc:40)
   by 0x8082109: webrtc::ModuleVideoRenderImpl::AddIncomingRenderStream(unsigned int, unsigned int, float, float, float, float) (video_render_impl.cc:470)
   by 0x80A82BC: webrtc::ViERenderer::Init(unsigned int, float, float, float, float) (vie_renderer.cc:120)
   by 0x80A841D: webrtc::ViERenderer::CreateViERenderer(int, int, webrtc::VideoRender&, webrtc::ViERenderManager&, unsigned int, float, float, float, float) (vie_renderer.cc:31)
   by 0x80A899C: webrtc::ViERenderManager::AddRenderStream(int, void*, unsigned int, float, float, float, float) (vie_render_manager.cc:142)
   by 0x808F6F7: webrtc::ViERenderImpl::AddRenderer(int, webrtc::RawVideoType, webrtc::ExternalRenderer*) (vie_render_impl.cc:384)
   by 0x74CD429: mozilla::MediaEngineWebRTCVideoSource::Start(mozilla::SourceMediaStream*, int) (MediaEngineWebRTCVideo.cpp:269)
   by 0x7344A86: mozilla::MediaOperationRunnable::Run() (MediaManager.h:224)
   by 0x7C74686: nsThread::ProcessNextEvent(bool, bool*) (nsThread.cpp:627)

Lock at 0x263391E8 was first observed
   at 0x4A0A0C9: pthread_mutex_init (hg_intercepts.c:429)
   by 0x8061802: webrtc::CriticalSectionPosix::CriticalSectionPosix() (critical_section_posix.cc:26)
   by 0x8061749: webrtc::CriticalSectionWrapper::CreateCriticalSection() (critical_section.cc:24)
   by 0x8082437: webrtc::ModuleVideoRenderImpl::ModuleVideoRenderImpl(int, webrtc::VideoRenderType, void*, bool) (video_render_impl.cc:97)
   by 0x8082516: webrtc::VideoRender::CreateVideoRender(int, void*, bool, webrtc::VideoRenderType) (video_render_impl.cc:80)
   by 0x80A8A37: webrtc::ViERenderManager::AddRenderStream(int, void*, unsigned int, float, float, float, float) (vie_render_manager.cc:128)
   by 0x808F6F7: webrtc::ViERenderImpl::AddRenderer(int, webrtc::RawVideoType, webrtc::ExternalRenderer*) (vie_render_impl.cc:384)
   by 0x74CD429: mozilla::MediaEngineWebRTCVideoSource::Start(mozilla::SourceMediaStream*, int) (MediaEngineWebRTCVideo.cpp:269)
   by 0x7344A86: mozilla::MediaOperationRunnable::Run() (MediaManager.h:224)
   by 0x7C74686: nsThread::ProcessNextEvent(bool, bool*) (nsThread.cpp:627)
   by 0x7C3B1D0: NS_ProcessNextEvent_P(nsIThread*, bool) (nsThreadUtils.cpp:237)
   by 0x7C751F9: nsThread::ThreadFunc(void*) (nsThread.cpp:265)

Possible data race during read of size 8 at 0x261A8BB8 by thread #67
Locks held: none
   at 0x80803FC: webrtc::IncomingVideoStream::IncomingVideoStreamProcess() (incoming_video_stream.cc:287)
   by 0x8080828: webrtc::IncomingVideoStream::IncomingVideoStreamThreadFun(void*) (incoming_video_stream.cc:282)
   by 0x8069EDE: webrtc::ThreadPosix::Run() (thread_posix.cc:361)
   by 0x8069FB6: StartThread (thread_posix.cc:72)
   by 0x4A09F6C: mythread_wrapper (hg_intercepts.c:219)
   by 0x30E2C07D13: start_thread (in /usr/lib64/libpthread-2.15.so)
   by 0x30E28F167C: clone (in /usr/lib64/libc-2.15.so)

This conflicts with a previous write of size 8 by thread #45
Locks held: 3, at addresses 0x1F4367D8 0x1F436848 0x263391E8
   at 0x80800E1: webrtc::IncomingVideoStream::Stop() (incoming_video_stream.cc:244)
   by 0x8081DED: webrtc::ModuleVideoRenderImpl::StopRender(unsigned int) (video_render_impl.cc:653)
   by 0x80A8206: webrtc::ViERenderer::StopRender() (vie_renderer.cc:50)
   by 0x808F013: webrtc::ViERenderImpl::StopRender(int) (vie_render_impl.cc:258)
   by 0x74CD6B4: mozilla::MediaEngineWebRTCVideoSource::Stop(mozilla::SourceMediaStream*, int) (MediaEngineWebRTCVideo.cpp:311)
   by 0x7344B16: mozilla::MediaOperationRunnable::Run() (MediaManager.h:247)
   by 0x7C74686: nsThread::ProcessNextEvent(bool, bool*) (nsThread.cpp:627)
   by 0x7C3B1D0: NS_ProcessNextEvent_P(nsIThread*, bool) (nsThreadUtils.cpp:237)

Address 0x261A8BB8 is 40 bytes inside a block of size 328 alloc'd
   at 0x4A072A1: operator new(unsigned long) (vg_replace_malloc.c:298)
   by 0x80820F8: webrtc::ModuleVideoRenderImpl::AddIncomingRenderStream(unsigned int, unsigned int, float, float, float, float) (video_render_impl.cc:470)
   by 0x80A82BC: webrtc::ViERenderer::Init(unsigned int, float, float, float, float) (vie_renderer.cc:120)
   by 0x80A841D: webrtc::ViERenderer::CreateViERenderer(int, int, webrtc::VideoRender&, webrtc::ViERenderManager&, unsigned int, float, float, float, float) (vie_renderer.cc:31)
   by 0x80A899C: webrtc::ViERenderManager::AddRenderStream(int, void*, unsigned int, float, float, float, float) (vie_render_manager.cc:142)
   by 0x808F6F7: webrtc::ViERenderImpl::AddRenderer(int, webrtc::RawVideoType, webrtc::ExternalRenderer*) (vie_render_impl.cc:384)
   by 0x74CD429: mozilla::MediaEngineWebRTCVideoSource::Start(mozilla::SourceMediaStream*, int) (MediaEngineWebRTCVideo.cpp:269)
   by 0x7344A86: mozilla::MediaOperationRunnable::Run() (MediaManager.h:224)
   by 0x7C74686: nsThread::ProcessNextEvent(bool, bool*) (nsThread.cpp:627)
   by 0x7C3B1D0: NS_ProcessNextEvent_P(nsIThread*, bool) (nsThreadUtils.cpp:237)
   by 0x7C751F9: nsThread::ThreadFunc(void*) (nsThread.cpp:265)
   by 0x505E156: _pt_root (ptthread.c:158)

----------------------------------------------------------------
(Assignee)

Comment 2

6 years ago
Upstream issue with webrtc.org trunk; we need to file an issue (and verify it's not fixed).
Assignee: nobody → rjesup
Depends on: 818631
Priority: -- → P2
Whiteboard: [webrtc][blocking-webrtc+]
(Reporter)

Updated

6 years ago
Whiteboard: [webrtc][blocking-webrtc+] → [webrtc][blocking-webrtc+] [data-race] [helgrind]
(Assignee)

Comment 3

6 years ago
I need to generate a patch for this
(Assignee)

Comment 4

6 years ago
Created attachment 721670 [details] [diff] [review]
move critsect to cover thread existence test (upstream issue 1465)
(Assignee)

Updated

6 years ago
Attachment #721670 - Flags: review?(tterribe)
Comment on attachment 721670 [details] [diff] [review]
move critsect to cover thread existence test (upstream issue 1465)

Review of attachment 721670 [details] [diff] [review]:
-----------------------------------------------------------------

Don't forget to file that upstream issue.
Attachment #721670 - Flags: review?(tterribe) → review+
(Assignee)

Comment 6

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ff35a246bb0

Already filed (note the comment on the patch - Issue 1465)
status-firefox21: --- → affected
status-firefox22: --- → fixed
Target Milestone: --- → mozilla22
https://hg.mozilla.org/mozilla-central/rev/8ff35a246bb0
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Backed out for now while we investigate bug 848966. I'll re-land whatever comes up clean.
https://hg.mozilla.org/integration/mozilla-inbound/rev/3cb82d9f3f9c
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/6cab296a4940
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Whiteboard: [webrtc][blocking-webrtc+] [data-race] [helgrind] → [webrtc][blocking-webrtc+] [data-race] [helgrind][qa-]
You need to log in before you can comment on or make changes to this bug.