Review and document multi-threading support and limitations

[eduar-hte]

The goal of this issue is to collect and centralize information about multi-threading support of the current version of modSecurity , as the topic has come up in a number of issues and there are documented guidelines about usage in this context.

Multi-threading support

The library is expected to work in multi-threaded scenarios, as stated in #1726. This means that any issues should be related to incorrect usage or bugs in the current implementation.

"The modSecurity life cycle is divided into different stages. The stage that the rules are loaded is not threading safe by design. (...) Once the rules are loaded multiple requests can share the same RulesSet object, leading to parallelism while addressing different requests in different processes or threads." (Source: #2536, here)

Notes

• Operator & Action (including Transformation) objects are created in the first stage, when rules are loaded and parsed, so their initialization is not synchronized. If their evaluation updates internal state, this needs to be protected to prevent issues in multithreaded contexts.

Examples

The repository currently includes an example of usage of the library in a multi-threaded context, see reading_logs_via_rule_message in the examples directory.

Potential issues/limitations

• Shared files (audit or debug log) potential deadlock or incorrect behaviour on non-Windows platforms
  • NOTE: This has not been reproduced or reported as an issue, but is documented here as discussed in the context of a similar issue in the Windows port.
  • A deadlock using shared files was found in the initial port of the library to Windows (Add support to build libModSecurity v3 on Windows #3132) which was addressed in Fixed shared files deadlock in a multi-threaded Windows application #3210.
  • The PR discussion mentions the possibility of a similar issue (or other incorrect behaviour) happening in non-Unix platforms due to the use of the F_SETLKW fcntl to lock the shared files.
  • The library previously used a mutex (using shared memory to make it available to other processes in a multi-process context), but this was replaced by the F_SETLKW fcntl in commit 3d20304 due to an unlocking issue under heavy load using nginx, where an acquired lock would not be released when the process was killed.
  • A possible way to address this would be to go back to the mutex (plus shared memory) implementation but using the robust mutex feature in pthreads, that when a process terminates while holding the mutex notifies the next acquirer of this situation with the EOWNERDEAD return value, which allows it to recover the mutex and make it 'consistent' (see PTHREAD_MUTEX_ROBUST).

Reviewed/addressed issues

• InMemoryPerProcess potential issues in multi-threading scenarios
  • This has been reported in Inadequate locking for multithreading in libModSecurity In-Memory persistent storage #3054 and points to the fact that the InMemoryPerProcess class has incorrect locking of its resources, as it only protects updates to the structures but not reads, which would generate undefined behaviour if a thread was updating the structure and then at the same time another threads tries to read the structure.
  • This has been addressed in PR Prevent concurrent access to data in InMemoryPerProcess' resolveXXX methods #3216.
• MODSEC_MUTEX_ON_PM define & --enable-mutex-on-pm configure flag
  • The Pm operator includes an optional mutex when accessing acmp trees.
  • This was introduced in commit 119a6fc & 7d786b3 likely because of issue double free in ModSecurity3 'reading_logs_via_rule_message' example and OWASP CRS #1573.
  • This option is off by default and it's not clear whether it's necessary, as stated here.
  • The need for this optional lock was reviewed and confirmed not to be necessary.
  • Removed in PR Removed unnecessary lock to call acmp_process_quick in Pm::evaluate #3227.
• string.h's ascTime uses std::ctime, which is not safe in multi-threaded contexts
  • From IBM's ctime documentation:
    • The asctime() and ctime() functions, and other time functions can use a common, statically allocated buffer to hold the return string. Each call to one of these functions might destroy the result of the previous call. The asctime_r(), ctime_r(), gmtime_r(), and localtime_r() functions do not use a common, statically allocated buffer to hold the return string. These functions can be used in place of asctime(), ctime(), gmtime(), and localtime() if reentrancy is desired.
  • ascTime is used by Transaction::toJSON, which is used to log transactions.
  • This was reported by sonarcloud while working on PR Remove several string copies and unnecessary heap allocations #3222:
    • Non-reentrant POSIX functions should be replaced with their reentrant versions cpp:S1912
  • This has been addressed in PR Replace usage of std::ctime, which is not safe in multithread contexts #3228.

Misc

• The unit_test program has support to run the operator/transformation tests in a multi-threaded context (launching 50 threads and running each test 5000 times).
  • The goal is to check if the evaluation of the operator/transformation triggers an issue or unexpected result.
  • Notice that allocation & initialization of the operator/transformation is performed in the main thread (as this is done in the stage where rules are loaded, see above).
  • This feature was introduced in PR Add support to run unit tests in a multithreaded context #3221.

[eduar-hte]

"The modSecurity life cycle is divided into different stages. The stage that the rules are loaded is not threading safe by design. (...) Once the rules are loaded multiple requests can share the same RulesSet object, leading to parallelism while addressing different requests in different processes or threads." (Source: #2536, here)

@airween: I think #3138 could be closed with a reference to this.

[eduar-hte]

• InMemoryPerProcess potential issues in multi-threading scenarios

Created PR #3216 to try to correct this.

[eduar-hte]

• MODSEC_MUTEX_ON_PM define & --enable-mutex-on-pm configure flag

The need for this optional lock was reviewed and confirmed not to be necessary. PR #3227 was submitted to remove it.

[eduar-hte]

• string.h's ascTime uses std::ctime, which is not safe in multi-threaded contexts

Created PR #3228 to address this.