From e313ac7de75d84b491066857d381891439524abc Mon Sep 17 00:00:00 2001 From: eduar-hte <130087371+eduar-hte@users.noreply.github.com> Date: Tue, 7 May 2024 01:29:16 +0000 Subject: [PATCH 1/4] 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 | 4 +- test/Makefile.am | 5 +- test/common/custom_debug_log.cc | 53 ++++ .../{regression => common}/custom_debug_log.h | 0 test/common/modsecurity_test_context.h | 42 +++ test/regression/custom_debug_log.cc | 55 ---- test/regression/regression.cc | 278 ++++++------------ 7 files changed, 196 insertions(+), 241 deletions(-) create mode 100644 test/common/custom_debug_log.cc rename test/{regression => common}/custom_debug_log.h (100%) create mode 100644 test/common/modsecurity_test_context.h delete mode 100644 test/regression/custom_debug_log.cc diff --git a/build/win32/CMakeLists.txt b/build/win32/CMakeLists.txt index cebf4c9be2..fbf39f08d9 100644 --- a/build/win32/CMakeLists.txt +++ b/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) diff --git a/test/Makefile.am b/test/Makefile.am index 4d4396ae82..2c1ba02a49 100644 --- a/test/Makefile.am +++ b/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) \ diff --git a/test/common/custom_debug_log.cc b/test/common/custom_debug_log.cc new file mode 100644 index 0000000000..7f69b58c1c --- /dev/null +++ b/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 +#include + +#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 diff --git a/test/regression/custom_debug_log.h b/test/common/custom_debug_log.h similarity index 100% rename from test/regression/custom_debug_log.h rename to test/common/custom_debug_log.h diff --git a/test/common/modsecurity_test_context.h b/test/common/modsecurity_test_context.h new file mode 100644 index 0000000000..46461cb2bd --- /dev/null +++ b/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 + +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(msgv); + std::stringstream *ss = (std::stringstream *)data; + *ss << msg << std::endl; + } + }; + +} // namespace modsecurity_test + +#endif // TEST_COMMON_MODSECURITY_TEST_H_ diff --git a/test/regression/custom_debug_log.cc b/test/regression/custom_debug_log.cc deleted file mode 100644 index 1e7de0e02a..0000000000 --- a/test/regression/custom_debug_log.cc +++ /dev/null @@ -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 -#include - -#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 diff --git a/test/regression/regression.cc b/test/regression/regression.cc index a6ede61c72..5992d2cf47 100644 --- a/test/regression/regression.cc +++ b/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 *r, } } -void logCb(void *data, const void *msgv) { - const char *msg = reinterpret_cast(msgv); - std::stringstream *ss = (std::stringstream *) data; - *ss << msg << std::endl; -} - - void perform_unit_test(ModSecurityTest *test, std::vector *tests, ModSecurityTestResults *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 r; - std::stringstream serverLog; RegressionTestResult *testRes = new RegressionTestResult(); testRes->test = t; @@ -169,11 +156,8 @@ void perform_unit_test(ModSecurityTest *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 *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 *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 *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 *test, testRes->passed = false; res->push_back(testRes); - delete modsec_transaction; - delete modsec_rules; - delete modsec; - continue; } } else { @@ -293,190 +261,136 @@ void perform_unit_test(ModSecurityTest *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 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 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 - (modsec_rules->m_debugLog); + const auto *d = reinterpret_cast(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 \ - << ":" << t->name << ":" << *count << std::endl; - } else { - std::cout << KRED << "failed!" << RESET << std::endl; - } - testRes->reason << "Debug log was not matching the " \ - << "expected results." << std::endl; - testRes->reason << KWHT << "Expecting: " << RESET \ - << t->debug_log + ""; - testRes->passed = false; - } else if (r.status != t->http_code) { - if (test->m_automake_output) { - std::cout << ":test-result: FAIL " << filename \ - << ":" << t->name << ":" << *count << std::endl; - } else { - std::cout << KRED << "failed!" << RESET << std::endl; - } - 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)) { - if (test->m_automake_output) { - std::cout << ":test-result: FAIL " << filename \ - << ":" << t->name << std::endl; - } else { - std::cout << KRED << "failed!" << RESET << std::endl; - } - testRes->reason << "Error log was not matching the " \ - << "expected results." << std::endl; - 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)) { - if (test->m_automake_output) { - std::cout << ":test-result: FAIL " << filename \ - << ":" << t->name << ":" << *count << std::endl; - } else { - std::cout << KRED << "failed!" << RESET << std::endl; - } - testRes->reason << "Audit log was not matching the " \ - << "expected results." << std::endl; - testRes->reason << KWHT << "Expecting: " << RESET \ - << t->audit_log + ""; - testRes->passed = false; + if (!d->contains(t->debug_log)) { + if (test->m_automake_output) { + std::cout << ":test-result: FAIL " << filename \ + << ":" << t->name << ":" << *count << std::endl; } else { - if (test->m_automake_output) { - std::cout << ":test-result: PASS " << filename \ - << ":" << t->name << std::endl; - } else { - std::cout << KGRN << "passed!" << RESET << std::endl; - } - testRes->passed = true; - goto after_debug_log; + std::cout << KRED << "failed!" << RESET << std::endl; } - - if (testRes->passed == false) { - testRes->reason << std::endl; - 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 << KWHT << "Audit log:" << RESET << std::endl; - testRes->reason << getAuditLogContent(modsec_transaction->m_rules->m_auditLog->m_path1) << std::endl; + testRes->reason << "Debug log was not matching the " \ + << "expected results." << std::endl; + testRes->reason << KWHT << "Expecting: " << RESET + << t->debug_log + ""; + testRes->passed = false; + } else if (r.status != t->http_code) { + if (test->m_automake_output) { + std::cout << ":test-result: FAIL " << filename \ + << ":" << t->name << ":" << *count << std::endl; + } else { + std::cout << KRED << "failed!" << RESET << std::endl; + } + 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(context.m_server_log.str(), t->error_log)) { + if (test->m_automake_output) { + std::cout << ":test-result: FAIL " << filename \ + << ":" << t->name << std::endl; + } else { + std::cout << KRED << "failed!" << RESET << std::endl; } + testRes->reason << "Error log was not matching the " \ + << "expected results." << std::endl; + 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)) { + if (test->m_automake_output) { + std::cout << ":test-result: FAIL " << filename \ + << ":" << t->name << ":" << *count << std::endl; + } else { + std::cout << KRED << "failed!" << RESET << std::endl; + } + testRes->reason << "Audit log was not matching the " \ + << "expected results." << std::endl; + testRes->reason << KWHT << "Expecting: " << RESET \ + << t->audit_log + ""; + testRes->passed = false; + } else { + if (test->m_automake_output) { + std::cout << ":test-result: PASS " << filename \ + << ":" << t->name << std::endl; + } else { + std::cout << KGRN << "passed!" << RESET << std::endl; + } + testRes->passed = true; } - -after_debug_log: - if (d != NULL) { - r.log_raw_debug_log = d->log_messages(); + if (testRes->passed == false) { + testRes->reason << std::endl; + 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 << 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; } - delete modsec_transaction; - delete modsec_rules; - delete modsec; - /* delete debug_log; */ + r.log_raw_debug_log = d->log_messages(); res->push_back(testRes); } } - -int main(int argc, char **argv) { +int main(int argc, char **argv) +{ ModSecurityTest test; std::string ver(MODSECURITY_VERSION); @@ -541,7 +455,7 @@ int main(int argc, char **argv) { int counter = 0; std::list keyList; - for (std::pair *> a : test) { + for (const auto &a : test) { keyList.push_back(a.first); } keyList.sort(); @@ -554,7 +468,7 @@ int main(int argc, char **argv) { ModSecurityTestResults res; for (const std::string &a : keyList) { test_number++; - if ((test.m_test_number == 0) + if ((test.m_test_number == 0) || (test_number == test.m_test_number)) { std::vector *tests = test[a]; perform_unit_test(&test, tests, &res, &counter); @@ -605,11 +519,11 @@ int main(int argc, char **argv) { } std::cout << KCYN << std::to_string(skipped) << " "; - std::cout << "skipped test(s). " << std::to_string(disabled) << " "; + std::cout << "skipped test(s). " << std::to_string(disabled) << " "; std::cout << "disabled test(s)." << RESET << std::endl; } - for (std::pair *> a : test) { + for (auto a : test) { std::vector *vec = a.second; for (int i = 0; i < vec->size(); i++) { delete vec->at(i); From 4df297b5960e40fc5fd1740cb2746cd672e0c78d Mon Sep 17 00:00:00 2001 From: eduar-hte <130087371+eduar-hte@users.noreply.github.com> Date: Mon, 6 May 2024 01:24:52 +0000 Subject: [PATCH 2/4] Avoid passing RuleMessage by std::shared_ptr and use a reference instead. - Avoids copying std::shared_ptr when lifetime of the RuleMessage is controlled by the caller. - The RuleMessage instance is created in RuleWithActions::evaluate and then used to call the overloaded version of this method that is specialized by subclasses. - Once the call to the overloaded method returns, the std::shared_ptr is destroyed as it's not stored by any of the callers, so it can be replaced with a stack variable and avoid paying the cost of copying the std::shared_ptr (and its control block that is guaranteed to be thread-safe and thus is not a straightforward pointer copy) - Introduced RuleMessage::reset because this is required by RuleWithActions::performLogging when it's not the 'last log', the rule has multimatch and it's to be logged. - The current version is creating allocating another instance of RuleMessage on the heap to copy the Rule & Transaction related state while all the other members in the RuleMessage are set to their default values. - The new version leverages the existent, unused and incomplete function 'clean' (renamed as 'reset') to do this on the current instance. - Notice that the current code preserves the value of m_saveMessage, so 'reset' provides an argument for the caller to control whether this member should be reinitialized. --- .../reading_logs_via_rule_message.h | 4 +- .../using_bodies_in_chunks/simple_request.cc | 4 +- headers/modsecurity/actions/action.h | 2 +- headers/modsecurity/modsecurity.h | 2 +- headers/modsecurity/rule.h | 3 +- headers/modsecurity/rule_marker.h | 3 +- headers/modsecurity/rule_message.h | 58 ++++++++++--------- headers/modsecurity/rule_unconditional.h | 2 +- headers/modsecurity/rule_with_actions.h | 10 ++-- headers/modsecurity/rule_with_operator.h | 5 +- headers/modsecurity/transaction.h | 2 +- src/actions/audit_log.cc | 7 +-- src/actions/audit_log.h | 3 +- src/actions/block.cc | 5 +- src/actions/block.h | 3 +- src/actions/data/status.cc | 2 +- src/actions/data/status.h | 3 +- src/actions/disruptive/deny.cc | 6 +- src/actions/disruptive/deny.h | 3 +- src/actions/disruptive/drop.cc | 6 +- src/actions/disruptive/drop.h | 3 +- src/actions/disruptive/pass.cc | 2 +- src/actions/disruptive/pass.h | 3 +- src/actions/disruptive/redirect.cc | 6 +- src/actions/disruptive/redirect.h | 3 +- src/actions/log.cc | 5 +- src/actions/log.h | 3 +- src/actions/log_data.cc | 5 +- src/actions/log_data.h | 3 +- src/actions/msg.cc | 7 +-- src/actions/msg.h | 3 +- src/actions/no_audit_log.cc | 7 +-- src/actions/no_audit_log.h | 3 +- src/actions/no_log.cc | 5 +- src/actions/no_log.h | 3 +- src/actions/severity.cc | 5 +- src/actions/severity.h | 3 +- src/actions/tag.cc | 5 +- src/actions/tag.h | 3 +- src/modsecurity.cc | 12 ++-- src/operators/begins_with.cc | 2 +- src/operators/begins_with.h | 2 +- src/operators/contains.cc | 2 +- src/operators/contains.h | 2 +- src/operators/contains_word.cc | 2 +- src/operators/contains_word.h | 2 +- src/operators/detect_sqli.cc | 2 +- src/operators/detect_sqli.h | 2 +- src/operators/detect_xss.cc | 2 +- src/operators/detect_xss.h | 2 +- src/operators/ends_with.cc | 2 +- src/operators/ends_with.h | 2 +- src/operators/operator.cc | 4 +- src/operators/operator.h | 14 ++--- src/operators/pm.cc | 2 +- src/operators/pm.h | 2 +- src/operators/rbl.cc | 2 +- src/operators/rbl.h | 2 +- src/operators/rx.cc | 2 +- src/operators/rx.h | 2 +- src/operators/rx_global.cc | 2 +- src/operators/rx_global.h | 2 +- src/operators/validate_byte_range.cc | 2 +- src/operators/validate_byte_range.h | 2 +- src/operators/validate_url_encoding.cc | 2 +- src/operators/validate_url_encoding.h | 2 +- src/operators/validate_utf8_encoding.cc | 2 +- src/operators/validate_utf8_encoding.h | 2 +- src/operators/verify_cc.cc | 2 +- src/operators/verify_cc.h | 2 +- src/operators/verify_cpf.cc | 2 +- src/operators/verify_cpf.h | 2 +- src/operators/verify_ssn.cc | 2 +- src/operators/verify_ssn.h | 2 +- src/operators/verify_svnr.cc | 2 +- src/operators/verify_svnr.h | 2 +- src/operators/within.cc | 2 +- src/operators/within.h | 2 +- src/rule_message.cc | 50 ++++++++-------- src/rule_script.cc | 2 +- src/rule_script.h | 3 +- src/rule_unconditional.cc | 2 +- src/rule_with_actions.cc | 39 ++++++------- src/rule_with_operator.cc | 10 ++-- src/transaction.cc | 2 +- test/unit/unit.cc | 38 +++++++----- 86 files changed, 217 insertions(+), 241 deletions(-) diff --git a/examples/reading_logs_via_rule_message/reading_logs_via_rule_message.h b/examples/reading_logs_via_rule_message/reading_logs_via_rule_message.h index c91a473bc3..f7f18804c9 100644 --- a/examples/reading_logs_via_rule_message/reading_logs_via_rule_message.h +++ b/examples/reading_logs_via_rule_message/reading_logs_via_rule_message.h @@ -160,13 +160,13 @@ class ReadingLogsViaRuleMessage { std::cout << std::endl; if (ruleMessage->m_isDisruptive) { std::cout << " * Disruptive action: "; - std::cout << modsecurity::RuleMessage::log(ruleMessage); + std::cout << modsecurity::RuleMessage::log(*ruleMessage); std::cout << std::endl; std::cout << " ** %d is meant to be informed by the webserver."; std::cout << std::endl; } else { std::cout << " * Match, but no disruptive action: "; - std::cout << modsecurity::RuleMessage::log(ruleMessage); + std::cout << modsecurity::RuleMessage::log(*ruleMessage); std::cout << std::endl; } } diff --git a/examples/using_bodies_in_chunks/simple_request.cc b/examples/using_bodies_in_chunks/simple_request.cc index b050f3cc42..ec1b963d52 100644 --- a/examples/using_bodies_in_chunks/simple_request.cc +++ b/examples/using_bodies_in_chunks/simple_request.cc @@ -81,13 +81,13 @@ static void logCb(void *data, const void *ruleMessagev) { std::cout << std::endl; if (ruleMessage->m_isDisruptive) { std::cout << " * Disruptive action: "; - std::cout << modsecurity::RuleMessage::log(ruleMessage); + std::cout << modsecurity::RuleMessage::log(*ruleMessage); std::cout << std::endl; std::cout << " ** %d is meant to be informed by the webserver."; std::cout << std::endl; } else { std::cout << " * Match, but no disruptive action: "; - std::cout << modsecurity::RuleMessage::log(ruleMessage); + std::cout << modsecurity::RuleMessage::log(*ruleMessage); std::cout << std::endl; } } diff --git a/headers/modsecurity/actions/action.h b/headers/modsecurity/actions/action.h index 41c24abf15..f0c2165cf5 100644 --- a/headers/modsecurity/actions/action.h +++ b/headers/modsecurity/actions/action.h @@ -89,7 +89,7 @@ class Action { virtual bool evaluate(RuleWithActions *rule, Transaction *transaction); virtual bool evaluate(RuleWithActions *rule, Transaction *transaction, - std::shared_ptr ruleMessage) { + RuleMessage &ruleMessage) { return evaluate(rule, transaction); } virtual bool init(std::string *error) { return true; } diff --git a/headers/modsecurity/modsecurity.h b/headers/modsecurity/modsecurity.h index a829a0e1ce..3a67e5bba8 100644 --- a/headers/modsecurity/modsecurity.h +++ b/headers/modsecurity/modsecurity.h @@ -292,7 +292,7 @@ class ModSecurity { */ void setServerLogCb(ModSecLogCb cb, int properties); - void serverLog(void *data, std::shared_ptr rm); + void serverLog(void *data, const RuleMessage &rm); const std::string& getConnectorInformation() const; diff --git a/headers/modsecurity/rule.h b/headers/modsecurity/rule.h index e73f73d38a..5178ab53ba 100644 --- a/headers/modsecurity/rule.h +++ b/headers/modsecurity/rule.h @@ -78,8 +78,7 @@ class Rule { virtual bool evaluate(Transaction *transaction) = 0; - virtual bool evaluate(Transaction *transaction, - std::shared_ptr rm) = 0; + virtual bool evaluate(Transaction *transaction, RuleMessage &ruleMessage) = 0; const std::string& getFileName() const { return m_fileName; diff --git a/headers/modsecurity/rule_marker.h b/headers/modsecurity/rule_marker.h index 1df6615c3e..bfd3e387eb 100644 --- a/headers/modsecurity/rule_marker.h +++ b/headers/modsecurity/rule_marker.h @@ -42,8 +42,7 @@ class RuleMarker : public Rule { RuleMarker &operator=(const RuleMarker &r) = delete; - virtual bool evaluate(Transaction *transaction, - std::shared_ptr rm) override { + virtual bool evaluate(Transaction *transaction, RuleMessage &ruleMessage) override { return evaluate(transaction); } diff --git a/headers/modsecurity/rule_message.h b/headers/modsecurity/rule_message.h index 330e7fced0..ba30bc889c 100644 --- a/headers/modsecurity/rule_message.h +++ b/headers/modsecurity/rule_message.h @@ -13,14 +13,6 @@ * */ -#ifdef __cplusplus -#include -#include -#include -#include -#include -#endif - #ifndef HEADERS_MODSECURITY_RULE_MESSAGE_H_ #define HEADERS_MODSECURITY_RULE_MESSAGE_H_ @@ -31,8 +23,10 @@ #ifdef __cplusplus -namespace modsecurity { +#include +#include +namespace modsecurity { class RuleMessage { @@ -45,43 +39,51 @@ class RuleMessage { RuleMessage(const RuleWithActions &rule, const Transaction &trans) : m_rule(rule), m_transaction(trans) - { } + { + reset(true); + } RuleMessage(const RuleMessage &ruleMessage) = default; RuleMessage &operator=(const RuleMessage &ruleMessage) = delete; - void clean() { - m_data = ""; - m_match = ""; + void reset(const bool resetSaveMessage) + { + m_data.clear(); m_isDisruptive = false; - m_reference = ""; + m_match.clear(); + m_message.clear(); + m_noAuditLog = false; + m_reference.clear(); + if (resetSaveMessage == true) + m_saveMessage = true; m_severity = 0; + m_tags.clear(); } - std::string log() { - return log(this, 0); + std::string log() const { + return log(*this, 0); } - std::string log(int props) { - return log(this, props); + std::string log(int props) const { + return log(*this, props); } - std::string log(int props, int responseCode) { - return log(this, props, responseCode); + std::string log(int props, int responseCode) const { + return log(*this, props, responseCode); } - std::string errorLog() { - return log(this, - ClientLogMessageInfo | ErrorLogTailLogMessageInfo); + std::string errorLog() const { + return log(*this, + ClientLogMessageInfo | ErrorLogTailLogMessageInfo); } - static std::string log(const RuleMessage *rm, int props, int code); - static std::string log(const RuleMessage *rm, int props) { + static std::string log(const RuleMessage &rm, int props, int code); + static std::string log(const RuleMessage &rm, int props) { return log(rm, props, -1); } - static std::string log(const RuleMessage *rm) { + static std::string log(const RuleMessage &rm) { return log(rm, 0); } - static std::string _details(const RuleMessage *rm); - static std::string _errorLogTail(const RuleMessage *rm); + static std::string _details(const RuleMessage &rm); + static std::string _errorLogTail(const RuleMessage &rm); int getPhase() const { return m_rule.getPhase() - 1; } diff --git a/headers/modsecurity/rule_unconditional.h b/headers/modsecurity/rule_unconditional.h index 693395661f..20cc89a469 100644 --- a/headers/modsecurity/rule_unconditional.h +++ b/headers/modsecurity/rule_unconditional.h @@ -36,7 +36,7 @@ class RuleUnconditional : public RuleWithActions { public: using RuleWithActions::RuleWithActions; - virtual bool evaluate(Transaction *transaction, std::shared_ptr ruleMessage) override; + virtual bool evaluate(Transaction *transaction, RuleMessage &ruleMessage) override; }; diff --git a/headers/modsecurity/rule_with_actions.h b/headers/modsecurity/rule_with_actions.h index c29e068ece..aee16d8c04 100644 --- a/headers/modsecurity/rule_with_actions.h +++ b/headers/modsecurity/rule_with_actions.h @@ -51,21 +51,21 @@ class RuleWithActions : public Rule { virtual bool evaluate(Transaction *transaction) override; - virtual bool evaluate(Transaction *transaction, std::shared_ptr ruleMessage) override; + virtual bool evaluate(Transaction *transaction, RuleMessage &ruleMessage) override; void executeActionsIndependentOfChainedRuleResult( Transaction *trasn, bool *containsDisruptive, - std::shared_ptr ruleMessage); + RuleMessage &ruleMessage); void executeActionsAfterFullMatch( Transaction *trasn, bool containsDisruptive, - std::shared_ptr ruleMessage); + RuleMessage &ruleMessage); void executeAction(Transaction *trans, bool containsBlock, - std::shared_ptr ruleMessage, + RuleMessage &ruleMessage, actions::Action *a, bool context); @@ -74,7 +74,7 @@ class RuleWithActions : public Rule { const Transaction *trasn, const std::string &value, TransformationResults &ret); void performLogging(Transaction *trans, - std::shared_ptr ruleMessage, + RuleMessage &ruleMessage, bool lastLog = true, bool chainedParentNull = false) const; diff --git a/headers/modsecurity/rule_with_operator.h b/headers/modsecurity/rule_with_operator.h index b0abcfd89f..d0297409f0 100644 --- a/headers/modsecurity/rule_with_operator.h +++ b/headers/modsecurity/rule_with_operator.h @@ -47,8 +47,7 @@ class RuleWithOperator : public RuleWithActions { ~RuleWithOperator() override; - bool evaluate(Transaction *transaction, - std::shared_ptr rm) override; + bool evaluate(Transaction *transaction, RuleMessage &ruleMessage) override; void getVariablesExceptions(Transaction &t, variables::Variables *exclusion, variables::Variables *addition); @@ -56,7 +55,7 @@ class RuleWithOperator : public RuleWithActions { variables::Variables *eclusion, Transaction *trans); bool executeOperatorAt(Transaction *trasn, const std::string &key, - const std::string &value, std::shared_ptr rm); + const std::string &value, RuleMessage &ruleMessage); static void updateMatchedVars(Transaction *trasn, const std::string &key, const std::string &value); diff --git a/headers/modsecurity/transaction.h b/headers/modsecurity/transaction.h index 12eb3e3e45..488e6c5a7c 100644 --- a/headers/modsecurity/transaction.h +++ b/headers/modsecurity/transaction.h @@ -407,7 +407,7 @@ class Transaction : public TransactionAnchoredVariables, public TransactionSecMa #ifndef NO_LOGS void debug(int, const std::string &) const; // cppcheck-suppress functionStatic #endif - void serverLog(std::shared_ptr rm); + void serverLog(const RuleMessage &rm); int getRuleEngineState() const; diff --git a/src/actions/audit_log.cc b/src/actions/audit_log.cc index c628ac2364..53f5557114 100644 --- a/src/actions/audit_log.cc +++ b/src/actions/audit_log.cc @@ -27,11 +27,10 @@ namespace modsecurity { namespace actions { -bool AuditLog::evaluate(RuleWithActions *rule, Transaction *transaction, - std::shared_ptr rm) { - rm->m_noAuditLog = false; +bool AuditLog::evaluate(RuleWithActions *rule, Transaction *transaction, RuleMessage &ruleMessage) { + ruleMessage.m_noAuditLog = false; ms_dbg_a(transaction, 9, "Saving transaction to logs"); - rm->m_saveMessage = true; + ruleMessage.m_saveMessage = true; return true; } diff --git a/src/actions/audit_log.h b/src/actions/audit_log.h index cde743870a..e6669e434a 100644 --- a/src/actions/audit_log.h +++ b/src/actions/audit_log.h @@ -35,8 +35,7 @@ class AuditLog : public Action { explicit AuditLog(const std::string &action) : Action(action) { } - bool evaluate(RuleWithActions *rule, Transaction *transaction, - std::shared_ptr rm) override; + bool evaluate(RuleWithActions *rule, Transaction *transaction, RuleMessage &ruleMessage) override; }; diff --git a/src/actions/block.cc b/src/actions/block.cc index bde5e63464..b36540627e 100644 --- a/src/actions/block.cc +++ b/src/actions/block.cc @@ -29,15 +29,14 @@ namespace modsecurity { namespace actions { -bool Block::evaluate(RuleWithActions *rule, Transaction *transaction, - std::shared_ptr rm) { +bool Block::evaluate(RuleWithActions *rule, Transaction *transaction, RuleMessage &ruleMessage) { ms_dbg_a(transaction, 8, "Marking request as disruptive."); for (auto &a : transaction->m_rules->m_defaultActions[rule->getPhase()]) { if (a->isDisruptive() == false) { continue; } - a->evaluate(rule, transaction, rm); + a->evaluate(rule, transaction, ruleMessage); } return true; diff --git a/src/actions/block.h b/src/actions/block.h index 7c40bbd830..1f265fdac0 100644 --- a/src/actions/block.h +++ b/src/actions/block.h @@ -35,8 +35,7 @@ class Block : public Action { public: explicit Block(const std::string &action) : Action(action) { } - bool evaluate(RuleWithActions *rule, Transaction *transaction, - std::shared_ptr rm) override; + bool evaluate(RuleWithActions *rule, Transaction *transaction, RuleMessage &ruleMessage) override; }; diff --git a/src/actions/data/status.cc b/src/actions/data/status.cc index 9429973859..ed9cd0ed6e 100644 --- a/src/actions/data/status.cc +++ b/src/actions/data/status.cc @@ -39,7 +39,7 @@ bool Status::init(std::string *error) { bool Status::evaluate(RuleWithActions *rule, Transaction *transaction, - std::shared_ptr rm) { + RuleMessage &ruleMessage) { transaction->m_it.status = m_status; return true; } diff --git a/src/actions/data/status.h b/src/actions/data/status.h index 566a927ea3..3a3ec5dc4a 100644 --- a/src/actions/data/status.h +++ b/src/actions/data/status.h @@ -37,8 +37,7 @@ class Status : public Action { : Action(action), m_status(0) { } bool init(std::string *error) override; - bool evaluate(RuleWithActions *rule, Transaction *transaction, - std::shared_ptr rm) override; + bool evaluate(RuleWithActions *rule, Transaction *transaction, RuleMessage &ruleMessage) override; int m_status; }; diff --git a/src/actions/disruptive/deny.cc b/src/actions/disruptive/deny.cc index e105d65127..038c8e3d8d 100644 --- a/src/actions/disruptive/deny.cc +++ b/src/actions/disruptive/deny.cc @@ -29,7 +29,7 @@ namespace disruptive { bool Deny::evaluate(RuleWithActions *rule, Transaction *transaction, - std::shared_ptr rm) { + RuleMessage &ruleMessage) { ms_dbg_a(transaction, 8, "Running action deny"); if (transaction->m_it.status == 200) { @@ -38,9 +38,9 @@ bool Deny::evaluate(RuleWithActions *rule, Transaction *transaction, transaction->m_it.disruptive = true; intervention::freeLog(&transaction->m_it); - rm->m_isDisruptive = true; + ruleMessage.m_isDisruptive = true; transaction->m_it.log = strdup( - rm->log(RuleMessage::LogMessageInfo::ClientLogMessageInfo).c_str()); + ruleMessage.log(RuleMessage::LogMessageInfo::ClientLogMessageInfo).c_str()); return true; } diff --git a/src/actions/disruptive/deny.h b/src/actions/disruptive/deny.h index fb841a49a7..3183f531d2 100644 --- a/src/actions/disruptive/deny.h +++ b/src/actions/disruptive/deny.h @@ -33,8 +33,7 @@ class Deny : public Action { public: explicit Deny(const std::string &action) : Action(action) { } - bool evaluate(RuleWithActions *rule, Transaction *transaction, - std::shared_ptr rm) override; + bool evaluate(RuleWithActions *rule, Transaction *transaction, RuleMessage &ruleMessage) override; bool isDisruptive() override { return true; } }; diff --git a/src/actions/disruptive/drop.cc b/src/actions/disruptive/drop.cc index 18a3b55280..3dc44b199f 100644 --- a/src/actions/disruptive/drop.cc +++ b/src/actions/disruptive/drop.cc @@ -33,7 +33,7 @@ namespace disruptive { bool Drop::evaluate(RuleWithActions *rule, Transaction *transaction, - std::shared_ptr rm) { + RuleMessage &ruleMessage) { ms_dbg_a(transaction, 8, "Running action drop " \ "[executing deny instead of drop.]"); @@ -43,9 +43,9 @@ bool Drop::evaluate(RuleWithActions *rule, Transaction *transaction, transaction->m_it.disruptive = true; intervention::freeLog(&transaction->m_it); - rm->m_isDisruptive = true; + ruleMessage.m_isDisruptive = true; transaction->m_it.log = strdup( - rm->log(RuleMessage::LogMessageInfo::ClientLogMessageInfo).c_str()); + ruleMessage.log(RuleMessage::LogMessageInfo::ClientLogMessageInfo).c_str()); return true; } diff --git a/src/actions/disruptive/drop.h b/src/actions/disruptive/drop.h index f60eddfa6d..aa66b0d802 100644 --- a/src/actions/disruptive/drop.h +++ b/src/actions/disruptive/drop.h @@ -32,8 +32,7 @@ class Drop : public Action { public: explicit Drop(const std::string &action) : Action(action) { } - bool evaluate(RuleWithActions *rule, Transaction *transaction, - std::shared_ptr rm) override; + bool evaluate(RuleWithActions *rule, Transaction *transaction, RuleMessage &ruleMessage) override; bool isDisruptive() override { return true; } }; diff --git a/src/actions/disruptive/pass.cc b/src/actions/disruptive/pass.cc index e0f038c4cb..9afaeecc4f 100644 --- a/src/actions/disruptive/pass.cc +++ b/src/actions/disruptive/pass.cc @@ -30,7 +30,7 @@ namespace disruptive { bool Pass::evaluate(RuleWithActions *rule, Transaction *transaction, - std::shared_ptr rm) { + RuleMessage &ruleMessage) { intervention::free(&transaction->m_it); intervention::reset(&transaction->m_it); diff --git a/src/actions/disruptive/pass.h b/src/actions/disruptive/pass.h index 0c09d2874f..65127e9902 100644 --- a/src/actions/disruptive/pass.h +++ b/src/actions/disruptive/pass.h @@ -31,8 +31,7 @@ class Pass : public Action { public: explicit Pass(const std::string &action) : Action(action) { } - bool evaluate(RuleWithActions *rule, Transaction *transaction, - std::shared_ptr rm) override; + bool evaluate(RuleWithActions *rule, Transaction *transaction, RuleMessage &ruleMessage) override; bool isDisruptive() override { return true; } }; diff --git a/src/actions/disruptive/redirect.cc b/src/actions/disruptive/redirect.cc index ac2993b4c0..ee4f2de886 100644 --- a/src/actions/disruptive/redirect.cc +++ b/src/actions/disruptive/redirect.cc @@ -35,7 +35,7 @@ bool Redirect::init(std::string *error) { bool Redirect::evaluate(RuleWithActions *rule, Transaction *transaction, - std::shared_ptr rm) { + RuleMessage &ruleMessage) { std::string m_urlExpanded(m_string->evaluate(transaction)); /* if it was changed before, lets keep it. */ if (transaction->m_it.status == 200 @@ -47,9 +47,9 @@ bool Redirect::evaluate(RuleWithActions *rule, Transaction *transaction, transaction->m_it.url = strdup(m_urlExpanded.c_str()); transaction->m_it.disruptive = true; intervention::freeLog(&transaction->m_it); - rm->m_isDisruptive = true; + ruleMessage.m_isDisruptive = true; transaction->m_it.log = strdup( - rm->log(RuleMessage::LogMessageInfo::ClientLogMessageInfo).c_str()); + ruleMessage.log(RuleMessage::LogMessageInfo::ClientLogMessageInfo).c_str()); return true; } diff --git a/src/actions/disruptive/redirect.h b/src/actions/disruptive/redirect.h index 72ecf98e4b..ada5144172 100644 --- a/src/actions/disruptive/redirect.h +++ b/src/actions/disruptive/redirect.h @@ -46,8 +46,7 @@ class Redirect : public Action { m_status(0), m_string(std::move(z)) { } - bool evaluate(RuleWithActions *rule, Transaction *transaction, - std::shared_ptr rm) override; + bool evaluate(RuleWithActions *rule, Transaction *transaction, RuleMessage &ruleMessage) override; bool init(std::string *error) override; bool isDisruptive() override { return true; } diff --git a/src/actions/log.cc b/src/actions/log.cc index 6ca507cdb1..dc335df842 100644 --- a/src/actions/log.cc +++ b/src/actions/log.cc @@ -28,10 +28,9 @@ namespace modsecurity { namespace actions { -bool Log::evaluate(RuleWithActions *rule, Transaction *transaction, - std::shared_ptr rm) { +bool Log::evaluate(RuleWithActions *rule, Transaction *transaction, RuleMessage &ruleMessage) { ms_dbg_a(transaction, 9, "Saving transaction to logs"); - rm->m_saveMessage = true; + ruleMessage.m_saveMessage = true; return true; } diff --git a/src/actions/log.h b/src/actions/log.h index d2cb5cd26c..9a8357e938 100644 --- a/src/actions/log.h +++ b/src/actions/log.h @@ -33,8 +33,7 @@ class Log : public Action { explicit Log(const std::string &action) : Action(action) { } - bool evaluate(RuleWithActions *rule, Transaction *transaction, - std::shared_ptr rm) override; + bool evaluate(RuleWithActions *rule, Transaction *transaction, RuleMessage &ruleMessage) override; }; } // namespace actions diff --git a/src/actions/log_data.cc b/src/actions/log_data.cc index 49c539cfc4..596505868f 100644 --- a/src/actions/log_data.cc +++ b/src/actions/log_data.cc @@ -29,9 +29,8 @@ namespace modsecurity { namespace actions { -bool LogData::evaluate(RuleWithActions *rule, Transaction *transaction, - std::shared_ptr rm) { - rm->m_data = data(transaction); +bool LogData::evaluate(RuleWithActions *rule, Transaction *transaction, RuleMessage &ruleMessage) { + ruleMessage.m_data = data(transaction); return true; } diff --git a/src/actions/log_data.h b/src/actions/log_data.h index 6e618f2a86..55a20ee9b1 100644 --- a/src/actions/log_data.h +++ b/src/actions/log_data.h @@ -39,8 +39,7 @@ class LogData : public Action { : Action("logdata"), m_string(std::move(z)) { } - bool evaluate(RuleWithActions *rule, Transaction *transaction, - std::shared_ptr rm) override; + bool evaluate(RuleWithActions *rule, Transaction *transaction, RuleMessage &ruleMessage) override; std::string data(Transaction *Transaction); diff --git a/src/actions/msg.cc b/src/actions/msg.cc index 1f0b3538d1..fecab53858 100644 --- a/src/actions/msg.cc +++ b/src/actions/msg.cc @@ -46,10 +46,9 @@ namespace modsecurity { namespace actions { -bool Msg::evaluate(RuleWithActions *rule, Transaction *transaction, - std::shared_ptr rm) { - std::string msg = data(transaction); - rm->m_message = msg; +bool Msg::evaluate(RuleWithActions *rule, Transaction *transaction, RuleMessage &ruleMessage) { + const auto msg = data(transaction); + ruleMessage.m_message = msg; ms_dbg_a(transaction, 9, "Saving msg: " + msg); return true; diff --git a/src/actions/msg.h b/src/actions/msg.h index c75e6d6eb3..aca6731cad 100644 --- a/src/actions/msg.h +++ b/src/actions/msg.h @@ -40,8 +40,7 @@ class Msg : public Action { : Action("msg"), m_string(std::move(z)) { } - bool evaluate(RuleWithActions *rule, Transaction *transaction, - std::shared_ptr rm) override; + bool evaluate(RuleWithActions *rule, Transaction *transaction, RuleMessage &ruleMessage) override; std::string data(Transaction *Transaction); std::unique_ptr m_string; diff --git a/src/actions/no_audit_log.cc b/src/actions/no_audit_log.cc index e4a59f7361..eee999de9c 100644 --- a/src/actions/no_audit_log.cc +++ b/src/actions/no_audit_log.cc @@ -26,10 +26,9 @@ namespace modsecurity { namespace actions { -bool NoAuditLog::evaluate(RuleWithActions *rule, Transaction *transaction, - std::shared_ptr rm) { - rm->m_noAuditLog = true; - rm->m_saveMessage = false; +bool NoAuditLog::evaluate(RuleWithActions *rule, Transaction *transaction, RuleMessage &ruleMessage) { + ruleMessage.m_noAuditLog = true; + ruleMessage.m_saveMessage = false; return true; } diff --git a/src/actions/no_audit_log.h b/src/actions/no_audit_log.h index 6deb5e4a88..d91b9fcdf4 100644 --- a/src/actions/no_audit_log.h +++ b/src/actions/no_audit_log.h @@ -35,8 +35,7 @@ class NoAuditLog : public Action { explicit NoAuditLog(const std::string &action) : Action(action) { } - bool evaluate(RuleWithActions *rule, Transaction *transaction, - std::shared_ptr rm) override; + bool evaluate(RuleWithActions *rule, Transaction *transaction, RuleMessage &ruleMessage) override; }; } // namespace actions diff --git a/src/actions/no_log.cc b/src/actions/no_log.cc index 501ea4da4d..bb9c1fc573 100644 --- a/src/actions/no_log.cc +++ b/src/actions/no_log.cc @@ -29,9 +29,8 @@ namespace modsecurity { namespace actions { -bool NoLog::evaluate(RuleWithActions *rule, Transaction *transaction, - std::shared_ptr rm) { - rm->m_saveMessage = false; +bool NoLog::evaluate(RuleWithActions *rule, Transaction *transaction, RuleMessage &ruleMessage) { + ruleMessage.m_saveMessage = false; return true; } diff --git a/src/actions/no_log.h b/src/actions/no_log.h index 193a64ea2e..2df753af27 100644 --- a/src/actions/no_log.h +++ b/src/actions/no_log.h @@ -33,8 +33,7 @@ class NoLog : public Action { explicit NoLog(const std::string &action) : Action(action) { } - bool evaluate(RuleWithActions *rule, Transaction *transaction, - std::shared_ptr rm) override; + bool evaluate(RuleWithActions *rule, Transaction *transaction, RuleMessage &ruleMessage) override; }; } // namespace actions diff --git a/src/actions/severity.cc b/src/actions/severity.cc index 8344e1052d..26c5056e71 100644 --- a/src/actions/severity.cc +++ b/src/actions/severity.cc @@ -71,13 +71,12 @@ bool Severity::init(std::string *error) { } -bool Severity::evaluate(RuleWithActions *rule, Transaction *transaction, - std::shared_ptr rm) { +bool Severity::evaluate(RuleWithActions *rule, Transaction *transaction, RuleMessage &ruleMessage) { ms_dbg_a(transaction, 9, "This rule severity is: " + \ std::to_string(this->m_severity) + " current transaction is: " + \ std::to_string(transaction->m_highestSeverityAction)); - rm->m_severity = m_severity; + ruleMessage.m_severity = m_severity; if (transaction->m_highestSeverityAction > this->m_severity) { transaction->m_highestSeverityAction = this->m_severity; diff --git a/src/actions/severity.h b/src/actions/severity.h index 32a223e005..4c03d32676 100644 --- a/src/actions/severity.h +++ b/src/actions/severity.h @@ -35,8 +35,7 @@ class Severity : public Action { : Action(action), m_severity(0) { } - bool evaluate(RuleWithActions *rule, Transaction *transaction, - std::shared_ptr rm) override; + bool evaluate(RuleWithActions *rule, Transaction *transaction, RuleMessage &ruleMessage) override; bool init(std::string *error) override; int m_severity; diff --git a/src/actions/tag.cc b/src/actions/tag.cc index 3b1b6fd539..120aba18dc 100644 --- a/src/actions/tag.cc +++ b/src/actions/tag.cc @@ -57,12 +57,11 @@ std::string Tag::getName(Transaction *transaction) { } -bool Tag::evaluate(RuleWithActions *rule, Transaction *transaction, - std::shared_ptr rm) { +bool Tag::evaluate(RuleWithActions *rule, Transaction *transaction, RuleMessage &ruleMessage) { std::string tag = getName(transaction); ms_dbg_a(transaction, 9, "Rule tag: " + tag); - rm->m_tags.push_back(tag); + ruleMessage.m_tags.push_back(tag); return true; } diff --git a/src/actions/tag.h b/src/actions/tag.h index bf3988b5bb..eb643cd5f6 100644 --- a/src/actions/tag.h +++ b/src/actions/tag.h @@ -38,8 +38,7 @@ class Tag : public Action { std::string getName(Transaction *transaction); - bool evaluate(RuleWithActions *rule, Transaction *transaction, - std::shared_ptr rm) override; + bool evaluate(RuleWithActions *rule, Transaction *transaction, RuleMessage &ruleMessage) override; protected: std::unique_ptr m_string; diff --git a/src/modsecurity.cc b/src/modsecurity.cc index 93caa95242..e1b29857e1 100644 --- a/src/modsecurity.cc +++ b/src/modsecurity.cc @@ -190,26 +190,22 @@ const std::string& ModSecurity::getConnectorInformation() const { return m_connector; } -void ModSecurity::serverLog(void *data, std::shared_ptr rm) { +void ModSecurity::serverLog(void *data, const RuleMessage &rm) { if (m_logCb == NULL) { - std::cerr << "Server log callback is not set -- " << rm->errorLog(); + std::cerr << "Server log callback is not set -- " << rm.errorLog(); std::cerr << std::endl; return; } - if (rm == NULL) { - return; - } - if (m_logProperties & TextLogProperty) { - auto d = rm->log(); + auto d = rm.log(); const void *a = static_cast(d.c_str()); m_logCb(data, a); return; } if (m_logProperties & RuleMessageLogProperty) { - const void *a = static_cast(rm.get()); + const void *a = static_cast(&rm); m_logCb(data, a); return; } diff --git a/src/operators/begins_with.cc b/src/operators/begins_with.cc index a2f89fce9c..007cafe270 100644 --- a/src/operators/begins_with.cc +++ b/src/operators/begins_with.cc @@ -25,7 +25,7 @@ namespace operators { bool BeginsWith::evaluate(Transaction *transaction, RuleWithActions *rule, - const std::string &str, std::shared_ptr ruleMessage) { + const std::string &str, RuleMessage &ruleMessage) { std::string p(m_string->evaluate(transaction)); if (str.size() < p.size()) { diff --git a/src/operators/begins_with.h b/src/operators/begins_with.h index c9fcee467a..7482012a78 100644 --- a/src/operators/begins_with.h +++ b/src/operators/begins_with.h @@ -33,7 +33,7 @@ class BeginsWith : public Operator { : Operator("BeginsWith", std::move(param)) { } bool evaluate(Transaction *transaction, RuleWithActions *rule, const std::string &str, - std::shared_ptr ruleMessage) override; + RuleMessage &ruleMessage) override; }; } // namespace operators diff --git a/src/operators/contains.cc b/src/operators/contains.cc index 95c2702a88..b5e3150716 100644 --- a/src/operators/contains.cc +++ b/src/operators/contains.cc @@ -22,7 +22,7 @@ namespace modsecurity { namespace operators { bool Contains::evaluate(Transaction *transaction, RuleWithActions *rule, - const std::string &input, std::shared_ptr ruleMessage) { + const std::string &input, RuleMessage &ruleMessage) { std::string p(m_string->evaluate(transaction)); size_t offset = input.find(p); diff --git a/src/operators/contains.h b/src/operators/contains.h index 13fcda9224..65fa73b351 100644 --- a/src/operators/contains.h +++ b/src/operators/contains.h @@ -36,7 +36,7 @@ class Contains : public Operator { : Operator("Contains", std::move(param)) { } bool evaluate(Transaction *transaction, RuleWithActions *rule, const std::string &str, - std::shared_ptr ruleMessage) override; + RuleMessage &ruleMessage) override; }; } // namespace operators diff --git a/src/operators/contains_word.cc b/src/operators/contains_word.cc index 9bcdbd6cf3..3de6785166 100644 --- a/src/operators/contains_word.cc +++ b/src/operators/contains_word.cc @@ -37,7 +37,7 @@ bool ContainsWord::acceptableChar(const std::string& a, size_t pos) { } bool ContainsWord::evaluate(Transaction *transaction, RuleWithActions *rule, - const std::string &str, std::shared_ptr ruleMessage) { + const std::string &str, RuleMessage &ruleMessage) { std::string paramTarget(m_string->evaluate(transaction)); if (paramTarget.empty()) { diff --git a/src/operators/contains_word.h b/src/operators/contains_word.h index c15e399089..8bc859a883 100644 --- a/src/operators/contains_word.h +++ b/src/operators/contains_word.h @@ -34,7 +34,7 @@ class ContainsWord : public Operator { bool evaluate(Transaction *transaction, RuleWithActions *rule, const std::string &str, - std::shared_ptr ruleMessage) override; + RuleMessage &ruleMessage) override; private: static bool acceptableChar(const std::string& a, size_t pos); diff --git a/src/operators/detect_sqli.cc b/src/operators/detect_sqli.cc index 2c734d85eb..49cef935c1 100644 --- a/src/operators/detect_sqli.cc +++ b/src/operators/detect_sqli.cc @@ -26,7 +26,7 @@ namespace operators { bool DetectSQLi::evaluate(Transaction *t, RuleWithActions *rule, - const std::string& input, std::shared_ptr ruleMessage) { + const std::string& input, RuleMessage &ruleMessage) { char fingerprint[8]; int issqli; diff --git a/src/operators/detect_sqli.h b/src/operators/detect_sqli.h index 237e6a2ffd..71308a4912 100644 --- a/src/operators/detect_sqli.h +++ b/src/operators/detect_sqli.h @@ -34,7 +34,7 @@ class DetectSQLi : public Operator { bool evaluate(Transaction *t, RuleWithActions *rule, const std::string& input, - std::shared_ptr ruleMessage) override; + RuleMessage &ruleMessage) override; }; } // namespace operators diff --git a/src/operators/detect_xss.cc b/src/operators/detect_xss.cc index cf4b7861de..014202e731 100644 --- a/src/operators/detect_xss.cc +++ b/src/operators/detect_xss.cc @@ -26,7 +26,7 @@ namespace operators { bool DetectXSS::evaluate(Transaction *t, RuleWithActions *rule, - const std::string& input, std::shared_ptr ruleMessage) { + const std::string& input, RuleMessage &ruleMessage) { int is_xss; is_xss = libinjection_xss(input.c_str(), input.length()); diff --git a/src/operators/detect_xss.h b/src/operators/detect_xss.h index c455462513..943937c0c2 100644 --- a/src/operators/detect_xss.h +++ b/src/operators/detect_xss.h @@ -33,7 +33,7 @@ class DetectXSS : public Operator { bool evaluate(Transaction *t, RuleWithActions *rule, const std::string& input, - std::shared_ptr ruleMessage) override; + RuleMessage &ruleMessage) override; }; } // namespace operators diff --git a/src/operators/ends_with.cc b/src/operators/ends_with.cc index 404f3ffd03..f81e179345 100644 --- a/src/operators/ends_with.cc +++ b/src/operators/ends_with.cc @@ -24,7 +24,7 @@ namespace operators { bool EndsWith::evaluate(Transaction *transaction, RuleWithActions *rule, - const std::string &str, std::shared_ptr ruleMessage) { + const std::string &str, RuleMessage &ruleMessage) { bool ret = false; std::string p(m_string->evaluate(transaction)); diff --git a/src/operators/ends_with.h b/src/operators/ends_with.h index 47e42c3fd9..bb69091099 100644 --- a/src/operators/ends_with.h +++ b/src/operators/ends_with.h @@ -35,7 +35,7 @@ class EndsWith : public Operator { } bool evaluate(Transaction *transaction, RuleWithActions *rule, const std::string &str, - std::shared_ptr ruleMessage) override; + RuleMessage &ruleMessage) override; }; diff --git a/src/operators/operator.cc b/src/operators/operator.cc index 57974bacf5..238c885e6d 100644 --- a/src/operators/operator.cc +++ b/src/operators/operator.cc @@ -71,8 +71,8 @@ namespace operators { bool Operator::evaluateInternal(Transaction *transaction, - RuleWithActions *rule, const std::string& a, std::shared_ptr rm) { - bool res = evaluate(transaction, rule, a, rm); + RuleWithActions *rule, const std::string& a, RuleMessage &ruleMessage) { + bool res = evaluate(transaction, rule, a, ruleMessage); if (m_negation) { return !res; diff --git a/src/operators/operator.h b/src/operators/operator.h index bcf14e9dc3..ba94a1b466 100644 --- a/src/operators/operator.h +++ b/src/operators/operator.h @@ -115,7 +115,7 @@ class Operator { bool evaluateInternal(Transaction *t, RuleWithActions *rule, const std::string& a); bool evaluateInternal(Transaction *t, RuleWithActions *rule, - const std::string& a, std::shared_ptr ruleMessage); + const std::string& a, RuleMessage &ruleMessage); virtual bool evaluate(Transaction *transaction, const std::string &str); @@ -124,16 +124,14 @@ class Operator { return evaluate(transaction, str); } virtual bool evaluate(Transaction *transaction, RuleWithActions *rule, - const std::string &str, std::shared_ptr ruleMessage) { + const std::string &str, RuleMessage &ruleMessage) { return evaluate(transaction, str); } - static void logOffset(std::shared_ptr ruleMessage, int offset, int len) { - if (ruleMessage) { - ruleMessage->m_reference.append("o" - + std::to_string(offset) + "," - + std::to_string(len)); - } + static void logOffset(RuleMessage &ruleMessage, int offset, int len) { + ruleMessage.m_reference.append("o" + + std::to_string(offset) + "," + + std::to_string(len)); } std::string m_match_message; diff --git a/src/operators/pm.cc b/src/operators/pm.cc index ccd809e125..91e4f5d7b4 100644 --- a/src/operators/pm.cc +++ b/src/operators/pm.cc @@ -140,7 +140,7 @@ void Pm::postOrderTraversal(acmp_btree_node_t *node) { bool Pm::evaluate(Transaction *transaction, RuleWithActions *rule, - const std::string &input, std::shared_ptr ruleMessage) { + const std::string &input, RuleMessage &ruleMessage) { int rc; ACMPT pt; pt.parser = m_p; diff --git a/src/operators/pm.h b/src/operators/pm.h index 72d3a54611..1b45fff79e 100644 --- a/src/operators/pm.h +++ b/src/operators/pm.h @@ -43,7 +43,7 @@ class Pm : public Operator { ~Pm(); bool evaluate(Transaction *transaction, RuleWithActions *rule, const std::string &str, - std::shared_ptr ruleMessage) override; + RuleMessage &ruleMessage) override; bool init(const std::string &file, std::string *error) override; diff --git a/src/operators/rbl.cc b/src/operators/rbl.cc index f1561e7f08..35bbc5f934 100644 --- a/src/operators/rbl.cc +++ b/src/operators/rbl.cc @@ -207,7 +207,7 @@ void Rbl::furtherInfo(struct sockaddr_in *sin, const std::string &ipStr, bool Rbl::evaluate(Transaction *t, RuleWithActions *rule, const std::string& ipStr, - std::shared_ptr ruleMessage) { + RuleMessage &ruleMessage) { struct addrinfo *info = NULL; std::string host = Rbl::mapIpToAddress(ipStr, t); int rc = 0; diff --git a/src/operators/rbl.h b/src/operators/rbl.h index 30fcaa3e37..e7d9538cd3 100644 --- a/src/operators/rbl.h +++ b/src/operators/rbl.h @@ -83,7 +83,7 @@ class Rbl : public Operator { } bool evaluate(Transaction *transaction, RuleWithActions *rule, const std::string& input, - std::shared_ptr ruleMessage) override; + RuleMessage &ruleMessage) override; std::string mapIpToAddress(const std::string &ipStr, Transaction *trans) const; diff --git a/src/operators/rx.cc b/src/operators/rx.cc index 559b2d8169..ce6526356f 100644 --- a/src/operators/rx.cc +++ b/src/operators/rx.cc @@ -37,7 +37,7 @@ bool Rx::init(const std::string &arg, std::string *error) { bool Rx::evaluate(Transaction *transaction, RuleWithActions *rule, - const std::string& input, std::shared_ptr ruleMessage) { + const std::string& input, RuleMessage &ruleMessage) { Regex *re; if (m_param.empty() && !m_string->m_containsMacro) { diff --git a/src/operators/rx.h b/src/operators/rx.h index 817e74eb0b..86a12d523a 100644 --- a/src/operators/rx.h +++ b/src/operators/rx.h @@ -51,7 +51,7 @@ class Rx : public Operator { bool evaluate(Transaction *transaction, RuleWithActions *rule, const std::string& input, - std::shared_ptr ruleMessage) override; + RuleMessage &ruleMessage) override; bool init(const std::string &arg, std::string *error) override; diff --git a/src/operators/rx_global.cc b/src/operators/rx_global.cc index 9a0978ca86..6aeda76132 100644 --- a/src/operators/rx_global.cc +++ b/src/operators/rx_global.cc @@ -37,7 +37,7 @@ bool RxGlobal::init(const std::string &arg, std::string *error) { bool RxGlobal::evaluate(Transaction *transaction, RuleWithActions *rule, - const std::string& input, std::shared_ptr ruleMessage) { + const std::string& input, RuleMessage &ruleMessage) { Regex *re; if (m_param.empty() && !m_string->m_containsMacro) { diff --git a/src/operators/rx_global.h b/src/operators/rx_global.h index 86e37d0d5f..5b6ed8757b 100644 --- a/src/operators/rx_global.h +++ b/src/operators/rx_global.h @@ -51,7 +51,7 @@ class RxGlobal : public Operator { bool evaluate(Transaction *transaction, RuleWithActions *rule, const std::string& input, - std::shared_ptr ruleMessage) override; + RuleMessage &ruleMessage) override; bool init(const std::string &arg, std::string *error) override; diff --git a/src/operators/validate_byte_range.cc b/src/operators/validate_byte_range.cc index 07b88149c2..47802cef48 100644 --- a/src/operators/validate_byte_range.cc +++ b/src/operators/validate_byte_range.cc @@ -111,7 +111,7 @@ bool ValidateByteRange::init(const std::string &file, bool ValidateByteRange::evaluate(Transaction *transaction, RuleWithActions *rule, - const std::string &input, std::shared_ptr ruleMessage) { + const std::string &input, RuleMessage &ruleMessage) { bool ret = true; size_t count = 0; diff --git a/src/operators/validate_byte_range.h b/src/operators/validate_byte_range.h index 2c44e76921..7551171b01 100644 --- a/src/operators/validate_byte_range.h +++ b/src/operators/validate_byte_range.h @@ -39,7 +39,7 @@ class ValidateByteRange : public Operator { bool evaluate(Transaction *transaction, RuleWithActions *rule, const std::string &input, - std::shared_ptr ruleMessage) override; + RuleMessage &ruleMessage) override; bool getRange(const std::string &rangeRepresentation, std::string *error); bool init(const std::string& file, std::string *error) override; private: diff --git a/src/operators/validate_url_encoding.cc b/src/operators/validate_url_encoding.cc index 502aa3d49e..7ca71b221c 100644 --- a/src/operators/validate_url_encoding.cc +++ b/src/operators/validate_url_encoding.cc @@ -69,7 +69,7 @@ int ValidateUrlEncoding::validate_url_encoding(const char *input, bool ValidateUrlEncoding::evaluate(Transaction *transaction, RuleWithActions *rule, - const std::string &input, std::shared_ptr ruleMessage) { + const std::string &input, RuleMessage &ruleMessage) { size_t offset = 0; bool res = false; diff --git a/src/operators/validate_url_encoding.h b/src/operators/validate_url_encoding.h index fe274dc0a0..25c6db5d05 100644 --- a/src/operators/validate_url_encoding.h +++ b/src/operators/validate_url_encoding.h @@ -33,7 +33,7 @@ class ValidateUrlEncoding : public Operator { bool evaluate(Transaction *transaction, RuleWithActions *rule, const std::string &input, - std::shared_ptr ruleMessage) override; + RuleMessage &ruleMessage) override; static int validate_url_encoding(const char *input, uint64_t input_length, size_t *offset); diff --git a/src/operators/validate_utf8_encoding.cc b/src/operators/validate_utf8_encoding.cc index cd2f064c8c..1a166efb69 100644 --- a/src/operators/validate_utf8_encoding.cc +++ b/src/operators/validate_utf8_encoding.cc @@ -122,7 +122,7 @@ int ValidateUtf8Encoding::detect_utf8_character( } bool ValidateUtf8Encoding::evaluate(Transaction *transaction, RuleWithActions *rule, - const std::string &str, std::shared_ptr ruleMessage) { + const std::string &str, RuleMessage &ruleMessage) { unsigned int i, bytes_left; const char *str_c = str.c_str(); diff --git a/src/operators/validate_utf8_encoding.h b/src/operators/validate_utf8_encoding.h index 2bd75dc8e7..fdb6ab01d7 100644 --- a/src/operators/validate_utf8_encoding.h +++ b/src/operators/validate_utf8_encoding.h @@ -33,7 +33,7 @@ class ValidateUtf8Encoding : public Operator { bool evaluate(Transaction *transaction, RuleWithActions *rule, const std::string &str, - std::shared_ptr ruleMessage) override; + RuleMessage &ruleMessage) override; static int detect_utf8_character(const unsigned char *p_read, unsigned int length); diff --git a/src/operators/verify_cc.cc b/src/operators/verify_cc.cc index 8a36bb7e7d..66f2e91178 100644 --- a/src/operators/verify_cc.cc +++ b/src/operators/verify_cc.cc @@ -135,7 +135,7 @@ bool VerifyCC::init(const std::string ¶m2, std::string *error) { bool VerifyCC::evaluate(Transaction *t, RuleWithActions *rule, - const std::string& i, std::shared_ptr ruleMessage) { + const std::string& i, RuleMessage &ruleMessage) { #ifdef WITH_PCRE2 PCRE2_SIZE offset = 0; size_t target_length = i.length(); diff --git a/src/operators/verify_cc.h b/src/operators/verify_cc.h index f7d716daf3..e14f728af0 100644 --- a/src/operators/verify_cc.h +++ b/src/operators/verify_cc.h @@ -49,7 +49,7 @@ class VerifyCC : public Operator { bool evaluate(Transaction *t, RuleWithActions *rule, const std::string& input, - std::shared_ptr ruleMessage) override; + RuleMessage &ruleMessage) override; bool init(const std::string ¶m, std::string *error) override; private: #if WITH_PCRE2 diff --git a/src/operators/verify_cpf.cc b/src/operators/verify_cpf.cc index 1dcf18444f..b012eac826 100644 --- a/src/operators/verify_cpf.cc +++ b/src/operators/verify_cpf.cc @@ -109,7 +109,7 @@ bool VerifyCPF::verify(const char *cpfnumber, int len) { bool VerifyCPF::evaluate(Transaction *t, RuleWithActions *rule, - const std::string& input, std::shared_ptr ruleMessage) { + const std::string& input, RuleMessage &ruleMessage) { std::list matches; bool is_cpf = false; int i; diff --git a/src/operators/verify_cpf.h b/src/operators/verify_cpf.h index eecf71b1a2..ccb9988d17 100644 --- a/src/operators/verify_cpf.h +++ b/src/operators/verify_cpf.h @@ -48,7 +48,7 @@ class VerifyCPF : public Operator { bool evaluate(Transaction *transaction, RuleWithActions *rule, const std::string& input, - std::shared_ptr ruleMessage) override; + RuleMessage &ruleMessage) override; bool verify(const char *ssnumber, int len); diff --git a/src/operators/verify_ssn.cc b/src/operators/verify_ssn.cc index 59a36dd78f..eabeb1a4ec 100644 --- a/src/operators/verify_ssn.cc +++ b/src/operators/verify_ssn.cc @@ -111,7 +111,7 @@ bool VerifySSN::verify(const char *ssnumber, int len) { bool VerifySSN::evaluate(Transaction *t, RuleWithActions *rule, - const std::string& input, std::shared_ptr ruleMessage) { + const std::string& input, RuleMessage &ruleMessage) { std::list matches; bool is_ssn = false; int i; diff --git a/src/operators/verify_ssn.h b/src/operators/verify_ssn.h index 7c0828fbb9..7bb40c33c2 100644 --- a/src/operators/verify_ssn.h +++ b/src/operators/verify_ssn.h @@ -48,7 +48,7 @@ class VerifySSN : public Operator { bool evaluate(Transaction *transaction, RuleWithActions *rule, const std::string& input, - std::shared_ptr ruleMessage) override; + RuleMessage &ruleMessage) override; diff --git a/src/operators/verify_svnr.cc b/src/operators/verify_svnr.cc index 87834dd4d3..f869411d34 100644 --- a/src/operators/verify_svnr.cc +++ b/src/operators/verify_svnr.cc @@ -78,7 +78,7 @@ bool VerifySVNR::verify(const char *svnrnumber, int len) { bool VerifySVNR::evaluate(Transaction *t, RuleWithActions *rule, - const std::string& input, std::shared_ptr ruleMessage) { + const std::string& input, RuleMessage &ruleMessage) { std::list matches; bool is_svnr = false; int i; diff --git a/src/operators/verify_svnr.h b/src/operators/verify_svnr.h index 6fe9df9afb..ba4358343e 100644 --- a/src/operators/verify_svnr.h +++ b/src/operators/verify_svnr.h @@ -34,7 +34,7 @@ class VerifySVNR : public Operator { bool evaluate(Transaction *transaction, RuleWithActions *rule, const std::string& input, - std::shared_ptr ruleMessage) override; + RuleMessage &ruleMessage) override; bool verify(const char *ssnumber, int len); diff --git a/src/operators/within.cc b/src/operators/within.cc index ce781cc7d7..ed58ce2e3d 100644 --- a/src/operators/within.cc +++ b/src/operators/within.cc @@ -25,7 +25,7 @@ namespace operators { bool Within::evaluate(Transaction *transaction, RuleWithActions *rule, - const std::string &str, std::shared_ptr ruleMessage) { + const std::string &str, RuleMessage &ruleMessage) { bool res = false; size_t pos = 0; std::string paramTarget(m_string->evaluate(transaction)); diff --git a/src/operators/within.h b/src/operators/within.h index 7fcb430910..b67ad35f18 100644 --- a/src/operators/within.h +++ b/src/operators/within.h @@ -34,7 +34,7 @@ class Within : public Operator { m_couldContainsMacro = true; } bool evaluate(Transaction *transaction, RuleWithActions *rule, - const std::string &str, std::shared_ptr ruleMessage) override; + const std::string &str, RuleMessage &ruleMessage) override; }; } // namespace operators diff --git a/src/rule_message.cc b/src/rule_message.cc index b2cb727ae9..7c021d17eb 100644 --- a/src/rule_message.cc +++ b/src/rule_message.cc @@ -23,55 +23,55 @@ namespace modsecurity { -std::string RuleMessage::_details(const RuleMessage *rm) { +std::string RuleMessage::_details(const RuleMessage &rm) { std::string msg; - msg.append(" [file \"" + rm->m_rule.getFileName() + "\"]"); - msg.append(" [line \"" + std::to_string(rm->m_rule.getLineNumber()) + "\"]"); - msg.append(" [id \"" + std::to_string(rm->m_rule.m_ruleId) + "\"]"); - msg.append(" [rev \"" + utils::string::toHexIfNeeded(rm->m_rule.m_rev, true) + "\"]"); - msg.append(" [msg \"" + rm->m_message + "\"]"); - msg.append(" [data \"" + utils::string::toHexIfNeeded(utils::string::limitTo(200, rm->m_data), true) + "\"]"); + msg.append(" [file \"" + rm.m_rule.getFileName() + "\"]"); + msg.append(" [line \"" + std::to_string(rm.m_rule.getLineNumber()) + "\"]"); + msg.append(" [id \"" + std::to_string(rm.m_rule.m_ruleId) + "\"]"); + msg.append(" [rev \"" + utils::string::toHexIfNeeded(rm.m_rule.m_rev, true) + "\"]"); + msg.append(" [msg \"" + rm.m_message + "\"]"); + msg.append(" [data \"" + utils::string::toHexIfNeeded(utils::string::limitTo(200, rm.m_data), true) + "\"]"); msg.append(" [severity \"" + - std::to_string(rm->m_severity) + "\"]"); - msg.append(" [ver \"" + utils::string::toHexIfNeeded(rm->m_rule.m_ver, true) + "\"]"); - msg.append(" [maturity \"" + std::to_string(rm->m_rule.m_maturity) + "\"]"); - msg.append(" [accuracy \"" + std::to_string(rm->m_rule.m_accuracy) + "\"]"); + std::to_string(rm.m_severity) + "\"]"); + msg.append(" [ver \"" + utils::string::toHexIfNeeded(rm.m_rule.m_ver, true) + "\"]"); + msg.append(" [maturity \"" + std::to_string(rm.m_rule.m_maturity) + "\"]"); + msg.append(" [accuracy \"" + std::to_string(rm.m_rule.m_accuracy) + "\"]"); - for (const auto &a : rm->m_tags) { + for (const auto &a : rm.m_tags) { msg.append(" [tag \"" + utils::string::toHexIfNeeded(a, true) + "\"]"); } - msg.append(" [hostname \"" + rm->m_transaction.m_requestHostName \ + msg.append(" [hostname \"" + rm.m_transaction.m_requestHostName \ + "\"]"); - msg.append(" [uri \"" + utils::string::limitTo(200, rm->m_transaction.m_uri_no_query_string_decoded) + "\"]"); - msg.append(" [unique_id \"" + rm->m_transaction.m_id + "\"]"); - msg.append(" [ref \"" + utils::string::limitTo(200, rm->m_reference) + "\"]"); + msg.append(" [uri \"" + utils::string::limitTo(200, rm.m_transaction.m_uri_no_query_string_decoded) + "\"]"); + msg.append(" [unique_id \"" + rm.m_transaction.m_id + "\"]"); + msg.append(" [ref \"" + utils::string::limitTo(200, rm.m_reference) + "\"]"); return msg; } -std::string RuleMessage::_errorLogTail(const RuleMessage *rm) { +std::string RuleMessage::_errorLogTail(const RuleMessage &rm) { std::string msg; - msg.append("[hostname \"" + rm->m_transaction.m_serverIpAddress + "\"]"); - msg.append(" [uri \"" + utils::string::limitTo(200, rm->m_transaction.m_uri_no_query_string_decoded) + "\"]"); - msg.append(" [unique_id \"" + rm->m_transaction.m_id + "\"]"); + msg.append("[hostname \"" + rm.m_transaction.m_serverIpAddress + "\"]"); + msg.append(" [uri \"" + utils::string::limitTo(200, rm.m_transaction.m_uri_no_query_string_decoded) + "\"]"); + msg.append(" [unique_id \"" + rm.m_transaction.m_id + "\"]"); return msg; } -std::string RuleMessage::log(const RuleMessage *rm, int props, int code) { +std::string RuleMessage::log(const RuleMessage &rm, int props, int code) { std::string msg(""); msg.reserve(2048); if (props & ClientLogMessageInfo) { - msg.append("[client " + rm->m_transaction.m_clientIpAddress + "] "); + msg.append("[client " + rm.m_transaction.m_clientIpAddress + "] "); } - if (rm->m_isDisruptive) { + if (rm.m_isDisruptive) { msg.append("ModSecurity: Access denied with code "); if (code == -1) { msg.append("%d"); @@ -79,12 +79,12 @@ std::string RuleMessage::log(const RuleMessage *rm, int props, int code) { msg.append(std::to_string(code)); } msg.append(" (phase "); - msg.append(std::to_string(rm->getPhase()) + "). "); + msg.append(std::to_string(rm.getPhase()) + "). "); } else { msg.append("ModSecurity: Warning. "); } - msg.append(rm->m_match); + msg.append(rm.m_match); msg.append(_details(rm)); if (props & ErrorLogTailLogMessageInfo) { diff --git a/src/rule_script.cc b/src/rule_script.cc index c74497d3fd..ca4e5ae8bc 100644 --- a/src/rule_script.cc +++ b/src/rule_script.cc @@ -23,7 +23,7 @@ bool RuleScript::init(std::string *err) { } bool RuleScript::evaluate(Transaction *trans, - std::shared_ptr ruleMessage) { + RuleMessage &ruleMessage) { ms_dbg_a(trans, 4, " Executing script: " + m_name + "."); diff --git a/src/rule_script.h b/src/rule_script.h index d7fb8174b7..824378e4c8 100644 --- a/src/rule_script.h +++ b/src/rule_script.h @@ -59,8 +59,7 @@ class RuleScript : public RuleWithActions { bool init(std::string *err); - bool evaluate(Transaction *trans, - std::shared_ptr ruleMessage) override; + bool evaluate(Transaction *trans, RuleMessage &ruleMessage) override; std::string m_name; engine::Lua m_lua; diff --git a/src/rule_unconditional.cc b/src/rule_unconditional.cc index e6bacd666a..0bbef40e80 100644 --- a/src/rule_unconditional.cc +++ b/src/rule_unconditional.cc @@ -20,7 +20,7 @@ namespace modsecurity { bool RuleUnconditional::evaluate(Transaction *trans, - std::shared_ptr ruleMessage) { + RuleMessage &ruleMessage) { RuleWithActions::evaluate(trans, ruleMessage); // FIXME: This needs to be romeved on the runtime exeption review. diff --git a/src/rule_with_actions.cc b/src/rule_with_actions.cc index 301bbc311b..373acf1ef3 100644 --- a/src/rule_with_actions.cc +++ b/src/rule_with_actions.cc @@ -179,12 +179,13 @@ RuleWithActions::~RuleWithActions() { bool RuleWithActions::evaluate(Transaction *transaction) { - return evaluate(transaction, std::make_shared(*this, *transaction)); + RuleMessage rm(*this, *transaction); + return evaluate(transaction, rm); } bool RuleWithActions::evaluate(Transaction *transaction, - std::shared_ptr ruleMessage) { + RuleMessage &ruleMessage) { /* Rule evaluate is pure virtual. * @@ -199,7 +200,7 @@ bool RuleWithActions::evaluate(Transaction *transaction, void RuleWithActions::executeActionsIndependentOfChainedRuleResult(Transaction *trans, - bool *containsBlock, std::shared_ptr ruleMessage) { + bool *containsBlock, RuleMessage &ruleMessage) { for (actions::SetVar *a : m_actionsSetVar) { ms_dbg_a(trans, 4, "Running [independent] (non-disruptive) " \ @@ -243,7 +244,7 @@ void RuleWithActions::executeActionsIndependentOfChainedRuleResult(Transaction * void RuleWithActions::executeActionsAfterFullMatch(Transaction *trans, - bool containsBlock, std::shared_ptr ruleMessage) { + bool containsBlock, RuleMessage &ruleMessage) { bool disruptiveAlreadyExecuted = false; for (const auto &a : trans->m_rules->m_defaultActions[getPhase()]) { // cppcheck-suppress ctunullpointer @@ -296,7 +297,7 @@ void RuleWithActions::executeActionsAfterFullMatch(Transaction *trans, void RuleWithActions::executeAction(Transaction *trans, - bool containsBlock, std::shared_ptr ruleMessage, + bool containsBlock, RuleMessage &ruleMessage, Action *a, bool defaultContext) { if (a->isDisruptive() == false && *a->m_name.get() != "block") { ms_dbg_a(trans, 9, "Running " \ @@ -492,12 +493,12 @@ std::vector RuleWithActions::getActionsByName(const std::stri } void RuleWithActions::performLogging(Transaction *trans, - std::shared_ptr ruleMessage, + RuleMessage &ruleMessage, bool lastLog, bool chainedParentNull) const { /* last rule in the chain. */ - bool isItToBeLogged = ruleMessage->m_saveMessage; + bool isItToBeLogged = ruleMessage.m_saveMessage; /** * @@ -512,31 +513,31 @@ void RuleWithActions::performLogging(Transaction *trans, **/ if (lastLog) { if (chainedParentNull) { - isItToBeLogged = (ruleMessage->m_saveMessage && (m_chainedRuleParent == nullptr)); + isItToBeLogged = (ruleMessage.m_saveMessage && (m_chainedRuleParent == nullptr)); if (isItToBeLogged && !hasMultimatch()) { /* warn */ - trans->m_rulesMessages.push_back(*ruleMessage); + trans->m_rulesMessages.push_back(ruleMessage); /* error */ - if (!ruleMessage->m_isDisruptive) { + if (!ruleMessage.m_isDisruptive) { trans->serverLog(ruleMessage); } } } else if (hasBlockAction() && !hasMultimatch()) { /* warn */ - trans->m_rulesMessages.push_back(*ruleMessage); + trans->m_rulesMessages.push_back(ruleMessage); /* error */ - if (!ruleMessage->m_isDisruptive) { + if (!ruleMessage.m_isDisruptive) { trans->serverLog(ruleMessage); } } else { if (isItToBeLogged && !hasMultimatch() - && !ruleMessage->m_message.empty()) { + && !ruleMessage.m_message.empty()) { /* warn */ - trans->m_rulesMessages.push_back(*ruleMessage); + trans->m_rulesMessages.push_back(ruleMessage); /* error */ - if (!ruleMessage->m_isDisruptive) { + if (!ruleMessage.m_isDisruptive) { trans->serverLog(ruleMessage); } } @@ -544,16 +545,14 @@ void RuleWithActions::performLogging(Transaction *trans, } else { if (hasMultimatch() && isItToBeLogged) { /* warn */ - trans->m_rulesMessages.push_back(*ruleMessage.get()); + trans->m_rulesMessages.push_back(ruleMessage); /* error */ - if (!ruleMessage->m_isDisruptive) { + if (!ruleMessage.m_isDisruptive) { trans->serverLog(ruleMessage); } - RuleMessage *rm = new RuleMessage(*this, *trans); - rm->m_saveMessage = ruleMessage->m_saveMessage; - ruleMessage.reset(rm); + ruleMessage.reset(false); } } } diff --git a/src/rule_with_operator.cc b/src/rule_with_operator.cc index fcd6714881..e2864eb03f 100644 --- a/src/rule_with_operator.cc +++ b/src/rule_with_operator.cc @@ -102,7 +102,7 @@ void RuleWithOperator::cleanMatchedVars(Transaction *trans) { bool RuleWithOperator::executeOperatorAt(Transaction *trans, const std::string &key, - const std::string &value, std::shared_ptr ruleMessage) { + const std::string &value, RuleMessage &ruleMessage) { #if MSC_EXEC_CLOCK_ENABLED clock_t begin = clock(); clock_t end; @@ -202,7 +202,7 @@ inline void RuleWithOperator::getFinalVars(variables::Variables *vars, bool RuleWithOperator::evaluate(Transaction *trans, - std::shared_ptr ruleMessage) { + RuleMessage &ruleMessage) { bool globalRet = false; variables::Variables *variables = this->m_variables; bool recursiveGlobalRet; @@ -301,13 +301,13 @@ bool RuleWithOperator::evaluate(Transaction *trans, const bool ret = executeOperatorAt(trans, key, valueAfterTrans, ruleMessage); if (ret == true) { - ruleMessage->m_match = m_operator->resolveMatchMessage(trans, + ruleMessage.m_match = m_operator->resolveMatchMessage(trans, key, value); for (const auto &i : v->getOrigin()) { - ruleMessage->m_reference.append(i.toText()); + ruleMessage.m_reference.append(i.toText()); } - ruleMessage->m_reference.append(*valueTemp.second); + ruleMessage.m_reference.append(*valueTemp.second); updateMatchedVars(trans, key, valueAfterTrans); executeActionsIndependentOfChainedRuleResult(trans, &containsBlock, ruleMessage); diff --git a/src/transaction.cc b/src/transaction.cc index 78416f17aa..1b055ad084 100644 --- a/src/transaction.cc +++ b/src/transaction.cc @@ -1750,7 +1750,7 @@ std::string Transaction::toJSON(int parts) { } -void Transaction::serverLog(std::shared_ptr rm) { +void Transaction::serverLog(const RuleMessage &rm) { m_ms->serverLog(m_logCbData, rm); } diff --git a/test/unit/unit.cc b/test/unit/unit.cc index 5ebc9d3fe4..d1b871c6cd 100644 --- a/test/unit/unit.cc +++ b/test/unit/unit.cc @@ -32,6 +32,7 @@ #include "test/common/modsecurity_test.h" #include "test/common/modsecurity_test_results.h" +#include "test/common/modsecurity_test_context.h" #include "test/common/colors.h" #include "test/unit/unit_test.h" #include "src/utils/string.h" @@ -69,8 +70,10 @@ struct OperatorTest { return op; } - static UnitTestResult eval(ItemType &op, const UnitTest &t) { - return {op.evaluate(nullptr, nullptr, t.input, nullptr), {}}; + static UnitTestResult eval(ItemType &op, const UnitTest &t, modsecurity::Transaction &transaction) { + modsecurity::RuleWithActions rule{nullptr, nullptr, "dummy.conf", -1}; + modsecurity::RuleMessage ruleMessage{rule, transaction}; + return {op.evaluate(&transaction, &rule, t.input, ruleMessage), {}}; } static bool check(const UnitTestResult &result, const UnitTest &t) { @@ -89,9 +92,9 @@ struct TransformationTest { return tfn; } - static UnitTestResult eval(const ItemType &tfn, const UnitTest &t) { - std::string ret = t.input; - tfn.transform(ret, nullptr); + static UnitTestResult eval(const ItemType &tfn, const UnitTest &t, modsecurity::Transaction &transaction) { + auto ret = t.input; + tfn.transform(ret, &transaction); return {1, ret}; } @@ -102,16 +105,16 @@ struct TransformationTest { template -UnitTestResult perform_unit_test_once(const UnitTest &t) { +UnitTestResult perform_unit_test_once(const UnitTest &t, modsecurity::Transaction &transaction) { std::unique_ptr item(TestType::init(t)); assert(item.get() != nullptr); - return TestType::eval(*item.get(), t); + return TestType::eval(*item.get(), t, transaction); } template -UnitTestResult perform_unit_test_multithreaded(const UnitTest &t) { +UnitTestResult perform_unit_test_multithreaded(const UnitTest &t, modsecurity::Transaction &transaction) { constexpr auto NUM_THREADS = 50; constexpr auto ITERATIONS = 5'000; @@ -126,10 +129,10 @@ UnitTestResult perform_unit_test_multithreaded(const UnitTest &t) { { auto &result = results[i]; threads[i] = std::thread( - [&item, &t, &result]() + [&item, &t, &result, &transaction]() { for (auto j = 0; j != ITERATIONS; ++j) - result = TestType::eval(*item.get(), t); + result = TestType::eval(*item.get(), t, transaction); }); } @@ -150,12 +153,12 @@ UnitTestResult perform_unit_test_multithreaded(const UnitTest &t) { template void perform_unit_test_helper(const ModSecurityTest &test, UnitTest &t, - ModSecurityTestResults &res) { + ModSecurityTestResults &res, modsecurity::Transaction &transaction) { if (!test.m_test_multithreaded) - t.result = perform_unit_test_once(t); + t.result = perform_unit_test_once(t, transaction); else - t.result = perform_unit_test_multithreaded(t); + t.result = perform_unit_test_multithreaded(t, transaction); if (TestType::check(t.result, t)) { res.push_back(&t); @@ -172,6 +175,11 @@ void perform_unit_test(const ModSecurityTest &test, UnitTest &t, ModSecurityTestResults &res) { bool found = true; + modsecurity_test::ModSecurityTestContext context("ModSecurity-unit v0.0.1-alpha" + " (ModSecurity unit test utility)"); + + auto transaction = context.create_transaction(); + if (test.m_automake_output) { std::cout << ":test-result: "; } @@ -190,9 +198,9 @@ void perform_unit_test(const ModSecurityTest &test, UnitTest &t, } if (t.type == "op") { - perform_unit_test_helper(test, t, res); + perform_unit_test_helper(test, t, res, transaction); } else if (t.type == "tfn") { - perform_unit_test_helper(test, t, res); + perform_unit_test_helper(test, t, res, transaction); } else { std::cerr << "Failed. Test type is unknown: << " << t.type; std::cerr << std::endl; From b7b2d9a40d779c41ba316fed93838620cadbcdb2 Mon Sep 17 00:00:00 2001 From: Eduardo Arias Date: Tue, 10 Sep 2024 14:47:00 -0300 Subject: [PATCH 3/4] Minor codebase improvements suggested by Sonarcloud - src/modsecurity.cc - Replace the redundant type with "auto". - src/transaction.cc - Avoid this unnecessary copy by using a "const" reference. - test/common/custom_debug_log.cc - Use "=default" instead of the default implementation of this special member functions. - Removed the unnecessary destructor override instead. - Annotate this function with "override" or "final". - Removed the unnecessary destructor override instead. - Remove this "const" qualifier from the return type in all declarations. - test/common/modsecurity_test_context.h - Replace the redundant type with "auto". - test/regression/regression.cc - Use the "nullptr" literal. - Replace this declaration by a structured binding declaration. - Replace "reinterpret_cast" with a safer operation. --- src/modsecurity.cc | 4 ++-- src/transaction.cc | 2 +- test/common/custom_debug_log.cc | 6 ++---- test/common/custom_debug_log.h | 5 ++--- test/common/modsecurity_test_context.h | 4 ++-- test/regression/regression.cc | 26 +++++++++++++------------- 6 files changed, 22 insertions(+), 25 deletions(-) diff --git a/src/modsecurity.cc b/src/modsecurity.cc index e1b29857e1..8f943b7f76 100644 --- a/src/modsecurity.cc +++ b/src/modsecurity.cc @@ -199,13 +199,13 @@ void ModSecurity::serverLog(void *data, const RuleMessage &rm) { if (m_logProperties & TextLogProperty) { auto d = rm.log(); - const void *a = static_cast(d.c_str()); + auto a = static_cast(d.c_str()); m_logCb(data, a); return; } if (m_logProperties & RuleMessageLogProperty) { - const void *a = static_cast(&rm); + auto a = static_cast(&rm); m_logCb(data, a); return; } diff --git a/src/transaction.cc b/src/transaction.cc index 1b055ad084..1892252310 100644 --- a/src/transaction.cc +++ b/src/transaction.cc @@ -1529,7 +1529,7 @@ std::string Transaction::toOldAuditLogFormat(int parts, } if (parts & audit_log::AuditLog::HAuditLogPart) { audit_log << "--" << trailer << "-" << "H--" << std::endl; - for (auto a : m_rulesMessages) { + for (const auto &a : m_rulesMessages) { audit_log << a.log(0, m_httpCodeReturned) << std::endl; } audit_log << std::endl; diff --git a/test/common/custom_debug_log.cc b/test/common/custom_debug_log.cc index 7f69b58c1c..fbf97ab9dc 100644 --- a/test/common/custom_debug_log.cc +++ b/test/common/custom_debug_log.cc @@ -23,8 +23,6 @@ namespace modsecurity_test { - CustomDebugLog::~CustomDebugLog() {} - void CustomDebugLog::write(int level, const std::string &message) { m_log << "[" << level << "] " << message << std::endl; } @@ -36,13 +34,13 @@ namespace modsecurity_test { m_log << msgf << std::endl; } - bool const CustomDebugLog::contains(const std::string &pattern) const { + bool 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 { + std::string CustomDebugLog::log_messages() const { return m_log.str(); } diff --git a/test/common/custom_debug_log.h b/test/common/custom_debug_log.h index f1868acd6e..92857d1006 100644 --- a/test/common/custom_debug_log.h +++ b/test/common/custom_debug_log.h @@ -26,13 +26,12 @@ namespace modsecurity_test { class CustomDebugLog : public modsecurity::debug_log::DebugLog { public: CustomDebugLog *new_instance(); - ~CustomDebugLog(); void write(int level, const std::string& message) override; void write(int level, const std::string &id, const std::string &uri, const std::string &msg) override; - bool const contains(const std::string& pattern) const; - std::string const log_messages() const; + bool contains(const std::string& pattern) const; + std::string log_messages() const; std::string error_log_messages(); int getDebugLogLevel() override; diff --git a/test/common/modsecurity_test_context.h b/test/common/modsecurity_test_context.h index 46461cb2bd..cd99e23ab9 100644 --- a/test/common/modsecurity_test_context.h +++ b/test/common/modsecurity_test_context.h @@ -31,8 +31,8 @@ namespace modsecurity_test { private: static void logCb(void *data, const void *msgv) { - const char *msg = reinterpret_cast(msgv); - std::stringstream *ss = (std::stringstream *)data; + auto msg = reinterpret_cast(msgv); + auto ss = reinterpret_cast(data); *ss << msg << std::endl; } }; diff --git a/test/regression/regression.cc b/test/regression/regression.cc index 5992d2cf47..62ab1e8919 100644 --- a/test/regression/regression.cc +++ b/test/regression/regression.cc @@ -97,15 +97,15 @@ void actions(ModSecurityTestResults *r, if (it.status != 0) { r->status = it.status; } - if (it.url != NULL) { + if (it.url != nullptr) { r->location.append(it.url); free(it.url); - it.url = NULL; + it.url = nullptr; } - if (it.log != NULL) { + if (it.log != nullptr) { *serverLog << it.log; free(it.log); - it.log = NULL; + it.log = nullptr; } } } @@ -283,9 +283,9 @@ void perform_unit_test(ModSecurityTest *test, actions(&r, &modsec_transaction, &context.m_server_log); - for (const auto &headers : t->request_headers) { - modsec_transaction.addRequestHeader(headers.first.c_str(), - headers.second.c_str()); + for (const auto &[name, value] : t->request_headers) { + modsec_transaction.addRequestHeader(name.c_str(), + value.c_str()); } modsec_transaction.processRequestHeaders(); @@ -297,9 +297,9 @@ void perform_unit_test(ModSecurityTest *test, modsec_transaction.processRequestBody(); actions(&r, &modsec_transaction, &context.m_server_log); - for (const auto &headers : t->response_headers) { - modsec_transaction.addResponseHeader(headers.first.c_str(), - headers.second.c_str()); + for (const auto &[name, value] : t->response_headers) { + modsec_transaction.addResponseHeader(name.c_str(), + value.c_str()); } modsec_transaction.processResponseHeaders(r.status, @@ -314,7 +314,7 @@ void perform_unit_test(ModSecurityTest *test, modsec_transaction.processLogging(); - const auto *d = reinterpret_cast(context.m_modsec_rules.m_debugLog); + const auto *d = static_cast(context.m_modsec_rules.m_debugLog); if (!d->contains(t->debug_log)) { if (test->m_automake_output) { @@ -455,8 +455,8 @@ int main(int argc, char **argv) int counter = 0; std::list keyList; - for (const auto &a : test) { - keyList.push_back(a.first); + for (const auto &[name, tests] : test) { + keyList.push_back(name); } keyList.sort(); From 75d31a4d1ecc6f047820fac0ad0bd5d594bf0c99 Mon Sep 17 00:00:00 2001 From: Eduardo Arias Date: Tue, 10 Sep 2024 14:32:38 -0300 Subject: [PATCH 4/4] Simplified lifetime management of tests - Addresses Sonarcloud issues: - Rewrite the code so that you no longer need this "delete". - Make the type of this variable a reference-to-const. --- test/common/modsecurity_test.cc | 13 +++---------- test/common/modsecurity_test.h | 2 +- test/regression/regression.cc | 21 +++++++-------------- test/unit/unit.cc | 11 ++--------- 4 files changed, 13 insertions(+), 34 deletions(-) diff --git a/test/common/modsecurity_test.cc b/test/common/modsecurity_test.cc index 0f769054e8..1e0585d475 100644 --- a/test/common/modsecurity_test.cc +++ b/test/common/modsecurity_test.cc @@ -72,18 +72,11 @@ bool ModSecurityTest::load_test_json(const std::string &file) { for ( int i = 0; i < num_tests; i++ ) { yajl_val obj = node->u.array.values[i]; - T *u = T::from_yajl_node(obj); + auto u = std::unique_ptr(T::from_yajl_node(obj)); u->filename = file; - if (this->count(u->filename + ":" + u->name) == 0) { - auto vec = new std::vector; - vec->push_back(u); - std::string filename(u->filename + ":" + u->name); - this->insert({filename, vec}); - } else { - auto vec = this->at(u->filename + ":" + u->name); - vec->push_back(u); - } + const auto key = u->filename + ":" + u->name; + (*this)[key].push_back(std::move(u)); } yajl_tree_free(node); diff --git a/test/common/modsecurity_test.h b/test/common/modsecurity_test.h index 8b55a16c62..e7a8b1b3e5 100644 --- a/test/common/modsecurity_test.h +++ b/test/common/modsecurity_test.h @@ -29,7 +29,7 @@ extern std::string default_test_path; namespace modsecurity_test { template class ModSecurityTest : - public std::unordered_map *> { + public std::unordered_map>> { public: ModSecurityTest() : m_test_number(0), diff --git a/test/regression/regression.cc b/test/regression/regression.cc index 62ab1e8919..6d7b9dc3a1 100644 --- a/test/regression/regression.cc +++ b/test/regression/regression.cc @@ -111,13 +111,14 @@ void actions(ModSecurityTestResults *r, } void perform_unit_test(ModSecurityTest *test, - std::vector *tests, - ModSecurityTestResults *res, int *count) { - for (RegressionTest *t : *tests) { + std::vector> &tests, + ModSecurityTestResults *res, int *count) +{ + for (auto &t : tests) { ModSecurityTestResults r; RegressionTestResult *testRes = new RegressionTestResult(); - testRes->test = t; + testRes->test = t.get(); r.status = 200; (*count)++; @@ -468,9 +469,9 @@ int main(int argc, char **argv) ModSecurityTestResults res; for (const std::string &a : keyList) { test_number++; - if ((test.m_test_number == 0) + if ((test.m_test_number == 0) || (test_number == test.m_test_number)) { - std::vector *tests = test[a]; + auto &tests = test[a]; perform_unit_test(&test, tests, &res, &counter); } } @@ -523,14 +524,6 @@ int main(int argc, char **argv) std::cout << "disabled test(s)." << RESET << std::endl; } - for (auto a : test) { - std::vector *vec = a.second; - for (int i = 0; i < vec->size(); i++) { - delete vec->at(i); - } - delete vec; - } - return failed; #endif } diff --git a/test/unit/unit.cc b/test/unit/unit.cc index d1b871c6cd..a1c6b0b3c6 100644 --- a/test/unit/unit.cc +++ b/test/unit/unit.cc @@ -250,8 +250,8 @@ int main(int argc, char **argv) { } for (auto& [filename, tests] : test) { - total += tests->size(); - for (auto t : *tests) { + total += tests.size(); + for (auto &t : tests) { ModSecurityTestResults r; if (!test.m_automake_output) { @@ -311,12 +311,5 @@ int main(int argc, char **argv) { } } - for (auto a : test) { - auto vec = a.second; - for(auto t : *vec) - delete t; - delete vec; - } - return failed; }