Removed unnecessary lock to call acmp_process_quick in Pm::evaluate #3227
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- This was introduced in commit 119a6fc & 7d786b3 because of a potential issue reported in owasp-modsecurity#1573. - The ACMP tree structure is initialized when the operator is initialized. - During transaction execution the ACMP tree structure is only 'read' while traversing the tree (in acmp_process_quick) so this is safe for use in a multi-threaded environment.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
what
Remove the lock in the
Pmoperator, which is off by default (it can be enabled with the--enable-mutex-on-pmconfigure flag, which adds the define forMODSEC_MUTEX_ON_PM) and is not needed.why
This lock was introduced in commit 119a6fc & 7d786b3 at the same time (and probably because of) issue #1573, where a double-free issue running in a multi-threaded context was reported.
Some time later, it's stated that the lock should not be necessary (see here), and that it should be safe without the mutex (the issue explicitly asks about whether it's necessary for multithreaded environments).
Reviewing commit 119a6fc, it includes two separate changes:
acmp_process_quickin the same instance of the operator at the same time.acmp_preparefromacmp_process_quicktoPm::init.The second change is significant because it removes (comments out to be precise) the code that could potentially modify the data structure during the execution of
acmp_process_quick. The updated version ofacmp_process_quickonly reads the data structure (which once initialized inPm::initis not modified afterwards), so it can be safely executed concurrently in a multithreaded context.This is why the lock is not needed and can be removed.