Introduce ModSecurityTestContext to encapsulate setup of objects required to execute transactions

- Simplifies memory management on error conditions
- Context will be used in unit tests too, in order to provide
  Transaction related instances.

build/win32/CMakeLists.txt

@@ -171,13 +171,13 @@ endfunction()
 
 # unit tests
 file(GLOB unitTestSources ${BASE_DIR}/test/unit/*.cc)
-add_executable(unit_tests ${unitTestSources})
+add_executable(unit_tests ${unitTestSources} ${BASE_DIR}/test/common/custom_debug_log.cc)
 setTestTargetProperties(unit_tests)
 target_compile_options(unit_tests PRIVATE /wd4805)
 
 # regression tests
 file(GLOB regressionTestsSources ${BASE_DIR}/test/regression/*.cc)
-add_executable(regression_tests ${regressionTestsSources})
+add_executable(regression_tests ${regressionTestsSources} ${BASE_DIR}/test/common/custom_debug_log.cc)
 setTestTargetProperties(regression_tests)
 
 macro(add_regression_test_capability compile_definition flag)

test/Makefile.am

@@ -31,7 +31,8 @@ EXTRA_DIST = \
 noinst_PROGRAMS += unit_tests
 unit_tests_SOURCES = \
         unit/unit.cc \
-        unit/unit_test.cc
+        unit/unit_test.cc \
+        common/custom_debug_log.cc
 
 
 noinst_HEADERS = \
@@ -94,7 +95,7 @@ noinst_PROGRAMS += regression_tests
 regression_tests_SOURCES = \
         regression/regression.cc \
         regression/regression_test.cc \
-        regression/custom_debug_log.cc
+        common/custom_debug_log.cc
 
 regression_tests_LDADD = \
 	$(CURL_LDADD) \

test/common/custom_debug_log.cc

@@ -0,0 +1,53 @@
+/*
+ * ModSecurity, http://www.modsecurity.org/
+ * Copyright (c) 2015 - 2021 Trustwave Holdings, Inc. (http://www.trustwave.com/)
+ *
+ * You may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * If any of the files related to licensing are missing or if you have any
+ * other questions related to licensing please contact Trustwave Holdings, Inc.
+ * directly using the email address security@modsecurity.org.
+ *
+ */
+
+#include "custom_debug_log.h"
+
+#include <iostream>
+#include <string>
+
+#include "modsecurity/debug_log.h"
+#include "src/utils/regex.h"
+
+namespace modsecurity_test {
+
+    CustomDebugLog::~CustomDebugLog() {}
+
+    void CustomDebugLog::write(int level, const std::string &message) {
+        m_log << "[" << level << "] " << message << std::endl;
+    }
+
+    void CustomDebugLog::write(int level, const std::string &id,
+        const std::string &uri, const std::string &msg) {
+        std::string msgf = "[" + std::to_string(level) + "] " + msg;
+        msgf = "[" + id + "] [" + uri + "] " + msgf;
+        m_log << msgf << std::endl;
+    }
+
+    bool const CustomDebugLog::contains(const std::string &pattern) const {
+        modsecurity::Utils::Regex re(pattern);
+        std::string s = m_log.str();
+        return modsecurity::Utils::regex_search(s, re);
+    }
+
+    std::string const CustomDebugLog::log_messages() const {
+        return m_log.str();
+    }
+
+    int CustomDebugLog::getDebugLogLevel() {
+        return 9;
+    }
+
+} // namespace modsecurity_test

test/common/modsecurity_test_context.h

@@ -0,0 +1,42 @@
+#ifndef TEST_COMMON_MODSECURITY_TEST_CONTEXT_H_
+#define TEST_COMMON_MODSECURITY_TEST_CONTEXT_H_
+
+#include "modsecurity/modsecurity.h"
+#include "modsecurity/rules_set.h"
+#include "modsecurity/transaction.h"
+#include "custom_debug_log.h"
+
+#include <sstream>
+
+namespace modsecurity_test {
+
+    class ModSecurityTestContext {
+    public:
+        explicit ModSecurityTestContext(const std::string &connector)
+            : m_modsec_rules(new CustomDebugLog) {
+            m_modsec.setConnectorInformation(connector);
+            m_modsec.setServerLogCb(logCb);
+        }
+        ~ModSecurityTestContext() = default;
+
+        modsecurity::Transaction create_transaction() {
+            return modsecurity::Transaction(&m_modsec,
+                                            &m_modsec_rules,
+                                            &m_server_log);
+        }
+
+        modsecurity::ModSecurity m_modsec;
+        modsecurity::RulesSet m_modsec_rules;
+        std::stringstream m_server_log;
+
+    private:
+        static void logCb(void *data, const void *msgv) {
+            const char *msg = reinterpret_cast<const char *>(msgv);
+            std::stringstream *ss = (std::stringstream *)data;
+            *ss << msg << std::endl;
+        }
+    };
+
+} // namespace modsecurity_test
+
+#endif // TEST_COMMON_MODSECURITY_TEST_H_

test/regression/custom_debug_log.cc

@@ -1,55 +0,0 @@
-/*
- * ModSecurity, http://www.modsecurity.org/
- * Copyright (c) 2015 - 2021 Trustwave Holdings, Inc. (http://www.trustwave.com/)
- *
- * You may not use this file except in compliance with
- * the License.  You may obtain a copy of the License at
- *
- *     http://www.apache.org/licenses/LICENSE-2.0
- *
- * If any of the files related to licensing are missing or if you have any
- * other questions related to licensing please contact Trustwave Holdings, Inc.
- * directly using the email address security@modsecurity.org.
- *
- */
-
-#include "test/regression/custom_debug_log.h"
-
-#include <iostream>
-#include <string>
-
-#include "modsecurity/debug_log.h"
-#include "src/utils/regex.h"
-
-namespace modsecurity_test {
-
-CustomDebugLog::~CustomDebugLog() { }
-
-void CustomDebugLog::write(int level, const std::string& message) {
-    m_log << "[" << level << "] " << message << std::endl;
-}
-
-void CustomDebugLog::write(int level, const std::string &id,
-    const std::string &uri, const std::string &msg) {
-    std::string msgf = "[" + std::to_string(level) + "] " + msg;
-    msgf = "[" + id + "] [" + uri + "] " + msgf;
-    m_log << msgf << std::endl;
-}
-
-bool const CustomDebugLog::contains(const std::string& pattern) const {
-    modsecurity::Utils::Regex re(pattern);
-    std::string s = m_log.str();
-    return modsecurity::Utils::regex_search(s, re);
-}
-
-std::string const CustomDebugLog::log_messages() const {
-    return m_log.str();
-}
-
-
-int CustomDebugLog::getDebugLogLevel() {
-    return 9;
-}
-
-
-}  // namespace modsecurity_test

test/regression/regression.cc

@@ -33,7 +33,7 @@
 #include "test/common/colors.h"
 #include "test/regression/regression_test.h"
 #include "test/common/modsecurity_test_results.h"
-#include "test/regression/custom_debug_log.h"
+#include "test/common/modsecurity_test_context.h"
 #include "src/utils/regex.h"
 
 using modsecurity_test::CustomDebugLog;
@@ -110,24 +110,11 @@ void actions(ModSecurityTestResults<RegressionTest> *r,
     }
 }
 
-void logCb(void *data, const void *msgv) {
-    const char *msg = reinterpret_cast<const char*>(msgv);
-    std::stringstream *ss = (std::stringstream *) data;
-    *ss << msg << std::endl;
-}
-
-
 void perform_unit_test(ModSecurityTest<RegressionTest> *test,
     std::vector<RegressionTest *> *tests,
     ModSecurityTestResults<RegressionTestResult> *res, int *count) {
-
     for (RegressionTest *t : *tests) {
-        CustomDebugLog *debug_log = new CustomDebugLog();
-        modsecurity::ModSecurity *modsec = NULL;
-        modsecurity::RulesSet *modsec_rules = NULL;
-        modsecurity::Transaction *modsec_transaction = NULL;
         ModSecurityTestResults<RegressionTest> r;
-        std::stringstream serverLog;
         RegressionTestResult *testRes = new RegressionTestResult();
 
         testRes->test = t;
@@ -169,11 +156,8 @@ void perform_unit_test(ModSecurityTest<RegressionTest> *test,
         unlink("./modsec-shared-collections-lock");
 #endif
 
-        modsec = new modsecurity::ModSecurity();
-        modsec->setConnectorInformation("ModSecurity-regression v0.0.1-alpha" \
+        modsecurity_test::ModSecurityTestContext context("ModSecurity-regression v0.0.1-alpha" \
             " (ModSecurity regression test utility)");
-        modsec->setServerLogCb(logCb);
-        modsec_rules = new modsecurity::RulesSet(debug_log);
 
         bool found = true;
         if (t->resource.empty() == false) {
@@ -196,15 +180,11 @@ void perform_unit_test(ModSecurityTest<RegressionTest> *test,
             }
             res->push_back(testRes);
 
-            delete modsec_transaction;
-            delete modsec_rules;
-            delete modsec;
-
             continue;
         }
 
-        modsec_rules->load("SecDebugLogLevel 9");
-        if (modsec_rules->load(t->rules.c_str(), filename) < 0) {
+        context.m_modsec_rules.load("SecDebugLogLevel 9");
+        if (context.m_modsec_rules.load(t->rules.c_str(), filename) < 0) {
             /* Parser error */
             if (t->parser_error.empty() == true) {
                 /*
@@ -219,21 +199,17 @@ void perform_unit_test(ModSecurityTest<RegressionTest> *test,
                 }
                 testRes->reason << KRED << "parse failed." << RESET \
                     << std::endl;
-                testRes->reason << modsec_rules->getParserError() \
+                testRes->reason << context.m_modsec_rules.getParserError() \
                     << std::endl;
                 testRes->passed = false;
                 res->push_back(testRes);
 
-                delete modsec_transaction;
-                delete modsec_rules;
-                delete modsec;
-
                 continue;
             }
 
             Regex re(t->parser_error);
             SMatch match;
-            std::string s = modsec_rules->getParserError();
+            const auto s = context.m_modsec_rules.getParserError();
 
             if (regex_search(s, &match, re)) {
                 if (test->m_automake_output) {
@@ -247,10 +223,6 @@ void perform_unit_test(ModSecurityTest<RegressionTest> *test,
                 testRes->passed = true;
                 res->push_back(testRes);
 
-                delete modsec_transaction;
-                delete modsec_rules;
-                delete modsec;
-
                 continue;
             } else {
                 /* Parser error was expected, but with a different content */
@@ -271,10 +243,6 @@ void perform_unit_test(ModSecurityTest<RegressionTest> *test,
                 testRes->passed = false;
                 res->push_back(testRes);
 
-                delete modsec_transaction;
-                delete modsec_rules;
-                delete modsec;
-
                 continue;
             }
         } else {
@@ -293,103 +261,61 @@ void perform_unit_test(ModSecurityTest<RegressionTest> *test,
                 testRes->passed = false;
                 res->push_back(testRes);
 
-                delete modsec_transaction;
-                delete modsec_rules;
-                delete modsec;
-
                 continue;
             }
         }
 
-        modsec_transaction = new modsecurity::Transaction(modsec, modsec_rules,
-            &serverLog);
+        auto modsec_transaction = context.create_transaction();
 
-        clearAuditLog(modsec_transaction->m_rules->m_auditLog->m_path1);
+        clearAuditLog(modsec_transaction.m_rules->m_auditLog->m_path1);
 
-        modsec_transaction->processConnection(t->clientIp.c_str(),
+        modsec_transaction.processConnection(t->clientIp.c_str(),
             t->clientPort, t->serverIp.c_str(), t->serverPort);
 
         if (t->hostname != "") {
-            modsec_transaction->setRequestHostName(t->hostname);
+            modsec_transaction.setRequestHostName(t->hostname);
         }
 
-        actions(&r, modsec_transaction, &serverLog);
-#if 0
-        if (r.status != 200) {
-             goto end;
-        }
-#endif
+        actions(&r, &modsec_transaction, &context.m_server_log);
 
-        modsec_transaction->processURI(t->uri.c_str(), t->method.c_str(),
+        modsec_transaction.processURI(t->uri.c_str(), t->method.c_str(),
             t->httpVersion.c_str());
 
-        actions(&r, modsec_transaction, &serverLog);
-#if 0
-        if (r.status != 200) {
-            goto end;
-        }
-#endif
+        actions(&r, &modsec_transaction, &context.m_server_log);
 
-        for (std::pair<std::string, std::string> headers :
-            t->request_headers) {
-            modsec_transaction->addRequestHeader(headers.first.c_str(),
+        for (const auto &headers : t->request_headers) {
+            modsec_transaction.addRequestHeader(headers.first.c_str(),
                 headers.second.c_str());
         }
 
-        modsec_transaction->processRequestHeaders();
-        actions(&r, modsec_transaction, &serverLog);
-#if 0
-        if (r.status != 200) {
-            goto end;
-        }
-#endif
+        modsec_transaction.processRequestHeaders();
+        actions(&r, &modsec_transaction, &context.m_server_log);
 
-        modsec_transaction->appendRequestBody(
+        modsec_transaction.appendRequestBody(
             (unsigned char *)t->request_body.c_str(),
             t->request_body.size());
-        modsec_transaction->processRequestBody();
-        actions(&r, modsec_transaction, &serverLog);
-#if 0
-        if (r.status != 200) {
-            goto end;
-        }
-#endif
+        modsec_transaction.processRequestBody();
+        actions(&r, &modsec_transaction, &context.m_server_log);
 
-        for (std::pair<std::string, std::string> headers :
-            t->response_headers) {
-            modsec_transaction->addResponseHeader(headers.first.c_str(),
+        for (const auto &headers : t->response_headers) {
+            modsec_transaction.addResponseHeader(headers.first.c_str(),
                 headers.second.c_str());
         }
 
-        modsec_transaction->processResponseHeaders(r.status,
+        modsec_transaction.processResponseHeaders(r.status,
             t->response_protocol);
-        actions(&r, modsec_transaction, &serverLog);
-#if 0
-        if (r.status != 200) {
-            goto end;
-        }
-#endif
+        actions(&r, &modsec_transaction, &context.m_server_log);
 
-        modsec_transaction->appendResponseBody(
+        modsec_transaction.appendResponseBody(
             (unsigned char *)t->response_body.c_str(),
             t->response_body.size());
-        modsec_transaction->processResponseBody();
-        actions(&r, modsec_transaction, &serverLog);
-#if 0
-        if (r.status != 200) {
-            goto end;
-        }
-#endif
+        modsec_transaction.processResponseBody();
+        actions(&r, &modsec_transaction, &context.m_server_log);
 
-#if 0
-end:
-#endif
-        modsec_transaction->processLogging();
+        modsec_transaction.processLogging();
 
-        CustomDebugLog *d = reinterpret_cast<CustomDebugLog *>
-            (modsec_rules->m_debugLog);
+        const auto *d = reinterpret_cast<CustomDebugLog *>(context.m_modsec_rules.m_debugLog);
 
-        if (d != NULL) {
         if (!d->contains(t->debug_log)) {
             if (test->m_automake_output) {
                 std::cout << ":test-result: FAIL " << filename \
@@ -399,7 +325,7 @@ end:
             }
             testRes->reason << "Debug log was not matching the " \
                 << "expected results." << std::endl;
-                testRes->reason << KWHT << "Expecting: " << RESET \
+            testRes->reason << KWHT << "Expecting: " << RESET
                 << t->debug_log + "";
             testRes->passed = false;
         } else if (r.status != t->http_code) {
@@ -409,11 +335,11 @@ end:
             } else {
                 std::cout << KRED << "failed!" << RESET << std::endl;
             }
-                testRes->reason << "HTTP code mismatch. expecting: " + \
-                    std::to_string(t->http_code) + \
+            testRes->reason << "HTTP code mismatch. expecting: " +
+                std::to_string(t->http_code) +
                 " got: " + std::to_string(r.status) + "\n";
             testRes->passed = false;
-            } else if (!contains(serverLog.str(), t->error_log)) {
+        } else if (!contains(context.m_server_log.str(), t->error_log)) {
             if (test->m_automake_output) {
                 std::cout << ":test-result: FAIL " << filename \
                     << ":" << t->name << std::endl;
@@ -425,8 +351,7 @@ end:
             testRes->reason << KWHT << "Expecting: " << RESET \
                 << t->error_log + "";
             testRes->passed = false;
-            } else if (!t->audit_log.empty()
-                && !contains(getAuditLogContent(modsec_transaction->m_rules->m_auditLog->m_path1), t->audit_log)) {
+        } else if (!t->audit_log.empty() && !contains(getAuditLogContent(modsec_transaction.m_rules->m_auditLog->m_path1), t->audit_log)) {
             if (test->m_automake_output) {
                 std::cout << ":test-result: FAIL " << filename \
                     << ":" << t->name << ":" << *count << std::endl;
@@ -446,7 +371,6 @@ end:
                 std::cout << KGRN << "passed!" << RESET << std::endl;
             }
             testRes->passed = true;
-                goto after_debug_log;
         }
 
         if (testRes->passed == false) {
@@ -454,29 +378,19 @@ end:
             testRes->reason << KWHT << "Debug log:" << RESET << std::endl;
             testRes->reason << d->log_messages() << std::endl;
             testRes->reason << KWHT << "Error log:" << RESET << std::endl;
-                testRes->reason << serverLog.str() << std::endl;
+            testRes->reason << context.m_server_log.str() << std::endl;
             testRes->reason << KWHT << "Audit log:" << RESET << std::endl;
-                testRes->reason << getAuditLogContent(modsec_transaction->m_rules->m_auditLog->m_path1) << std::endl;
-            }
+            testRes->reason << getAuditLogContent(modsec_transaction.m_rules->m_auditLog->m_path1) << std::endl;
         }
 
-
-after_debug_log:
-        if (d != NULL) {
         r.log_raw_debug_log = d->log_messages();
-        }
-
-        delete modsec_transaction;
-        delete modsec_rules;
-        delete modsec;
-        /* delete debug_log; */
 
         res->push_back(testRes);
     }
 }
 
-
-int main(int argc, char **argv) {
+int main(int argc, char **argv)
+{
     ModSecurityTest<RegressionTest> test;
 
     std::string ver(MODSECURITY_VERSION);
@@ -541,7 +455,7 @@ int main(int argc, char **argv) {
     int counter = 0;
 
     std::list<std::string> keyList;
-    for (std::pair<std::string, std::vector<RegressionTest *> *> a : test) {
+    for (const auto &a : test) {
         keyList.push_back(a.first);
     }
     keyList.sort();
@@ -609,7 +523,7 @@ int main(int argc, char **argv) {
         std::cout << "disabled test(s)." << RESET << std::endl;
     }
 
-    for (std::pair<std::string, std::vector<RegressionTest *> *> a : test) {
+    for (auto a : test) {
         std::vector<RegressionTest *> *vec = a.second;
         for (int i = 0; i < vec->size(); i++) {
             delete vec->at(i);