google / re2

RE2 is a fast, safe, thread-friendly alternative to backtracking regular expression engines like those used in PCRE, Perl, and Python. It is a C++ library.
BSD 3-Clause "New" or "Revised" License
8.91k stars 1.13k forks source link

Data race condition in void DFA::RWLocker::LockForWriting() #464

Closed vinaygund closed 10 months ago

vinaygund commented 10 months ago

Hi , in my project I use Coverity tool {Coverity is a static analysis tool used for identifying software security vulnerabilities, quality defects, and code maintenance issues in source code.}

in dfa.cc file line 1132.

image


this is the code and warning reported by coverity in above snapshot ,

1130// This function is marked as NO_THREAD_SAFETY_ANALYSIS because 1131// the annotations don't support lock upgrade. 1132void DFA::RWLocker::LockForWriting() NO_THREAD_SAFETY_ANALYSIS {

CID 155380 (#1 of 1): Data race condition (MISSING_LOCK)

  1. missinglock: Accessing this->writing without holding lock re2::Mutex.mutex. Elsewhere, re2::DFA::RWLocker.writing is written to with Mutex.mutex held 1 out of 1 times (1 of these accesses strongly imply that it is necessary). 1133 if (!writing) { 1134 mu_->ReaderUnlock(); A1. lockacquire: Example 1: Calling WriterLock acquires lock re2::Mutex.mutex. [hide details] 1135 mu->WriterLock(); /Thirdparty/re2/util/mutex.h 53 inline void ReaderUnlock(); // Release a read share of this Mutex A1. lock: Lock locks this->mutex. 54 inline void WriterLock() { Lock(); } // Acquire an exclusive lock 55 inline void WriterUnlock() { Unlock(); } // Release a lock from WriterLock() A2. exampleaccess: Example 1 (cont.): re2::DFA::RWLocker.writing is written to with lock re2::Mutex.mutex held. 1136 writing = true; 1137 } 1138} 1139

please provide your feedback.

Regards vinay

junyer commented 10 months ago

This is a false positive. I'm guessing that Coverity doesn't recognise NO_THREAD_SAFETY_ANALYSIS.

junyer commented 10 months ago

Oh, hang on. You are using an older version of RE2 that predates the Abseil dependency. NO_THREAD_SAFETY_ANALYSIS was a no-op, which is presumably why the false positive arose. Nowadays, ABSL_NO_THREAD_SAFETY_ANALYSIS is used and Coverity should hopefully recognise that.