diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3350743b16..27e0828c82 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -32,7 +32,7 @@ jobs: - name: build.sh run: ./build.sh - name: configure ${{ matrix.configure.label }} - run: ./configure ${{ matrix.configure.opt }} + run: ./configure ${{ matrix.configure.opt }} --enable-assertions=yes - uses: ammaraskar/gcc-problem-matcher@master - name: make run: make -j `nproc` @@ -66,7 +66,7 @@ jobs: - name: build.sh run: ./build.sh - name: configure ${{ matrix.configure.label }} - run: ./configure ${{ matrix.configure.opt }} + run: ./configure ${{ matrix.configure.opt }} --enable-assertions=yes - uses: ammaraskar/gcc-problem-matcher@master - name: make run: make -j `sysctl -n hw.logicalcpu` diff --git a/README.md b/README.md index 3be0881080..7315db844e 100644 --- a/README.md +++ b/README.md @@ -213,6 +213,12 @@ $ cd test $ ./regression-tests $ ./unit-tests ``` +Please take care that the '/test/cppcheck_suppressions.txt' +file contains hardcoded 'filename:line number' suppressions. If you modify a +file with explicit suppressions, you must update the 'cppcheck_suppressions.txt' +accordingly to ensure the suppression references remain aligned with the +intended lines of code. Failing to do so might trigger 'make check-static' to +break the build with errors that are logically unrelated to your modifications. ### Debugging @@ -231,11 +237,16 @@ CFLAGS to disable the compilation optimization parameters: ```shell $ export CFLAGS="-g -O0" $ ./build.sh -$ ./configure +$ ./configure --enable-assertions=yes $ make $ sudo make install ``` +"Assertions allow us to document assumptions and to spot violations early in the +development process. What is more, assertions allow us to spot violations with a +minimum of effort." https://dl.acm.org/doi/pdf/10.1145/240964.240969 +It is recommended to use assertions where applicable, and to enable them with +'--enable-assertions=yes' during the testing and debugging workflow. ## Reporting Issues diff --git a/configure.ac b/configure.ac index d8636036b9..06fd5e2dab 100644 --- a/configure.ac +++ b/configure.ac @@ -227,6 +227,19 @@ MSC_GIT_VERSION=msc_version_git AC_SUBST([MSC_GIT_VERSION]) + +AC_ARG_ENABLE(assertions, + [AS_HELP_STRING([--enable-assertions],[Turn on assertions feature: undefine NDEBUG])], + + [case "${enableval}" in + yes) assertions=true ;; + no) assertions=false ;; + *) AC_MSG_ERROR(bad value ${enableval} for --enable-assertions) ;; + esac], + + [assertions=false] + ) + AC_ARG_ENABLE(debug-logs, [AS_HELP_STRING([--disable-debug-logs],[Turn off the SecDebugLog feature])], @@ -355,6 +368,14 @@ if test "$aflFuzzer" == "true"; then GLOBAL_CPPFLAGS="$GLOBAL_CPPFLAGS $FUZZ_CPPCFLAGS" $buildExamples = false fi + +case $assertions in + false) ASSERTIONS_CPPCFLAGS="-DNDEBUG" ;; + true) ASSERTIONS_CPPCFLAGS="-UNDEBUG" ;; + *) AC_MSG_ERROR(bad value ${assertions} for assertions) ;; +esac +GLOBAL_CPPFLAGS="$GLOBAL_CPPFLAGS $ASSERTIONS_CPPCFLAGS" + AC_SUBST(GLOBAL_LDADD) AC_SUBST(GLOBAL_CPPFLAGS) @@ -589,6 +610,14 @@ if test $buildTestUtilities = true; then else echo " + Test Utilities ....disabled" fi + + +if test $assertions = true; then + echo " + Assertions ....enabled" +else + echo " + Assertions ....disabled" +fi + if test $debugLogs = true; then echo " + SecDebugLog ....enabled" else diff --git a/headers/modsecurity/transaction.h b/headers/modsecurity/transaction.h index b0dc4b7145..a923ad6ac2 100644 --- a/headers/modsecurity/transaction.h +++ b/headers/modsecurity/transaction.h @@ -14,6 +14,7 @@ */ #ifdef __cplusplus +#include #include #include #include @@ -307,11 +308,8 @@ class TransactionSecMarkerManagement { } std::shared_ptr getCurrentMarker() const { - if (m_marker) { - return m_marker; - } else { - throw; - } + assert((m_marker != nullptr) && "You might have forgotten to call and evaluate isInsideAMarker() before calling getCurrentMarker()."); + return m_marker; } void removeMarker() { diff --git a/src/rule_with_actions.cc b/src/rule_with_actions.cc index 04ee219c4b..5084fa2361 100644 --- a/src/rule_with_actions.cc +++ b/src/rule_with_actions.cc @@ -17,6 +17,7 @@ #include +#include #include #include #include @@ -86,45 +87,51 @@ RuleWithActions::RuleWithActions( if (actions) { for (Action *a : *actions) { - if (a->action_kind == Action::ConfigurationKind) { - a->evaluate(this, NULL); - delete a; - - } else if (a->action_kind == Action::RunTimeOnlyIfMatchKind) { - if (dynamic_cast(a)) { - m_containsCaptureAction = true; - delete a; - } else if (dynamic_cast(a)) { - m_containsMultiMatchAction = true; + switch (a->action_kind) { + case Action::ConfigurationKind: + a->evaluate(this, NULL); delete a; - } else if (dynamic_cast(a)) { - m_severity = dynamic_cast(a); - } else if (dynamic_cast(a)) { - m_logData = dynamic_cast(a); - } else if (dynamic_cast(a)) { - m_msg = dynamic_cast(a); - } else if (dynamic_cast(a)) { - m_actionsSetVar.push_back( - dynamic_cast(a)); - } else if (dynamic_cast(a)) { - m_actionsTag.push_back(dynamic_cast(a)); - } else if (dynamic_cast(a)) { - m_actionsRuntimePos.push_back(a); - m_containsStaticBlockAction = true; - } else if (a->isDisruptive() == true) { - if (m_disruptiveAction != nullptr) { - delete m_disruptiveAction; - m_disruptiveAction = nullptr; + break; + case Action::RunTimeOnlyIfMatchKind: + if (dynamic_cast(a)) { + m_containsCaptureAction = true; + delete a; + } else if (dynamic_cast(a)) { + m_containsMultiMatchAction = true; + delete a; + } else if (dynamic_cast(a)) { + m_severity = dynamic_cast(a); + } else if (dynamic_cast(a)) { + m_logData = dynamic_cast(a); + } else if (dynamic_cast(a)) { + m_msg = dynamic_cast(a); + } else if (dynamic_cast(a)) { + m_actionsSetVar.push_back( + dynamic_cast(a)); + } else if (dynamic_cast(a)) { + m_actionsTag.push_back(dynamic_cast(a)); + } else if (dynamic_cast(a)) { + m_actionsRuntimePos.push_back(a); + m_containsStaticBlockAction = true; + } else if (a->isDisruptive() == true) { + if (m_disruptiveAction != nullptr) { + delete m_disruptiveAction; + m_disruptiveAction = nullptr; + } + m_disruptiveAction = a; + } else { + m_actionsRuntimePos.push_back(a); } - m_disruptiveAction = a; - } else { - m_actionsRuntimePos.push_back(a); - } - } else { - delete a; - std::cout << "General failure, action: " << a->m_name; - std::cout << " has an unknown type." << std::endl; - throw; + break; + default: + std::cout << "General failure, action: " << a->m_name; + std::cout << " has an unknown type." << std::endl; + delete a; + #ifdef NDEBUG + break; + #else + assert(false); + #endif } } delete actions; @@ -241,7 +248,7 @@ void RuleWithActions::executeActionsAfterFullMatch(Transaction *trans, bool containsBlock, std::shared_ptr ruleMessage) { bool disruptiveAlreadyExecuted = false; - for (auto &a : trans->m_rules->m_defaultActions[getPhase()]) { + for (const auto &a : trans->m_rules->m_defaultActions[getPhase()]) { // cppcheck_suppressions.txt:55 if (a.get()->action_kind != actions::Action::RunTimeOnlyIfMatchKind) { continue; } diff --git a/test/cppcheck_suppressions.txt b/test/cppcheck_suppressions.txt index ebbc665ee0..51f4c9ef07 100644 --- a/test/cppcheck_suppressions.txt +++ b/test/cppcheck_suppressions.txt @@ -52,9 +52,7 @@ noConstructor:src/variables/variable.h:152 danglingTempReference:src/modsecurity.cc:206 knownConditionTrueFalse:src/operators/validate_url_encoding.cc:77 knownConditionTrueFalse:src/operators/verify_svnr.cc:87 -rethrowNoCurrentException:headers/modsecurity/transaction.h:313 -rethrowNoCurrentException:src/rule_with_actions.cc:127 -ctunullpointer:src/rule_with_actions.cc:244 +ctunullpointer:src/rule_with_actions.cc:251 ctunullpointer:src/rule_with_operator.cc:135 ctunullpointer:src/rule_with_operator.cc:95 passedByValue:test/common/modsecurity_test.cc:49