Refactoring: new structure for logging alerts

Disruptive actions were moved to actions::disruptive namespace

headers/modsecurity/intervention.h

@@ -23,11 +23,47 @@ namespace modsecurity {
 typedef struct ModSecurityIntervention_t {
     int status;
     int pause;
-    const char *url;
+    char *url;
-    const char *log;
+    char *log;
     int disruptive;
 } ModSecurityIntervention;
 
+#ifdef __cplusplus
+namespace intervention {
+    static void reset(ModSecurityIntervention_t *i) {
+        i->status = 200;
+        i->pause = 0;
+        i->disruptive = 0;
+    }
+
+    static void clean(ModSecurityIntervention_t *i) {
+        i->url = NULL;
+        i->log = NULL;
+        reset(i);
+    }
+
+    static void freeUrl(ModSecurityIntervention_t *i) {
+        if (i->url) {
+            free(i->url);
+            i->url = NULL;
+        }
+    }
+
+    static void freeLog(ModSecurityIntervention_t *i) {
+        if (i->log) {
+            free(i->log);
+            i->log = NULL;
+        }
+    }
+
+    static void free(ModSecurityIntervention_t *i) {
+        freeUrl(i);
+        freeLog(i);
+    }
+
+}  // namespace modsecurity
+#endif
+
 #ifdef __cplusplus
 }  // namespace modsecurity
 #endif

src/actions/data/status.h

@@ -18,8 +18,8 @@
 #include "modsecurity/actions/action.h"
 #include "modsecurity/rule_message.h"
 
-#ifndef SRC_ACTIONS_STATUS_H_
+#ifndef SRC_ACTIONS_DATA_STATUS_H_
-#define SRC_ACTIONS_STATUS_H_
+#define SRC_ACTIONS_DATA_STATUS_H_
 
 #ifdef __cplusplus
 class Transaction;
@@ -48,4 +48,4 @@ class Status : public Action {
 }  // namespace modsecurity
 #endif
 
-#endif  // SRC_ACTIONS_STATUS_H_
+#endif  // SRC_ACTIONS_DATA_STATUS_H_

src/actions/disruptive/allow.h

@@ -17,8 +17,8 @@
 
 #include "modsecurity/actions/action.h"
 
-#ifndef SRC_ACTIONS_ALLOW_H_
+#ifndef SRC_ACTIONS_DISRUPTIVE_ALLOW_H_
-#define SRC_ACTIONS_ALLOW_H_
+#define SRC_ACTIONS_DISRUPTIVE_ALLOW_H_
 
 #ifdef __cplusplus
 class Transaction;
@@ -84,4 +84,4 @@ class Allow : public Action {
 }  // namespace modsecurity
 #endif
 
-#endif  // SRC_ACTIONS_ALLOW_H_
+#endif  // SRC_ACTIONS_DISRUPTIVE_ALLOW_H_

src/actions/disruptive/block.cc

@@ -30,8 +30,6 @@ namespace disruptive {
 
 
 bool Block::evaluate(Rule *rule, Transaction *transaction, RuleMessage *rm) {
-    std::string log;
-
     transaction->debug(8, "Marking request as disruptive.");
 
     for (Action *a : transaction->m_rules->defaultActions[rule->phase]) {

src/actions/disruptive/block.h

@@ -18,8 +18,8 @@
 #include "modsecurity/actions/action.h"
 #include "modsecurity/rule_message.h"
 
-#ifndef SRC_ACTIONS_BLOCK_H_
+#ifndef SRC_ACTIONS_DISRUPTIVE_BLOCK_H_
-#define SRC_ACTIONS_BLOCK_H_
+#define SRC_ACTIONS_DISRUPTIVE_BLOCK_H_
 
 #ifdef __cplusplus
 class Transaction;
@@ -46,4 +46,4 @@ class Block : public Action {
 }  // namespace modsecurity
 #endif
 
-#endif  // SRC_ACTIONS_BLOCK_H_
+#endif  // SRC_ACTIONS_DISRUPTIVE_BLOCK_H_

src/actions/disruptive/deny.cc

@@ -15,10 +15,10 @@
 
 #include "src/actions/disruptive/deny.h"
 
+#include <string.h>
 #include <iostream>
 #include <string>
 #include <cstring>
-#include <string.h>
 
 #include "modsecurity/transaction.h"
 
@@ -42,7 +42,9 @@ bool Deny::evaluate(Rule *rule, Transaction *transaction, RuleMessage *rm) {
     log.append(std::to_string(rm->m_rule->phase - 1) + "). ");
 
     transaction->m_it.disruptive = true;
-    transaction->m_it.log = strdup(rm->disruptiveErrorLog(transaction, log).c_str());
+    intervention::freeLog(&transaction->m_it);
+    transaction->m_it.log = strdup(
+        rm->disruptiveErrorLog(transaction, log).c_str());
 
     return true;
 }

src/actions/disruptive/deny.h

@@ -19,8 +19,8 @@
 #include "modsecurity/transaction.h"
 #include "modsecurity/rule_message.h"
 
-#ifndef SRC_ACTIONS_DENY_H_
+#ifndef SRC_ACTIONS_DISRUPTIVE_DENY_H_
-#define SRC_ACTIONS_DENY_H_
+#define SRC_ACTIONS_DISRUPTIVE_DENY_H_
 
 namespace modsecurity {
 namespace actions {
@@ -41,4 +41,4 @@ class Deny : public Action {
 }  // namespace actions
 }  // namespace modsecurity
 
-#endif  // SRC_ACTIONS_DENY_H_
+#endif  // SRC_ACTIONS_DISRUPTIVE_DENY_H_

src/actions/disruptive/pass.cc

@@ -28,11 +28,8 @@ namespace disruptive {
 
 
 bool Pass::evaluate(Rule *rule, Transaction *transaction, RuleMessage *rm) {
-    transaction->m_it.status = 200;
+    intervention::free(&transaction->m_it);
-    transaction->m_it.disruptive = false;
+    intervention::reset(&transaction->m_it);
-    transaction->m_it.url = NULL;
-    transaction->m_it.log = NULL;
-    transaction->m_it.pause = 0;
 
     transaction->debug(8, "Running action pass");
 

src/actions/disruptive/redirect.cc

@@ -15,9 +15,9 @@
 
 #include "src/actions/disruptive/redirect.h"
 
+#include <string.h>
 #include <iostream>
 #include <string>
-#include <string.h>
 
 
 #include "modsecurity/transaction.h"
@@ -35,7 +35,8 @@ bool Redirect::init(std::string *error) {
 }
 
 
-bool Redirect::evaluate(Rule *rule, Transaction *transaction, RuleMessage *rm) {
+bool Redirect::evaluate(Rule *rule, Transaction *transaction,
+    RuleMessage *rm) {
     m_urlExpanded = MacroExpansion::expand(m_url, transaction);
     std::string log;
 
@@ -47,9 +48,12 @@ bool Redirect::evaluate(Rule *rule, Transaction *transaction, RuleMessage *rm) {
     log.append(" (phase ");
     log.append(std::to_string(rm->m_rule->phase - 1) + "). ");
 
+    intervention::freeUrl(&transaction->m_it);
     transaction->m_it.url = strdup(m_urlExpanded.c_str());
     transaction->m_it.disruptive = true;
-    transaction->m_it.log = strdup(rm->disruptiveErrorLog(transaction, log).c_str());
+    intervention::freeLog(&transaction->m_it);
+    transaction->m_it.log = strdup(
+        rm->disruptiveErrorLog(transaction, log).c_str());
 
     return true;
 }

src/actions/disruptive/redirect.h

@@ -18,8 +18,8 @@
 #include "modsecurity/actions/action.h"
 #include "modsecurity/rule_message.h"
 
-#ifndef SRC_ACTIONS_REDIRECT_H_
+#ifndef SRC_ACTIONS_DISRUPTIVE_REDIRECT_H_
-#define SRC_ACTIONS_REDIRECT_H_
+#define SRC_ACTIONS_DISRUPTIVE_REDIRECT_H_
 
 #ifdef __cplusplus
 class Transaction;
@@ -39,7 +39,8 @@ class Redirect : public Action {
         m_urlExpanded(""),
         m_url("") { }
 
-    bool evaluate(Rule *rule, Transaction *transaction, RuleMessage *rm) override;
+    bool evaluate(Rule *rule, Transaction *transaction, RuleMessage *rm)
+        override;
     bool init(std::string *error) override;
     bool isDisruptive() override { return true; }
 
@@ -55,4 +56,4 @@ class Redirect : public Action {
 }  // namespace modsecurity
 #endif
 
-#endif  // SRC_ACTIONS_REDIRECT_H_
+#endif  // SRC_ACTIONS_DISRUPTIVE_REDIRECT_H_

src/rule.cc

@@ -587,7 +587,7 @@ bool Rule::evaluate(Transaction *trasn) {
     if (globalRet == false) {
         trasn->debug(4, "Rule returned 0.");
         cleanMatchedVars(trasn);
-        return false;
+        goto end_clean;
     }
 
     trasn->debug(4, "Rule returned 1.");
@@ -599,7 +599,7 @@ bool Rule::evaluate(Transaction *trasn) {
     if (this->chainedRule == NULL) {
         trasn->debug(4, "Rule is marked as chained but there " \
             "isn't a subsequent rule.");
-        return false;
+        goto end_clean;
     }
 
     trasn->debug(4, "Executing chained rule.");
@@ -609,6 +609,13 @@ bool Rule::evaluate(Transaction *trasn) {
         goto end_exec;
     }
 
+end_clean:
+    while (finalVars.empty() == false) {
+        auto *a = finalVars.back();
+        finalVars.pop_back();
+        delete a;
+    }
+
     return false;
 
 end_exec:
@@ -617,6 +624,12 @@ end_exec:
         trasn->serverLog(u);
     }
 
+    while (finalVars.empty() == false) {
+        auto *a = finalVars.back();
+        finalVars.pop_back();
+        delete a;
+    }
+
     return true;
 }
 

src/rule_message.cc

@@ -23,7 +23,8 @@
 
 namespace modsecurity {
 
-std::string RuleMessage::disruptiveErrorLog(Transaction *trans, std::string msg2) {
+std::string RuleMessage::disruptiveErrorLog(Transaction *trans,
+    std::string msg2) {
     std::string msg;
 
     msg.append("[client " + std::string(trans->m_clientIpAddress) + "]");
@@ -50,7 +51,6 @@ std::string RuleMessage::disruptiveErrorLog(Transaction *trans, std::string msg2
     msg.append(" [unique_id \"" + trans->m_id + "\"]");
 
     return modsecurity::utils::string::toHexIfNeeded(msg);
-
 }
 
 std::string RuleMessage::errorLog(Transaction *trans) {

src/transaction.cc

@@ -161,11 +161,7 @@ Transaction::Transaction(ModSecurity *ms, Rules *rules, void *logCbData)
     this->debug(4, "Initializing transaction");
 #endif
 
-    m_it.status = 200;
+    intervention::clean(&m_it);
-    m_it.disruptive = false;
-    m_it.url = NULL;
-    m_it.log = NULL;
-    m_it.pause = 0;
 }
 
 
@@ -183,6 +179,9 @@ Transaction::~Transaction() {
 
     m_rules->decrementReferenceCount();
 
+    intervention::free(&m_it);
+    intervention::clean(&m_it);
+
     delete m_json;
     delete m_xml;
 }
@@ -885,7 +884,8 @@ int Transaction::appendRequestBody(const unsigned char *buf, size_t len) {
                 debug(5, "Request body limit is marked to reject the " \
                     "request");
 #endif
-                m_it.log = "Request body limit is marked to reject the request";
+                intervention::free(&m_it);
+                m_it.log = strdup("Request body limit is marked to reject the request");
                 m_it.status = 403;
                 m_it.disruptive = true;
             }
@@ -1142,8 +1142,9 @@ int Transaction::appendResponseBody(const unsigned char *buf, size_t len) {
                 debug(5, "Response body limit is marked to reject the " \
                     "request");
 #endif
-
+                intervention::free(&m_it);
-                m_it.log = "Response body limit is marked to reject the request";
+                m_it.log = strdup("Response body limit is marked to reject " \
+                    "the request");
                 m_it.status = 403;
                 m_it.disruptive = true;
             }
@@ -1299,17 +1300,12 @@ bool Transaction::intervention(ModSecurityIntervention *it) {
 
         if (m_it.log != NULL) {
             std::string log("");
-            const char *log_str;
             log.append(m_it.log);
-            utils::string::replaceAll(&log, std::string("%d"), std::to_string(it->status));
+            utils::string::replaceAll(&log, std::string("%d"),
-            log_str = strdup(log.c_str());
+                std::to_string(it->status));
-            it->log = log_str;
+            it->log = strdup(log.c_str());
         }
-        m_it.status = 200;
+        intervention::reset(&m_it);
-        m_it.disruptive = false;
-        m_it.url = NULL;
-        m_it.log = NULL;
-        m_it.pause = 0;
     }
 
     return it->disruptive;

src/utils/string.cc

@@ -210,7 +210,6 @@ void replaceAll(std::string *str, const std::string& from,
     const std::string& to) {
     size_t start_pos = 0;
     while ((start_pos = str->find(from, start_pos)) != std::string::npos) {
-        size_t end_pos = start_pos + from.length();
         str->replace(start_pos, from.length(), to);
         start_pos += to.length();
     }