diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d745ba4457..04a99b0053 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -12,8 +12,8 @@ jobs: matrix: os: [ubuntu-22.04] platform: - - {label: "x64", arch: "amd64", configure: ""} - - {label: "x32", arch: "i386", configure: "PKG_CONFIG_PATH=/usr/lib/i386-linux-gnu/pkgconfig CFLAGS=-m32 CXXFLAGS=-m32 LDFLAGS=-m32"} + - {label: "x64", arch: "amd64", configure: "--enable-assertions=yes"} + - {label: "x32", arch: "i386", configure: "PKG_CONFIG_PATH=/usr/lib/i386-linux-gnu/pkgconfig CFLAGS=-m32 CXXFLAGS=-m32 LDFLAGS=-m32 --enable-assertions=yes"} compiler: - {label: "gcc", cc: "gcc", cxx: "g++"} - {label: "clang", cc: "clang", cxx: "clang++"} @@ -112,7 +112,7 @@ jobs: - name: build.sh run: ./build.sh - name: configure - 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 36201735ff..704967d6ed 100644 --- a/README.md +++ b/README.md @@ -236,10 +236,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. ### Benchmarking diff --git a/configure.ac b/configure.ac index f69c5c61d1..d894316421 100644 --- a/configure.ac +++ b/configure.ac @@ -248,6 +248,17 @@ AC_SUBST([MSC_VERSION]) 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])], @@ -377,6 +388,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) @@ -613,6 +632,11 @@ 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 9caace2c9a..e35ed6ebda 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; // cppcheck-suppress rethrowNoCurrentException - } + 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 5ae7d407e7..7314b5c84f 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; // cppcheck-suppress rethrowNoCurrentException + 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; @@ -239,7 +246,7 @@ void RuleWithActions::executeActionsAfterFullMatch(Transaction *trans, bool containsBlock, std::shared_ptr ruleMessage) { bool disruptiveAlreadyExecuted = false; - for (auto &a : trans->m_rules->m_defaultActions[getPhase()]) { // cppcheck-suppress ctunullpointer + for (const auto &a : trans->m_rules->m_defaultActions[getPhase()]) { // cppcheck-suppress ctunullpointer if (a.get()->action_kind != actions::Action::RunTimeOnlyIfMatchKind) { continue; }