zqz/platform-external-webrtc
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix recursive locking in SignalThread.

Browse Source 
By requiring Release to be called with lock held. This is a bit of a
kludge, but I think this works, because all known users of this
deprecated class call Release either from OnWorkStop or
SignalWorkDone, and both are called with the lock already held.

Bug: webrtc:11567
Change-Id: Idf0007188e45a465aefcb8f13fea93a68930fe1c
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/204483
Reviewed-by: Tommi <tommi@webrtc.org>
Reviewed-by: Markus Handell <handellm@webrtc.org>
Commit-Queue: Niels Moller <nisse@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#33188}
This commit is contained in:
Niels Möller
2021-02-01 10:38:00 +01:00
committed by Commit Bot
parent a0848ddeff
commit 4e57d4bd77
2 changed files with 22 additions and 13 deletions
Download Patch File Download Diff File Expand all files Collapse all files

13
rtc_base/deprecated/signal_thread.cc
View File

@ -31,7 +31,7 @@ DEPRECATED_SignalThread::DEPRECATED_SignalThread()
} }
DEPRECATED_SignalThread::~DEPRECATED_SignalThread() { DEPRECATED_SignalThread::~DEPRECATED_SignalThread() {
rtc::CritScope lock(&cs_); webrtc::MutexLock lock(&mutex_);
RTC_DCHECK(refcount_ == 0); RTC_DCHECK(refcount_ == 0);
} }
@ -74,9 +74,9 @@ void DEPRECATED_SignalThread::Destroy(bool wait) {
OnWorkStop(); OnWorkStop();
if (wait) { if (wait) {
// Release the thread's lock so that it can return from ::Run. // Release the thread's lock so that it can return from ::Run.
cs_.Leave(); mutex_.Unlock();
worker_.Stop(); worker_.Stop();
cs_.Enter(); mutex_.Lock();
refcount_--; refcount_--;
} }
} else { } else {
@ -84,8 +84,11 @@ void DEPRECATED_SignalThread::Destroy(bool wait) {
} }
} }
void DEPRECATED_SignalThread::Release() { // Disable analysis, to allow calls via SignalWorkDone (which already holds the
EnterExit ee(this); // lock).
// TODO(bugs.webrtc.org/11567): Add a Mutex::AssertHeld, to reenable analysis
// and get a runtime check.
void DEPRECATED_SignalThread::Release() RTC_NO_THREAD_SAFETY_ANALYSIS {
RTC_DCHECK(!destroy_called_); RTC_DCHECK(!destroy_called_);
RTC_DCHECK(main_->IsCurrent()); RTC_DCHECK(main_->IsCurrent());
if (kComplete == state_) { if (kComplete == state_) {

22
rtc_base/deprecated/signal_thread.h
View File

@ -15,9 +15,9 @@
#include "rtc_base/checks.h" #include "rtc_base/checks.h"
#include "rtc_base/constructor_magic.h" #include "rtc_base/constructor_magic.h"
#include "rtc_base/deprecated/recursive_critical_section.h"
#include "rtc_base/deprecation.h" #include "rtc_base/deprecation.h"
#include "rtc_base/message_handler.h" #include "rtc_base/message_handler.h"
#include "rtc_base/synchronization/mutex.h"
#include "rtc_base/third_party/sigslot/sigslot.h" #include "rtc_base/third_party/sigslot/sigslot.h"
#include "rtc_base/thread.h" #include "rtc_base/thread.h"
#include "rtc_base/thread_annotations.h" #include "rtc_base/thread_annotations.h"
@ -65,6 +65,12 @@ class DEPRECATED_SignalThread : public sigslot::has_slots<>,
// Context: Main Thread. If the worker thread is complete, deletes the // Context: Main Thread. If the worker thread is complete, deletes the
// object immediately. Otherwise, schedules the object to be deleted once // object immediately. Otherwise, schedules the object to be deleted once
// the worker thread completes. SignalWorkDone will be signalled. // the worker thread completes. SignalWorkDone will be signalled.
// BEWARE: This method must be called with the object's internal lock held, as
// if annotated RTC_EXCLUSIVE_LOCKS_REQUIRED(mutex_). Callbacks via OnWorkStop
// and SignalWorkDone are already holding the needed lock. It's not annotated,
// because it's hard to tell the compiler that functions called via
// SignalWorkDone already hold the lock.
void Release(); void Release();
// Context: Main Thread. Signalled when work is complete. // Context: Main Thread. Signalled when work is complete.
@ -126,9 +132,9 @@ class DEPRECATED_SignalThread : public sigslot::has_slots<>,
class RTC_SCOPED_LOCKABLE EnterExit { class RTC_SCOPED_LOCKABLE EnterExit {
public: public:
explicit EnterExit(DEPRECATED_SignalThread* t) explicit EnterExit(DEPRECATED_SignalThread* t)
RTC_EXCLUSIVE_LOCK_FUNCTION(t->cs_) RTC_EXCLUSIVE_LOCK_FUNCTION(t->mutex_)
: t_(t) { : t_(t) {
t_->cs_.Enter(); t_->mutex_.Lock();
// If refcount_ is zero then the object has already been deleted and we // If refcount_ is zero then the object has already been deleted and we
// will be double-deleting it in ~EnterExit()! (shouldn't happen) // will be double-deleting it in ~EnterExit()! (shouldn't happen)
RTC_DCHECK_NE(0, t_->refcount_); RTC_DCHECK_NE(0, t_->refcount_);
@ -141,7 +147,7 @@ class DEPRECATED_SignalThread : public sigslot::has_slots<>,
~EnterExit() RTC_UNLOCK_FUNCTION() { ~EnterExit() RTC_UNLOCK_FUNCTION() {
bool d = (0 == --t_->refcount_); bool d = (0 == --t_->refcount_);
t_->cs_.Leave(); t_->mutex_.Unlock();
if (d) if (d)
delete t_; delete t_;
} }
@ -155,10 +161,10 @@ class DEPRECATED_SignalThread : public sigslot::has_slots<>,
Thread* main_; Thread* main_;
Worker worker_; Worker worker_;
RecursiveCriticalSection cs_; webrtc::Mutex mutex_;
State state_ RTC_GUARDED_BY(cs_); State state_ RTC_GUARDED_BY(mutex_);
int refcount_ RTC_GUARDED_BY(cs_); int refcount_ RTC_GUARDED_BY(mutex_);
bool destroy_called_ RTC_GUARDED_BY(cs_) = false; bool destroy_called_ RTC_GUARDED_BY(mutex_) = false;
RTC_DISALLOW_COPY_AND_ASSIGN(DEPRECATED_SignalThread); RTC_DISALLOW_COPY_AND_ASSIGN(DEPRECATED_SignalThread);
}; };