From 99190266201fe5e18a64e3c35d1c5adc273ff238 Mon Sep 17 00:00:00 2001 From: Felipe Zimmerle Date: Wed, 15 Jun 2016 23:52:26 -0300 Subject: [PATCH] Fixes regarding memory management Fixes assorted issues identified by valgrind. --- Makefile.am | 2 +- build/ax_valgrind_check.m4 | 2 +- headers/modsecurity/collection/collection.h | 1 + headers/modsecurity/debug_log.h | 2 +- headers/modsecurity/rules_properties.h | 2 +- src/collection/collections.cc | 1 + src/debug_log_writer.cc | 3 +- src/debug_log_writer_agent.cc | 1 - src/debug_log_writer_agent.h | 1 - src/modsecurity.cc | 4 +++ src/operators/ip_match.cc | 4 +-- src/operators/ip_match.h | 2 +- src/operators/ip_match_from_file.cc | 6 ++-- src/operators/ip_match_from_file.h | 2 +- src/operators/operator.h | 2 +- src/operators/pm.cc | 5 +-- src/operators/pm.h | 2 +- src/operators/pm_from_file.cc | 8 ++--- src/operators/pm_from_file.h | 2 +- src/operators/rx.h | 3 ++ src/operators/validate_byte_range.cc | 26 +++++++------- src/operators/validate_byte_range.h | 4 +-- src/operators/validate_dtd.cc | 5 ++- src/operators/validate_dtd.h | 2 +- src/operators/validate_schema.cc | 5 ++- src/operators/validate_schema.h | 2 +- src/operators/verify_cc.cc | 4 +-- src/operators/verify_cc.h | 2 +- src/parser/seclang-parser.yy | 6 ++-- src/request_body_processor/multipart.cc | 19 +++++++++-- src/request_body_processor/multipart.h | 7 +++- src/rule.cc | 1 + src/rules.cc | 13 +++++++ src/transaction.cc | 2 ++ src/utils/acmp.cc | 2 +- src/utils/ip_tree.cc | 28 +++++++++------ src/utils/ip_tree.h | 3 +- src/utils/msc_tree.cc | 38 +++++++++++++++++++++ src/utils/msc_tree.h | 1 + src/utils/regex.cc | 13 +++++++ src/utils/regex.h | 1 + test/regression/custom_debug_log.cc | 2 +- test/regression/custom_debug_log.h | 1 + test/regression/regression.cc | 28 ++++++++++++++- test/unit/unit.cc | 2 +- test/valgrind_suppressions.txt | 35 +++++++++++++++++++ 46 files changed, 234 insertions(+), 73 deletions(-) diff --git a/Makefile.am b/Makefile.am index c6e51b3a..b8286ef2 100644 --- a/Makefile.am +++ b/Makefile.am @@ -37,7 +37,7 @@ MAINTAINERCLEANFILES = \ depcomp @VALGRIND_CHECK_RULES@ -VALGRIND_SUPPRESSIONS_FILES = test/valgrind_suppressions.txt +VALGRIND_SUPPRESSIONS_FILES = valgrind_suppressions.txt LOG_DRIVER = env $(SHELL) $(top_srcdir)/test/custom-test-driver AM_TESTS_ENVIRONMENT=AUTOMAKE_TESTS=true; export AUTOMAKE_TESTS; diff --git a/build/ax_valgrind_check.m4 b/build/ax_valgrind_check.m4 index 20ec74bb..cb3890ec 100644 --- a/build/ax_valgrind_check.m4 +++ b/build/ax_valgrind_check.m4 @@ -201,7 +201,7 @@ VALGRIND_TESTS_ENVIRONMENT = \ G_SLICE=always-malloc,debug-blocks \ G_DEBUG=fatal-warnings,fatal-criticals,gc-friendly -VALGRIND_LOG_COMPILER = test/test-suite.sh +VALGRIND_LOG_COMPILER = test/test-suite.sh $(VALGRIND_SUPPRESSIONS) $(VALGRIND_FLAGS) # $(valgrind_lt) \ # $(VALGRIND) $(VALGRIND_SUPPRESSIONS) --error-exitcode=1 $(VALGRIND_FLAGS) diff --git a/headers/modsecurity/collection/collection.h b/headers/modsecurity/collection/collection.h index 0e9e60de..8ba0b46f 100644 --- a/headers/modsecurity/collection/collection.h +++ b/headers/modsecurity/collection/collection.h @@ -39,6 +39,7 @@ namespace collection { class Collection { public: + virtual ~Collection() { }; virtual void store(std::string key, std::string value) = 0; virtual bool storeOrUpdateFirst(const std::string &key, diff --git a/headers/modsecurity/debug_log.h b/headers/modsecurity/debug_log.h index b9b2738e..f700eae7 100644 --- a/headers/modsecurity/debug_log.h +++ b/headers/modsecurity/debug_log.h @@ -36,7 +36,7 @@ class DebugLog { : m_debugLevel(-1), m_fileName("") { } - ~DebugLog(); + virtual ~DebugLog(); virtual void write(int level, const std::string &msg); bool isLogFileSet(); diff --git a/headers/modsecurity/rules_properties.h b/headers/modsecurity/rules_properties.h index 7e7fb10a..7f26a0c2 100644 --- a/headers/modsecurity/rules_properties.h +++ b/headers/modsecurity/rules_properties.h @@ -103,7 +103,7 @@ class RulesProperties { */ ~RulesProperties() { delete m_debugLog; - } + }; std::vector rules[7]; std::vector * getRulesForPhase(int phase) { diff --git a/src/collection/collections.cc b/src/collection/collections.cc index de6d91d5..6a907114 100644 --- a/src/collection/collections.cc +++ b/src/collection/collections.cc @@ -51,6 +51,7 @@ Collections::~Collections() { for (const auto &thing : *this) { delete thing.second; } + delete m_transient; this->clear(); } diff --git a/src/debug_log_writer.cc b/src/debug_log_writer.cc index 89ffe74e..69e0bdab 100644 --- a/src/debug_log_writer.cc +++ b/src/debug_log_writer.cc @@ -40,17 +40,16 @@ void DebugLogWriter::open(const std::string& fileName) { void DebugLogWriter::close(const std::string& fileName) { -#if 0 std::map::iterator it; DebugLogWriterAgent *agent; it = agents.find(fileName); if (it != agents.end()) { agent = it->second; if (agent->refCountDecreaseAndCheck()) { + delete agent; agents.erase(it); } } -#endif } diff --git a/src/debug_log_writer_agent.cc b/src/debug_log_writer_agent.cc index d55ce718..050a3a97 100644 --- a/src/debug_log_writer_agent.cc +++ b/src/debug_log_writer_agent.cc @@ -33,7 +33,6 @@ DebugLogWriterAgent::DebugLogWriterAgent(const std::string& fileName) : void DebugLogWriterAgent::write(const std::string& msg) { if (!is_open()) { - std::cout << "Agent: " << m_fileName << ": " << msg << std::endl; return; } diff --git a/src/debug_log_writer_agent.h b/src/debug_log_writer_agent.h index 5394c585..ae4efc0e 100644 --- a/src/debug_log_writer_agent.h +++ b/src/debug_log_writer_agent.h @@ -36,7 +36,6 @@ class DebugLogWriterAgent : public std::ofstream { bool refCountDecreaseAndCheck() { this->m_referenceCount--; if (this->m_referenceCount == 0) { - delete this; return true; } return false; diff --git a/src/modsecurity.cc b/src/modsecurity.cc index 13726768..5b8498a1 100644 --- a/src/modsecurity.cc +++ b/src/modsecurity.cc @@ -66,6 +66,10 @@ ModSecurity::~ModSecurity() { #ifdef WITH_GEOIP Utils::GeoLookup::getInstance().cleanUp(); #endif + delete m_global_collection; + delete m_ip_collection; + delete m_session_collection; + delete m_user_collection; } diff --git a/src/operators/ip_match.cc b/src/operators/ip_match.cc index 75fd2ed4..af8e3288 100644 --- a/src/operators/ip_match.cc +++ b/src/operators/ip_match.cc @@ -25,12 +25,12 @@ namespace modsecurity { namespace operators { -bool IpMatch::init(const std::string &file, const char **error) { +bool IpMatch::init(const std::string &file, std::string *error) { std::string e(""); bool res = m_tree.addFromBuffer(param, &e); if (res == false) { - *error = e.c_str(); + error->assign(e); } return res; diff --git a/src/operators/ip_match.h b/src/operators/ip_match.h index 3f03ef40..caaba167 100644 --- a/src/operators/ip_match.h +++ b/src/operators/ip_match.h @@ -33,7 +33,7 @@ class IpMatch : public Operator { bool evaluate(Transaction *transaction, const std::string &input) override; - bool init(const std::string &file, const char **error) override; + bool init(const std::string &file, std::string *error) override; protected: Utils::IpTree m_tree; diff --git a/src/operators/ip_match_from_file.cc b/src/operators/ip_match_from_file.cc index e312b547..3b3c4920 100644 --- a/src/operators/ip_match_from_file.cc +++ b/src/operators/ip_match_from_file.cc @@ -15,6 +15,8 @@ #include "operators/ip_match_from_file.h" +#include + #include #include "operators/operator.h" @@ -24,7 +26,7 @@ namespace operators { bool IpMatchFromFile::init(const std::string &file, - const char **error) { + std::string *error) { std::string e(""); bool res = false; @@ -35,7 +37,7 @@ bool IpMatchFromFile::init(const std::string &file, } if (res == false) { - *error = e.c_str(); + error->assign(e); } return res; diff --git a/src/operators/ip_match_from_file.h b/src/operators/ip_match_from_file.h index 3aa5e6dd..d0eeb1db 100644 --- a/src/operators/ip_match_from_file.h +++ b/src/operators/ip_match_from_file.h @@ -29,7 +29,7 @@ class IpMatchFromFile : public IpMatch { IpMatchFromFile(std::string op, std::string param, bool negation) : IpMatch(op, param, negation) { } - bool init(const std::string& file, const char **error) override; + bool init(const std::string& file, std::string *error) override; }; } // namespace operators diff --git a/src/operators/operator.h b/src/operators/operator.h index 07492015..a422fd18 100644 --- a/src/operators/operator.h +++ b/src/operators/operator.h @@ -40,7 +40,7 @@ class Operator { std::string param; bool negation; - virtual bool init(const std::string &file, const char **error) { + virtual bool init(const std::string &file, std::string *error) { return true; } diff --git a/src/operators/pm.cc b/src/operators/pm.cc index 351429fe..65d16912 100644 --- a/src/operators/pm.cc +++ b/src/operators/pm.cc @@ -85,13 +85,14 @@ bool Pm::evaluate(Transaction *transaction, const std::string &input) { } -bool Pm::init(const std::string &file, const char **error) { +bool Pm::init(const std::string &file, std::string *error) { std::vector vec; std::istringstream *iss; + const char *err = NULL; replaceAll(param, "\\", "\\\\"); - char *content = parse_pm_content(param.c_str(), param.length(), error); + char *content = parse_pm_content(param.c_str(), param.length(), &err); if (content == NULL) { iss = new std::istringstream(param); } else { diff --git a/src/operators/pm.h b/src/operators/pm.h index 7f5def2c..7a882157 100644 --- a/src/operators/pm.h +++ b/src/operators/pm.h @@ -39,7 +39,7 @@ class Pm : public Operator { const std::string& to); bool evaluate(Transaction *transaction, const std::string &input) override; - bool init(const std::string &file, const char **error) override; + bool init(const std::string &file, std::string *error) override; void postOrderTraversal(acmp_btree_node_t *node); protected: diff --git a/src/operators/pm_from_file.cc b/src/operators/pm_from_file.cc index 42751507..4aac167c 100644 --- a/src/operators/pm_from_file.cc +++ b/src/operators/pm_from_file.cc @@ -25,14 +25,14 @@ namespace modsecurity { namespace operators { -bool PmFromFile::init(const std::string &config, const char **error) { +bool PmFromFile::init(const std::string &config, std::string *error) { std::istream *iss; if (param.compare(0, 8, "https://") == 0) { Utils::HttpsClient client; bool ret = client.download(param); if (ret == false) { - *error = client.error.c_str(); + error->assign(client.error); return false; } iss = new std::stringstream(client.content); @@ -41,7 +41,7 @@ bool PmFromFile::init(const std::string &config, const char **error) { iss = new std::ifstream(resource, std::ios::in); if (((std::ifstream *)iss)->is_open() == false) { - *error = std::string("Failed to open file: " + param).c_str(); + error->assign("Failed to open file: " + param); delete iss; return false; } @@ -51,7 +51,7 @@ bool PmFromFile::init(const std::string &config, const char **error) { acmp_add_pattern(m_p, line.c_str(), NULL, NULL, line.length()); } - acmp_prepare(m_p); + //acmp_prepare(m_p); delete iss; return true; diff --git a/src/operators/pm_from_file.h b/src/operators/pm_from_file.h index fcd7d0d8..85f86f99 100644 --- a/src/operators/pm_from_file.h +++ b/src/operators/pm_from_file.h @@ -31,7 +31,7 @@ class PmFromFile : public Pm { PmFromFile(std::string op, std::string param, bool negation) : Pm(op, param, negation) { } - bool init(const std::string &file, const char **error) override; + bool init(const std::string &file, std::string *error) override; }; diff --git a/src/operators/rx.h b/src/operators/rx.h index f56713fc..6511bd61 100644 --- a/src/operators/rx.h +++ b/src/operators/rx.h @@ -40,6 +40,9 @@ class Rx : public Operator { m_re = new Regex(param); } + ~Rx() { + delete m_re; + } bool evaluate(Transaction *transaction, const std::string &input); private: diff --git a/src/operators/validate_byte_range.cc b/src/operators/validate_byte_range.cc index cffd4ee3..d3d2762d 100644 --- a/src/operators/validate_byte_range.cc +++ b/src/operators/validate_byte_range.cc @@ -23,7 +23,7 @@ namespace modsecurity { namespace operators { bool ValidateByteRange::getRange(const std::string &rangeRepresentation, - const char **error) { + std::string *error) { size_t pos = param.find_first_of("-"); int start; int end; @@ -32,8 +32,8 @@ bool ValidateByteRange::getRange(const std::string &rangeRepresentation, try { start = std::stoi(rangeRepresentation); } catch(...) { - *error = ("Not able to convert '" + rangeRepresentation + - "' into a number").c_str(); + error->assign("Not able to convert '" + rangeRepresentation + + "' into a number"); return false; } table[start >> 3] = (table[start >> 3] | (1 << (start & 0x7))); @@ -43,9 +43,9 @@ bool ValidateByteRange::getRange(const std::string &rangeRepresentation, try { start = std::stoi(std::string(rangeRepresentation, 0, pos)); } catch (...) { - *error = ("Not able to convert '" + + error->assign("Not able to convert '" + std::string(rangeRepresentation, 0, pos) + - "' into a number").c_str(); + "' into a number"); return false; } @@ -53,24 +53,24 @@ bool ValidateByteRange::getRange(const std::string &rangeRepresentation, end = std::stoi(std::string(rangeRepresentation, pos + 1, rangeRepresentation.length() - (pos + 1))); } catch (...) { - *error = ("Not able to convert '" + std::string(rangeRepresentation, + error->assign("Not able to convert '" + std::string(rangeRepresentation, pos + 1, rangeRepresentation.length() - (pos + 1)) + - "' into a number").c_str(); + "' into a number"); return false; } if ((start < 0) || (start > 255)) { - *error = ("Invalid range start value: " + - std::to_string(start)).c_str(); + error->assign("Invalid range start value: " + + std::to_string(start)); return false; } if ((end < 0) || (end > 255)) { - *error = ("Invalid range end value: " + std::to_string(end)).c_str(); + error->assign("Invalid range end value: " + std::to_string(end)); return false; } if (start > end) { - *error = ("Invalid range: " + std::to_string(start) + "-" + - std::to_string(end)).c_str(); + error->assign("Invalid range: " + std::to_string(start) + "-" + + std::to_string(end)); return false; } @@ -84,7 +84,7 @@ bool ValidateByteRange::getRange(const std::string &rangeRepresentation, bool ValidateByteRange::init(const std::string &file, - const char **error) { + std::string *error) { size_t pos = param.find_first_of(","); if (pos == std::string::npos) { diff --git a/src/operators/validate_byte_range.h b/src/operators/validate_byte_range.h index 9649c4af..05d85b85 100644 --- a/src/operators/validate_byte_range.h +++ b/src/operators/validate_byte_range.h @@ -37,8 +37,8 @@ class ValidateByteRange : public Operator { ~ValidateByteRange() override { } bool evaluate(Transaction *transaction, const std::string &input) override; - bool getRange(const std::string &rangeRepresentation, const char **error); - bool init(const std::string& file, const char **error) override; + bool getRange(const std::string &rangeRepresentation, std::string *error); + bool init(const std::string& file, std::string *error) override; private: std::vector ranges; char table[32]; diff --git a/src/operators/validate_dtd.cc b/src/operators/validate_dtd.cc index c857eebd..11d918db 100644 --- a/src/operators/validate_dtd.cc +++ b/src/operators/validate_dtd.cc @@ -25,11 +25,10 @@ namespace modsecurity { namespace operators { -bool ValidateDTD::init(const std::string &file, const char **error) { +bool ValidateDTD::init(const std::string &file, std::string *error) { m_resource = find_resource(param, file); if (m_resource == "") { - std::string f("XML: File not found: " + param + "."); - *error = strdup(f.c_str()); + error->assign("XML: File not found: " + param + "."); return false; } diff --git a/src/operators/validate_dtd.h b/src/operators/validate_dtd.h index 3a47a71f..453de896 100644 --- a/src/operators/validate_dtd.h +++ b/src/operators/validate_dtd.h @@ -44,7 +44,7 @@ class ValidateDTD : public Operator { } bool evaluate(Transaction *transaction, const std::string &str) override; - bool init(const std::string &file, const char **error) override; + bool init(const std::string &file, std::string *error) override; static void error_runtime(void *ctx, const char *msg, ...) { diff --git a/src/operators/validate_schema.cc b/src/operators/validate_schema.cc index 815edd2d..7dd9e4c6 100644 --- a/src/operators/validate_schema.cc +++ b/src/operators/validate_schema.cc @@ -25,11 +25,10 @@ namespace modsecurity { namespace operators { -bool ValidateSchema::init(const std::string &file, const char **error) { +bool ValidateSchema::init(const std::string &file, std::string *error) { m_resource = find_resource(param, file); if (m_resource == "") { - std::string f("XML: File not found: " + param + "."); - *error = strdup(f.c_str()); + error->assign("XML: File not found: " + param + "."); return false; } diff --git a/src/operators/validate_schema.h b/src/operators/validate_schema.h index be54af84..ef3e3580 100644 --- a/src/operators/validate_schema.h +++ b/src/operators/validate_schema.h @@ -52,7 +52,7 @@ class ValidateSchema : public Operator { } bool evaluate(Transaction *transaction, const std::string &str) override; - bool init(const std::string &file, const char **error) override; + bool init(const std::string &file, std::string *error) override; static void error_load(void *ctx, const char *msg, ...) { diff --git a/src/operators/verify_cc.cc b/src/operators/verify_cc.cc index fbeab04f..6c914245 100644 --- a/src/operators/verify_cc.cc +++ b/src/operators/verify_cc.cc @@ -69,7 +69,7 @@ int VerifyCC::luhnVerify(const char *ccnumber, int len) { -bool VerifyCC::init(const std::string ¶m2, const char **error) { +bool VerifyCC::init(const std::string ¶m2, std::string *error) { const char *errptr = NULL; int erroffset = 0; @@ -78,7 +78,7 @@ bool VerifyCC::init(const std::string ¶m2, const char **error) { m_pce = pcre_study(m_pc, PCRE_STUDY_JIT_COMPILE, &errptr); if ((m_pc == NULL) || (m_pce == NULL)) { - *error = errptr; + error->assign(errptr); return false; } diff --git a/src/operators/verify_cc.h b/src/operators/verify_cc.h index d4f714b1..d3366dfb 100644 --- a/src/operators/verify_cc.h +++ b/src/operators/verify_cc.h @@ -34,7 +34,7 @@ class VerifyCC : public Operator { int luhnVerify(const char *ccnumber, int len); bool evaluate(Transaction *transaction, const std::string &input) override; - bool init(const std::string ¶m, const char **error) override; + bool init(const std::string ¶m, std::string *error) override; private: pcre *m_pc; pcre_extra *m_pce; diff --git a/src/parser/seclang-parser.yy b/src/parser/seclang-parser.yy index 4627d901..d372398c 100644 --- a/src/parser/seclang-parser.yy +++ b/src/parser/seclang-parser.yy @@ -429,7 +429,7 @@ op: OPERATOR { Operator *op = Operator::instantiate($1); - const char *error = NULL; + std::string error; if (op->init(driver.ref.back(), &error) == false) { driver.error(@0, error); YYERROR; @@ -440,7 +440,7 @@ op: { #ifdef WITH_GEOIP Operator *op = Operator::instantiate($1); - const char *error = NULL; + std::string error; if (op->init(driver.ref.back(), &error) == false) { driver.error(@0, error); YYERROR; @@ -459,7 +459,7 @@ op: text.pop_back(); text.erase(0, 1); Operator *op = Operator::instantiate("\"@rx " + text + "\""); - const char *error = NULL; + std::string error; if (op->init(driver.ref.back(), &error) == false) { driver.error(@0, error); YYERROR; diff --git a/src/request_body_processor/multipart.cc b/src/request_body_processor/multipart.cc index e099b9e9..1159e170 100644 --- a/src/request_body_processor/multipart.cc +++ b/src/request_body_processor/multipart.cc @@ -92,9 +92,17 @@ Multipart::~Multipart() { } } - while (!m_parts.empty()) { - m_parts.pop_front(); + while (m_parts.empty() == false) { + auto *a = m_parts.back(); + m_parts.pop_back(); + delete a; } + + if (m_mpp != NULL) { + delete m_mpp; + m_mpp = NULL; + } + } @@ -734,9 +742,13 @@ int Multipart::process_part_header() { return false; } - m_mpp->m_headers.insert({header_name, header_value}); m_mpp->m_last_header_name.assign(header_name); + + m_mpp->m_headers.emplace( + std::string(header_name), std::string(header_value)); + + debug(9, "Multipart: Added part header \"" + header_name \ + "\" \"" + header_value + "\"."); } @@ -784,6 +796,7 @@ int Multipart::process_boundary(int last_part) { debug(3, "Multipart: Skipping invalid part (part name missing): " "(offset " + std::to_string(m_mpp->m_offset) + ", length " + std::to_string(m_mpp->m_length) + ")"); + delete m_mpp; } m_mpp = NULL; diff --git a/src/request_body_processor/multipart.h b/src/request_body_processor/multipart.h index 34e9452a..bdce8361 100644 --- a/src/request_body_processor/multipart.h +++ b/src/request_body_processor/multipart.h @@ -54,13 +54,18 @@ struct MyEqual { class MultipartPart { public: - MultipartPart() + MultipartPart() : m_type(MULTIPART_FORMDATA), m_tmp_file_fd(0), m_tmp_file_size(0), m_offset(0), m_length(0) { } + ~MultipartPart () { + m_headers.clear(); + m_value_parts.clear(); + } + /* part type, can be MULTIPART_FORMDATA or MULTIPART_FILE */ int m_type; diff --git a/src/rule.cc b/src/rule.cc index c8a7f61d..12b47b7a 100644 --- a/src/rule.cc +++ b/src/rule.cc @@ -309,6 +309,7 @@ bool Rule::evaluate(Transaction *trasn) { variable->evaluateInternal(trasn, this, &z); for (auto &y : z) { exclusions.push_back(y->m_key); + delete y; } exclusions.push_back(variable->m_name); } diff --git a/src/rules.cc b/src/rules.cc index 85a075e3..6274b528 100644 --- a/src/rules.cc +++ b/src/rules.cc @@ -78,6 +78,8 @@ void Rules::decrementReferenceCount(void) { Rules::~Rules() { + int i = 0; + /** Cleanup the rules */ for (int i = 0; i < ModSecurity::Phases::NUMBER_OF_PHASES; i++) { std::vector rules = this->rules[i]; @@ -87,10 +89,20 @@ Rules::~Rules() { rules.pop_back(); } } + for (i = 0; i < ModSecurity::Phases::NUMBER_OF_PHASES; i++) { + std::vector *tmp = &defaultActions[i]; + while (tmp->empty() == false) { + actions::Action *a = tmp->back(); + tmp->pop_back(); + delete a; + } + } /** Cleanup audit log */ if (audit_log) { audit_log->refCountDecreaseAndCheck(); } + + free(unicode_map_table); } @@ -128,6 +140,7 @@ int Rules::load(const char *file, const std::string &ref) { if (driver->parse(file, ref) == false) { parserError << driver->parserError.str(); + delete driver; return -1; } int rules = this->merge(driver); diff --git a/src/transaction.cc b/src/transaction.cc index 866fab5e..c296755c 100644 --- a/src/transaction.cc +++ b/src/transaction.cc @@ -1395,6 +1395,7 @@ std::string Transaction::toJSON(int parts) { m_collections.m_transient->resolveMultiMatches("REQUEST_HEADERS", &l); for (auto h : l) { LOGFY_ADD(h->m_key.c_str(), h->m_value.c_str()); + delete h; } /* end: request headers */ @@ -1424,6 +1425,7 @@ std::string Transaction::toJSON(int parts) { m_collections.m_transient->resolveMultiMatches("RESPONSE_HEADERS", &l); for (auto h : l) { LOGFY_ADD(h->m_key.c_str(), h->m_value.c_str()); + delete h; } /* end: response headers */ diff --git a/src/utils/acmp.cc b/src/utils/acmp.cc index 82f691f4..f714d496 100644 --- a/src/utils/acmp.cc +++ b/src/utils/acmp.cc @@ -569,4 +569,4 @@ int acmp_process_quick(ACMPT *acmpt, const char **match, const char *data, size_ return 0; } -} \ No newline at end of file +} diff --git a/src/utils/ip_tree.cc b/src/utils/ip_tree.cc index ad425a03..f811de55 100644 --- a/src/utils/ip_tree.cc +++ b/src/utils/ip_tree.cc @@ -39,50 +39,57 @@ void IpTree::postOrderTraversal(TreeNode *node) { postOrderTraversal(node->right); if (node->netmasks) { - delete node->netmasks; + free(node->netmasks); node->netmasks = NULL; } if (node->prefix) { if (node->prefix->buffer) { - delete node->prefix->buffer; + free(node->prefix->buffer); node->prefix->buffer = NULL; } if (node->prefix->prefix_data) { - delete node->prefix->prefix_data; + free(node->prefix->prefix_data); node->prefix->prefix_data = NULL; } - delete node->prefix; + free(node->prefix); node->prefix = NULL; } - delete node; + free(node); node = NULL; } + +IpTree::IpTree() { + // FIXME: deal with possible error. + char *error; + create_radix_tree(&m_tree, &error); +} + + IpTree::~IpTree() { if (m_tree != NULL) { if (m_tree->ipv4_tree != NULL) { // Tree_traversal: Post-order to delete all the items. postOrderTraversal(m_tree->ipv4_tree->head); - delete m_tree->ipv4_tree; + free(m_tree->ipv4_tree); m_tree->ipv4_tree = NULL; } if (m_tree->ipv6_tree != NULL) { // Tree_traversal: Post-order to delete all the items. postOrderTraversal(m_tree->ipv6_tree->head); - delete m_tree->ipv6_tree; + free(m_tree->ipv6_tree); m_tree->ipv6_tree = NULL; } - delete m_tree; + free(m_tree); m_tree = NULL; } } bool IpTree::addFromBuffer(std::istream *ss, std::string *error) { char *error_msg = NULL; - for (std::string line; std::getline(*ss, line); ) { - int res = ip_tree_from_param(line.c_str(), &m_tree, &error_msg); + int res = add_ip_from_param(line.c_str(), &m_tree, &error_msg); if (res != 0) { if (error_msg != NULL) { error->assign(error_msg); @@ -98,7 +105,6 @@ bool IpTree::addFromBuffer(std::istream *ss, std::string *error) { bool IpTree::addFromBuffer(const std::string& buffer, std::string *error) { std::stringstream ss; ss << buffer; - return addFromBuffer(&ss, error); } diff --git a/src/utils/ip_tree.h b/src/utils/ip_tree.h index d3150bc0..35dbf90b 100644 --- a/src/utils/ip_tree.h +++ b/src/utils/ip_tree.h @@ -30,8 +30,7 @@ namespace Utils { class IpTree { public: - IpTree() - : m_tree(NULL) { } + IpTree(); ~IpTree(); bool contains(const std::string &ip); diff --git a/src/utils/msc_tree.cc b/src/utils/msc_tree.cc index 72e04c2f..8a25894b 100644 --- a/src/utils/msc_tree.cc +++ b/src/utils/msc_tree.cc @@ -958,6 +958,44 @@ int tree_contains_ip(TreeRoot *rtree, return 0; } + + +int add_ip_from_param( + const char *param, TreeRoot **rtree, char **error_msg) +{ + char *param_copy = strdup(param); + char *saved = NULL; + char *str = NULL; + TreeNode *tnode = NULL; + + str = strtok_r(param_copy, ",", &saved); + while (str != NULL) + { + if (strchr(str, ':') == NULL) + { + tnode = TreeAddIP(str, (*rtree)->ipv4_tree, IPV4_TREE); + } + else + { + tnode = TreeAddIP(str, (*rtree)->ipv6_tree, IPV6_TREE); + } + + if (tnode == NULL) + { + //*error_msg = apr_psprintf("Could not add entry " \ + // "\"%s\" from: %s.", str, param); + free(param_copy); + return -1; + } + + str = strtok_r(NULL, ",", &saved); + } + free(param_copy); + + return 0; +} + + int ip_tree_from_param( const char *param, TreeRoot **rtree, char **error_msg) { diff --git a/src/utils/msc_tree.h b/src/utils/msc_tree.h index a1e1e34f..6f79ae08 100644 --- a/src/utils/msc_tree.h +++ b/src/utils/msc_tree.h @@ -97,6 +97,7 @@ unsigned char is_netmask_v6(char *ip_strv6); int tree_contains_ip(TreeRoot *rtree, const char *value, char **error_msg); +int add_ip_from_param(const char *param, TreeRoot **rtree, char **error_msg); int ip_tree_from_param(const char *param, TreeRoot **rtree, char **error_msg); int create_radix_tree(TreeRoot **rtree, char **error_msg); } diff --git a/src/utils/regex.cc b/src/utils/regex.cc index 3b72727b..0819e806 100644 --- a/src/utils/regex.cc +++ b/src/utils/regex.cc @@ -45,6 +45,19 @@ Regex::Regex(const std::string& pattern_) m_pce = pcre_study(m_pc, PCRE_STUDY_JIT_COMPILE, &errptr); } + +Regex::~Regex() { + if (m_pc != NULL) { + pcre_free(m_pc); + m_pc = NULL; + } + if (m_pce != NULL) { + pcre_free_study(m_pce); + m_pce = NULL; + } +} + + int regex_search(const std::string& s, SMatch *match, const Regex& regex) { int ovector[OVECCOUNT]; diff --git a/src/utils/regex.h b/src/utils/regex.h index c964d4e7..536fd461 100644 --- a/src/utils/regex.h +++ b/src/utils/regex.h @@ -31,6 +31,7 @@ namespace Utils { class Regex { public: explicit Regex(const std::string& pattern_); + ~Regex(); std::string pattern; pcre *m_pc = NULL; pcre_extra *m_pce = NULL; diff --git a/test/regression/custom_debug_log.cc b/test/regression/custom_debug_log.cc index a2170560..f34607b7 100644 --- a/test/regression/custom_debug_log.cc +++ b/test/regression/custom_debug_log.cc @@ -27,6 +27,7 @@ CustomDebugLog *CustomDebugLog::new_instance() { return new CustomDebugLog(); } +CustomDebugLog::~CustomDebugLog() { } void CustomDebugLog::write(int level, const std::string& message) { m_log << "[" << level << "] " << message << std::endl; @@ -35,7 +36,6 @@ void CustomDebugLog::write(int level, const std::string& message) { bool CustomDebugLog::contains(const std::string& pattern) { modsecurity::Utils::Regex re(pattern); - modsecurity::Utils::SMatch match; std::string s = m_log.str(); return modsecurity::Utils::regex_search(s, re); } diff --git a/test/regression/custom_debug_log.h b/test/regression/custom_debug_log.h index 898e2999..1544e7cc 100644 --- a/test/regression/custom_debug_log.h +++ b/test/regression/custom_debug_log.h @@ -26,6 +26,7 @@ namespace modsecurity_test { class CustomDebugLog : public modsecurity::DebugLog { public: CustomDebugLog *new_instance(); + ~CustomDebugLog(); void write(int level, const std::string& message) override; bool contains(const std::string& pattern); diff --git a/test/regression/regression.cc b/test/regression/regression.cc index a49913a8..89f29193 100644 --- a/test/regression/regression.cc +++ b/test/regression/regression.cc @@ -134,6 +134,11 @@ void perform_unit_test(ModSecurityTest *test, std::cout << KCYN << "skipped!" << RESET << std::endl; } res->push_back(testRes); + + delete modsec_transaction; + delete modsec_rules; + delete modsec; + continue; } @@ -156,6 +161,11 @@ void perform_unit_test(ModSecurityTest *test, << std::endl; testRes->passed = false; res->push_back(testRes); + + delete modsec_transaction; + delete modsec_rules; + delete modsec; + continue; } @@ -174,6 +184,11 @@ void perform_unit_test(ModSecurityTest *test, testRes->reason << KGRN << "passed!" << RESET << std::endl; 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 */ @@ -193,6 +208,11 @@ void perform_unit_test(ModSecurityTest *test, << s << std::endl; testRes->passed = false; res->push_back(testRes); + + delete modsec_transaction; + delete modsec_rules; + delete modsec; + continue; } } else { @@ -210,6 +230,11 @@ void perform_unit_test(ModSecurityTest *test, } testRes->passed = false; res->push_back(testRes); + + delete modsec_transaction; + delete modsec_rules; + delete modsec; + continue; } } @@ -340,7 +365,6 @@ after_debug_log: int main(int argc, char **argv) { ModSecurityTest test; - ModSecurityTestResults results; int test_number = 0; #ifdef WITH_GEOIP @@ -414,6 +438,7 @@ int main(int argc, char **argv) { } failed++; } + delete r; } if (!test.m_automake_output) { @@ -439,6 +464,7 @@ int main(int argc, char **argv) { } delete vec; } + #endif return 0; } diff --git a/test/unit/unit.cc b/test/unit/unit.cc index b4e006a2..00f7d594 100644 --- a/test/unit/unit.cc +++ b/test/unit/unit.cc @@ -47,7 +47,7 @@ void print_help() { void perform_unit_test(ModSecurityTest *test, UnitTest *t, ModSecurityTestResults* res) { - const char *error = NULL; + std::string error; if (test->m_automake_output) { std::cout << ":test-result: "; diff --git a/test/valgrind_suppressions.txt b/test/valgrind_suppressions.txt index bfa677ee..f62ddad4 100644 --- a/test/valgrind_suppressions.txt +++ b/test/valgrind_suppressions.txt @@ -1,3 +1,26 @@ +{ + + Memcheck:Leak + match-leak-kinds: definite + fun:_Znwm + fun:_ZN11modsecurity9operators8Operator11instantiateENSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEE + fun:_ZN2yy14seclang_parser5parseEv + fun:_ZN11modsecurity6Parser6Driver5parseERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEES9_ + fun:_ZN11modsecurity5Rules4loadEPKcRKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEE + fun:_Z17perform_unit_testPN16modsecurity_test15ModSecurityTestINS_14RegressionTestEEEPSt6vectorIPS1_SaIS5_EEPNS_22ModSecurityTestResultsINS_20RegressionTestResultEEEPi + fun:main +} +{ + + Memcheck:Leak + match-leak-kinds: definite + fun:_Znwm + fun:_ZN2yy14seclang_parser5parseEv + fun:_ZN11modsecurity6Parser6Driver5parseERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEES9_ + fun:_ZN11modsecurity5Rules4loadEPKcRKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEE + fun:_Z17perform_unit_testPN16modsecurity_test15ModSecurityTestINS_14RegressionTestEEEPSt6vectorIPS1_SaIS5_EEPNS_22ModSecurityTestResultsINS_20RegressionTestResultEEEPi + fun:main +} { Memcheck:Leak @@ -73,4 +96,16 @@ fun:_Z17perform_unit_testPSt6vectorIPN16modsecurity_test14RegressionTestESaIS2_EEPNS0_22ModSecurityTestResultsIS1_EEPi fun:main } +{ + + Memcheck:Leak + match-leak-kinds: definite + fun:_Znwm + fun:_ZN11modsecurity7actions6Action11instantiateERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEE + fun:_ZN2yy14seclang_parser5parseEv + fun:_ZN11modsecurity6Parser6Driver5parseERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEES9_ + fun:_ZN11modsecurity5Rules4loadEPKcRKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEE + fun:_Z17perform_unit_testPN16modsecurity_test15ModSecurityTestINS_14RegressionTestEEEPSt6vectorIPS1_SaIS5_EEPNS_22ModSecurityTestResultsINS_20RegressionTestResultEEEPi + fun:main +}