828917 - data race on IncomingVideoStream::incoming_render_thread_

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect, P2)

Description

[Julian Seward [:jseward]]

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.

Comment 1

[Julian Seward [:jseward]]

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)

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

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Upstream issue with webrtc.org trunk; we need to file an issue (and verify it's not fixed).

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

I need to generate a patch for this

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

Attachment: move critsect to cover thread existence test (upstream issue 1465)

Comment 5

[Timothy B. Terriberry (:derf)]

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.

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

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

Already filed (note the comment on the patch - Issue 1465)

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8ff35a246bb0

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6cab296a4940

Windows PGO M2 is looking green.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6cab296a4940