Merge pull request #3207 from gberkes/v3/remove_this_throw_call_transaction_h_mk2

V3/remove this throw call transaction h mk2

Author: airween

.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`

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
 

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

headers/modsecurity/transaction.h

@@ -14,6 +14,7 @@
  */
 
 #ifdef __cplusplus
+#include <cassert>
 #include <ctime>
 #include <fstream>
 #include <iomanip>
@@ -307,11 +308,8 @@ class TransactionSecMarkerManagement {
     }
 
     std::shared_ptr<std::string> 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() {

src/rule_with_actions.cc

@@ -17,6 +17,7 @@
 
 #include <stdio.h>
 
+#include <cassert>
 #include <algorithm>
 #include <iostream>
 #include <string>
@@ -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<actions::Capture *>(a)) {
-                    m_containsCaptureAction = true;
-                    delete a;
-                } else if (dynamic_cast<actions::MultiMatch *>(a)) {
-                    m_containsMultiMatchAction = true;
+            switch (a->action_kind) {
+                case Action::ConfigurationKind:
+                    a->evaluate(this, NULL);
                     delete a;
-                } else if (dynamic_cast<actions::Severity *>(a)) {
-                    m_severity = dynamic_cast<actions::Severity *>(a);
-                } else if (dynamic_cast<actions::LogData *>(a)) {
-                    m_logData = dynamic_cast<actions::LogData*>(a);
-                } else if (dynamic_cast<actions::Msg *>(a)) {
-                    m_msg = dynamic_cast<actions::Msg*>(a);
-                } else if (dynamic_cast<actions::SetVar *>(a)) {
-                    m_actionsSetVar.push_back(
-                        dynamic_cast<actions::SetVar *>(a));
-                } else if (dynamic_cast<actions::Tag *>(a)) {
-                    m_actionsTag.push_back(dynamic_cast<actions::Tag *>(a));
-                } else if (dynamic_cast<actions::Block *>(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<actions::Capture *>(a)) {
+                        m_containsCaptureAction = true;
+                        delete a;
+                    } else if (dynamic_cast<actions::MultiMatch *>(a)) {
+                        m_containsMultiMatchAction = true;
+                        delete a;
+                    } else if (dynamic_cast<actions::Severity *>(a)) {
+                        m_severity = dynamic_cast<actions::Severity *>(a);
+                    } else if (dynamic_cast<actions::LogData *>(a)) {
+                        m_logData = dynamic_cast<actions::LogData*>(a);
+                    } else if (dynamic_cast<actions::Msg *>(a)) {
+                        m_msg = dynamic_cast<actions::Msg*>(a);
+                    } else if (dynamic_cast<actions::SetVar *>(a)) {
+                        m_actionsSetVar.push_back(
+                            dynamic_cast<actions::SetVar *>(a));
+                    } else if (dynamic_cast<actions::Tag *>(a)) {
+                        m_actionsTag.push_back(dynamic_cast<actions::Tag *>(a));
+                    } else if (dynamic_cast<actions::Block *>(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> 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;
         }