C++17 introduced a construct to create and initialize a variable within the
condition of if and switch statements, and C++20 added this construct to
range-based for loops. Using this new feature simplifies common code patterns
and helps in giving variables the right scope.
Reference: https://sonarcloud.io/project/issues?open=AZDCieK2zGtqRpL2rnl-&id=owasp-modsecurity_ModSecurity
- After the GitHub macOS runner images were upgraded to macOS 14.6
(Sonoma), the test 'Include remote rules - failed download (Abort)'
started failing because the error message reported by curl/OS is no
longer 'HTTP response code said error'.
- Initialize `log` temporary value on construction instead of doing
default initialization and then calling `append`.
- Leverage `std::string_view` to replace `const std::string&` parameters
in `utils::string::replaceAll` to avoid creating a `std::string`
object (and associated allocation and copy) for the string literal`%d`
- Keep m_it->disruptive value and use it as return value to guarantee
that the value is correct.
- If m_it->disruptive is false and the 'it' argument has not been
initialized/cleaned, the function may incorrectly return a non-zero
value.
- When a disruptive intervention is being reported by the function,
defensively initialize log & url to NULL if there's no such data to
provide to the caller.
- If the caller has not initialized/cleaned those fields in the 'it'
argument, after returning from transaction::intervention, the user
can safely read the log & url fields and in all scenarios they'll
have valid values.
- The shared files Windows implementation introduced in PR #3132 works
in multi-process single-threaded contexts but it doesn't work
correctly in single-process multi-threaded contexts.
- The issue is that the LockFileEx Win32 function works on a per-handle
basis.
- In a multi-process context, each process will have called
SharedFiles::add_new_handler when initializing the SharedFile and
obtained a handle, and thus locking will work.
- When running ModSecurity in a single process using multiple threads,
the initialization of the SharedFile will happen once and the handle
will be shared by all threads. Then, if two threads try to write to
the same shared file concurrently, they may deadlock as one of them
will lock the file (by calling LockFileEx) and then proceed to write
to the file. If before writing to the file and unlocking it, another
thread calls LockFileEx on the same handle, the attempt to write to
the file will lock generating a deadlock.
- The new implementation replaces usage of LockFileEx/UnlockFileEx with
a named mutex to lock access to the shared file.
- A named mutex is used to support multi-process scenarios.
- The mutex name is generated using the filename to support multiple
shared files (such as that for the debug and audit logs).
- This assumes that both process will initialize the SharedFile
instance using the same filename (which is expected as they'd be
using the same configuration file)
- The following methods are introduced to allow clients of
libModSecurity that are not able to link and call the C/C++ standard
library to be able to free the buffers allocated by libModSecurity.
- msc_intervention_cleanup: Frees the buffers in a
ModSecurityIntervention structure that have been allocated by calls to
msc_intervention.
- msc_rules_error_cleanup: Frees an error message buffer allocated by
the msc_rules_xxx functions to detail the condition that triggered
the error.
Implemented a new configuration option --enable-assertions=[yes|no] within config.ac, enabling controlled inclusion of -DNDEBUG in CPPFLAGS. The default setting suppresses assertions (by adding -DNDEBUG to CPPFLAGS), preserving the original behavior. This enhancement allows for the optional enabling of assertions during development or debugging by setting --enable-assertions=yes, thereby excluding -DNDEBUG from CPPFLAGS.
- SonarCloud analysis identified standalone `throw;` calls without accompanying `try-catch` blocks, used inconsistently as placeholders or for premature termination under specific conditions.
- Removed these `throw;` instances to prevent potential runtime issues in future development phases, where such configurations might inadvertently be created.
- Introduced `assert` statements as a more appropriate mechanism for asserting preconditions in the affected class member functions, ensuring clearer intent and safer code behavior during development.
- Refactor action_kind processing to use switch() instead of if-else chains; add assertion in default case.
- Fix SonarCloud issue: Make this variable a const reference.
https://sonarcloud.io/project/issues?resolved=false&pullRequest=3104&id=owasp-modsecurity_ModSecurity&open=AY8Vpgy4f6U6E7VKL4Cn
- The previous approach would create a std::unique_ptr and store it in
a std::list in VariableValue (Origins)
- The new approach now stores Origins in a std::vector and constructs
VariableOrigin elements in-place on insertion.
- Instead of having two heap-allocations for every added VariableOrigin
instance, this performs only one.
- If multiple origins are added, std::vector's growth strategy may even
prevent a heap-allocation. There's a cost on growing the size of the
vector, because a copy of current elements will be necessary.
- Introduced reserveOrigin method to notify that multiple insertions
will be made, so that we can use std::vector's reserve and do a
single allocation (and copy of previous elements), and then just
initialize the new elements in-place.
