[core] Fix a data race by adding a shared mutex #2961
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
maxsharabayko
added
the
Type: Bug
| @@ -67,6 +67,36 @@ namespace srt | |||
| { | |||
| class CChannel; | |||
| class CUDT; | |||
| class CUDTWrapper; | |||
There was a problem hiding this comment.
The class definition follows below. This declaration is excessive.
| class CUDTWrapper { | ||
| public: | ||
| CUDT *udt; | ||
| sync::SharedMutex mut; | ||
|
|
||
| public: | ||
| CUDTWrapper() | ||
| :udt(NULL) | ||
| ,mut() | ||
| { | ||
| } | ||
| void lockRead() | ||
| { | ||
| return mut.lockRead(); | ||
| } | ||
| void lockWrite() | ||
| { | ||
| return mut.lockWrite(); | ||
| } | ||
| void unlockRead() | ||
| { | ||
| return mut.unlockRead(); | ||
|
|
||
| } | ||
| void unlockWrite(){ | ||
| return mut.unlockWrite(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
I don't quite like the name. It does not express what the class does, except for wrapping CUDT.
In fact, it can be a template class holding a pointer to a resource and a shared mutex, e.g.
template <class T>
class CSharedObject
{
private:
T* m_pObj;
sync::SharedMutex m_mtx;
public:
// ...
};| class CUDTWrapper; | ||
|
|
||
| class CUDTWrapper { | ||
| public: |
There was a problem hiding this comment.
| public: | |
| private: |
| Mutex m_pMutex; | ||
|
|
||
| int m_pCountRead; | ||
| bool m_pWriterLocked; |
There was a problem hiding this comment.
'p' means "pointer". This variable is of a type "Boolean", so should be m_bWriterLocked.
|
|
||
| Mutex m_pMutex; | ||
|
|
||
| int m_pCountRead; |
There was a problem hiding this comment.
"p" mean pointer, while the variable is not a pointer.
Please review the whole class.
| /* REFERENCE IMPLEMENTATION | ||
| class shared_mutex | ||
| { | ||
| Mutex mut_; | ||
| Condition gate1_; | ||
| Condition gate2_; | ||
| unsigned state_; | ||
|
|
||
| static const unsigned write_entered_ = 1U << (sizeof(unsigned)*CHAR_BIT - 1); | ||
| static const unsigned n_readers_ = ~write_entered_; | ||
|
|
||
| public: | ||
|
|
||
| shared_mutex() : state_(0) {} | ||
|
|
||
|
|
||
| // Exclusive ownership | ||
|
|
||
| void | ||
| lock() | ||
| { | ||
| UniqueLock lk(mut_); | ||
| std::cout << "LOCK WRITE " << std::endl; | ||
| while (state_ & write_entered_) | ||
| gate1_.wait(lk); | ||
| state_ |= write_entered_; | ||
| while (state_ & n_readers_) | ||
| gate2_.wait(lk); | ||
| std::cout << "LOCK WRITE DONE" << std::endl; | ||
|
|
||
| } | ||
|
|
||
| void | ||
| unlock() | ||
| { | ||
| { | ||
| ScopedLock _(mut_); | ||
| state_ = 0; | ||
| } | ||
| std::cout << "UNLOCK WRITE " << std::endl; | ||
| gate1_.notify_all(); | ||
| std::cout << "UNLOCK WRITE DONE" << std::endl; | ||
|
|
||
| } | ||
|
|
||
| // Shared ownership | ||
|
|
||
| void | ||
| lock_shared() | ||
| { | ||
| UniqueLock lk(mut_); | ||
| while ((state_ & write_entered_) || (state_ & n_readers_) == n_readers_) | ||
| gate1_.wait(lk); | ||
| unsigned num_readers = (state_ & n_readers_) + 1; | ||
| state_ &= ~n_readers_; | ||
| state_ |= num_readers; | ||
| } | ||
|
|
||
| void | ||
| unlock_shared() | ||
| { | ||
| ScopedLock _(mut_); | ||
| unsigned num_readers = (state_ & n_readers_) - 1; | ||
| state_ &= ~n_readers_; | ||
| state_ |= num_readers; | ||
| if (state_ & write_entered_) | ||
| { | ||
| if (num_readers == 0) | ||
| gate2_.notify_one(); | ||
| } | ||
| else | ||
| { | ||
| if (num_readers == n_readers_ - 1) | ||
| gate1_.notify_one(); | ||
| } | ||
| } | ||
| };*/ |
Check notice
Code scanning / CodeQL
Commented-out code Note
|
|
||
| if (u == m_pListener) | ||
| m_pListener = NULL; | ||
| //ScopedLock lslock(m_LSLock); |
Check notice
Code scanning / CodeQL
Commented-out code Note
No description provided.