Prevent concurrent access to data in InMemoryPerProcess' resolveXXX methods #3216
Conversation
eduar-hte
force-pushed
the
inmemory-collection-shared-mutex
branch
3 times, most recently
from
461a999 to
40888dc
Compare
|
@airween: I took a look at the multithreaded example you mentioned here (and adapted it to align with the similar code in the Do you want me to upload it as part of this PR both as an example and also if you want to test some other rules to stress test this code? |
eduar-hte
force-pushed
the
inmemory-collection-shared-mutex
branch
from
4e4c044 to
3718eca
Compare
eduar-hte
force-pushed
the
inmemory-collection-shared-mutex
branch
from
3718eca to
c51500c
Compare
|
@eduar-hte - please let us know if you finished the work on this PR. Btw: have you seen the Sonarcloud's notifications? |
I think I'm done with the changes, as I tried to limit them to introduce the There is still this question I tried to ask you in this previous comment about whether we should add the multithreaded example you were working on in the context of that issue and that I reworked to align with the changes to the
Yes, I went through all of them. Most of them ( Some of them ( |
eduar-hte
force-pushed
the
inmemory-collection-shared-mutex
branch
from
c51500c to
3d9027b
Compare
Yes, I think that would be fine - thanks! |
@eduar-hte sorry, seems you've already uploaded with commit 3d9027b - thanks. |
airween
left a comment
airween
left a comment
There was a problem hiding this comment.
LGTM - thanks for this very impressive PR.
No, those were updates to the existing example, I've just uploaded your test from #3054 (and rewritten it to use libModSecurity C++ API and |
eduar-hte
force-pushed
the
inmemory-collection-shared-mutex
branch
4 times, most recently
from
df25d02 to
da2bc50
Compare
- As reported in owasp-modsecurity#3054, the resolve methods in InMemoryPerProcess are not acquiring a lock/mutex to prevent concurrent access to the data structures that may be modified at the same time from other threads, and thus triggering undefined behaviour. - Replace inheritance of std::unordered_multimap data structure with data member to prevent potential clients to use it without acquiring the mutex to protect concurrent access. - Replace pthreads lock with std C++11 std::shared_mutex - Provides exclusive/shared lock access so that multiple readers can access the data at the same time, but only one writer. this is used to favor query performance by allowing more concurrent access to the data until an update needs to be performed. - Simplifies acquisition and disposal of lock/mutex with std::lock_guard, which has RAII semantics. - NOTE: Because std::shared_mutex is not recursive, calls to another function that tries to acquire the lock will fail. Introduced __store & __updateFirst helper methods to workaround this. - Updates to InMemoryPerProcess::resolveFirst - Updated the code to store the expired var in 'expiredVars' to delete them after iterating over the range (and releasing the read lock, as 'delIfExpired' needs to acquire it for exclusive access), as the current call to 'delIfExpired' would invalidate the range triggering undefined behaviour on the following iteration. - Noticed that in commit 118e1b3 the call to 'delIfExpired' in this function is done using 'it->second.getValue()'' instead of 'it->first', which seems incorrect (based on similar code in other resolveXXX functions). - Updated InMemoryPerProcess::delIfExpired to use 'std::find_if' (with a lambda that matches both the key and the 'isExpired' condition) because the data structure is a multimap. The version introduced in commit 118e1b3 could find an entry (not necessarily the first, because the map is unordered) where 'isExpired' is 'false' and exit, while another entry could be expired.
…other platforms - Replaced WITHOUT_XXX build options with WITH_XXX to make it easier to understand and configure. - Updated GitHub workflow to align with these changes and include a build 'with lmdb' (again, analogous to non-Windows configurations)
- Replaced pthread_mutex_t in modsecurity::operators::Pm with std::mutex - Replaced pthread's thread usage in reading_logs_via_rule_message example with std::thread. - Simplified and modernized C++ code. - Removed unnecessary includes of pthread.h
…rween) - Rewritten to use C++ libModSecurity API and std::thread (instead of pthreads)
eduar-hte
force-pushed
the
inmemory-collection-shared-mutex
branch
from
da2bc50 to
4bf9616
Compare
|
@airween - this is now done. when this is merged, issue #3054 should be closed. |
3 participants
what
Lock access to
InMemoryPerProcess' data inresolveXXXmethods.why
As reported in #3054, the
resolveXXXmethods inInMemoryPerProcessare not acquiring a lock/mutex to prevent concurrent access to the data structures that may be modified at the same time from other threads, and thus triggering undefined behaviour.changes
std::unordered_multimapdata structure with data member to prevent potential clients to use it without acquiring the mutex to protect concurrent access.InMemoryPerProcess::resolveFirstexpiredVarsto delete them after iterating over the range (and releasing the read lock, asdelIfExpiredneeds to acquire it for exclusive access), as the current call todelIfExpiredwould invalidate the range triggering undefined behaviour on the followingiteration.delIfExpiredin this function is done usingit->second.getValue()instead ofit->first, which seems incorrect (based on similar code in otherresolveXXXfunctions).InMemoryPerProcess::delIfExpiredto usestd::find_if(with a lambda that matches both the key and theisExpiredcondition) because the data structure is a multimap. The version introduced in commit 118e1b3 could find an entry (not necessarily the first, because the map is unordered) whereisExpiredisfalseand exit, while another entry could be expired.std::shared_mutexstd::shared_mutexis not recursive, calls to another function that tries to acquire the lock will fail. Introduced__store&__updateFirsthelper methods to workaround this.