From 67f32a4e63766ec769e998cb52abd39baee28f29 Mon Sep 17 00:00:00 2001 From: Ervin Hegedus Date: Mon, 11 Aug 2025 15:51:09 +0200 Subject: [PATCH 1/9] Fix cppcheck 2.18.0 warnings --- src/operators/fuzzy_hash.cc | 4 ++-- src/operators/inspect_file.cc | 4 ++-- src/operators/pm_from_file.cc | 14 +++++--------- src/operators/rbl.cc | 11 +++++++++-- src/operators/validate_dtd.cc | 2 +- src/variables/xml.cc | 8 ++++---- 6 files changed, 23 insertions(+), 20 deletions(-) diff --git a/src/operators/fuzzy_hash.cc b/src/operators/fuzzy_hash.cc index 61ea2821..57b8c565 100644 --- a/src/operators/fuzzy_hash.cc +++ b/src/operators/fuzzy_hash.cc @@ -27,7 +27,7 @@ bool FuzzyHash::init(const std::string ¶m2, std::string *error) { #ifdef WITH_SSDEEP std::string digit; std::string file; - std::istream *iss; + std::ifstream *iss; std::shared_ptr chunk, t; std::string err; @@ -48,7 +48,7 @@ bool FuzzyHash::init(const std::string ¶m2, std::string *error) { std::string resource = utils::find_resource(file, param2, &err); iss = new std::ifstream(resource, std::ios::in); - if (((std::ifstream *)iss)->is_open() == false) { + if ((iss)->is_open() == false) { error->assign("Failed to open file: " + m_param + ". " + err); delete iss; return false; diff --git a/src/operators/inspect_file.cc b/src/operators/inspect_file.cc index 3796d26e..43767ff8 100644 --- a/src/operators/inspect_file.cc +++ b/src/operators/inspect_file.cc @@ -31,14 +31,14 @@ namespace modsecurity { namespace operators { bool InspectFile::init(const std::string ¶m2, std::string *error) { - std::istream *iss; + std::ifstream *iss; std::string err; std::string err_lua; m_file = utils::find_resource(m_param, param2, &err); iss = new std::ifstream(m_file, std::ios::in); - if (((std::ifstream *)iss)->is_open() == false) { + if ((iss)->is_open() == false) { error->assign("Failed to open file: " + m_param + ". " + err); delete iss; return false; diff --git a/src/operators/pm_from_file.cc b/src/operators/pm_from_file.cc index 8016c9cb..07c0e900 100644 --- a/src/operators/pm_from_file.cc +++ b/src/operators/pm_from_file.cc @@ -51,7 +51,7 @@ bool PmFromFile::init(const std::string &config, std::string *error) { for (const auto& token : tokens) { if (! token.empty()) { - std::istream *iss; + std::unique_ptr iss; if (token.compare(0, 8, "https://") == 0) { Utils::HttpsClient client; @@ -60,26 +60,22 @@ bool PmFromFile::init(const std::string &config, std::string *error) { error->assign(client.error); return false; } - iss = new std::stringstream(client.content); + iss = std::make_unique(client.content); } else { std::string err; std::string resource = utils::find_resource(token, config, &err); - iss = new std::ifstream(resource, std::ios::in); - - if (((std::ifstream *)iss)->is_open() == false) { + auto file = std::make_unique(resource, std::ios::in); + if (file->is_open() == false) { error->assign("Failed to open file: '" + token + "'. " + err); - delete iss; return false; } + iss = std::move(file); } - for (std::string line; std::getline(*iss, line); ) { if (isComment(line) == false) { acmp_add_pattern(m_p, line.c_str(), NULL, NULL, line.length()); } } - - delete iss; } } diff --git a/src/operators/rbl.cc b/src/operators/rbl.cc index 4b06f337..b09933c2 100644 --- a/src/operators/rbl.cc +++ b/src/operators/rbl.cc @@ -227,8 +227,15 @@ bool Rbl::evaluate(Transaction *t, RuleWithActions *rule, } struct sockaddr *addr = info->ai_addr; - struct sockaddr_in *sin = (struct sockaddr_in *) addr; - furtherInfo(sin, ipStr, t, m_provider); + if (addr->sa_family == AF_INET) { // only IPv4 address is allowed + struct sockaddr_in *sin = reinterpret_cast(addr); + furtherInfo(sin, ipStr, t, m_provider); + } + else { + ms_dbg_a(t, 7, "Unsupported address family: " + std::to_string(addr->sa_family)); + freeaddrinfo(info); + return false; + } freeaddrinfo(info); if (rule && t && rule->hasCaptureAction()) { diff --git a/src/operators/validate_dtd.cc b/src/operators/validate_dtd.cc index 138c7078..30423bd6 100644 --- a/src/operators/validate_dtd.cc +++ b/src/operators/validate_dtd.cc @@ -45,7 +45,7 @@ bool ValidateDTD::init(const std::string &file, std::string *error) { bool ValidateDTD::evaluate(Transaction *transaction, const std::string &str) { - XmlDtdPtrManager dtd(xmlParseDTD(NULL, (const xmlChar *)m_resource.c_str())); + XmlDtdPtrManager dtd(xmlParseDTD(NULL, reinterpret_cast(m_resource.c_str()))); if (dtd.get() == NULL) { std::string err = std::string("XML: Failed to load DTD: ") \ + m_resource; diff --git a/src/variables/xml.cc b/src/variables/xml.cc index 03dbc967..6a819be6 100644 --- a/src/variables/xml.cc +++ b/src/variables/xml.cc @@ -79,7 +79,7 @@ void XML::evaluate(Transaction *t, } /* Process the XPath expression. */ - xpathExpr = (const xmlChar*)param.c_str(); + xpathExpr = reinterpret_cast(param.c_str()); xpathCtx = xmlXPathNewContext(t->m_xml->m_data.doc); if (xpathCtx == NULL) { ms_dbg_a(t, 1, "XML: Unable to create new XPath context. : "); @@ -91,9 +91,9 @@ void XML::evaluate(Transaction *t, } else { std::vector acts = rule->getActionsByName("xmlns", t); for (auto &x : acts) { - actions::XmlNS *z = (actions::XmlNS *)x; - if (xmlXPathRegisterNs(xpathCtx, (const xmlChar*)z->m_scope.c_str(), - (const xmlChar*)z->m_href.c_str()) != 0) { + actions::XmlNS *z = reinterpret_cast(x); + if (xmlXPathRegisterNs(xpathCtx, reinterpret_cast(z->m_scope.c_str()), + reinterpret_cast(z->m_href.c_str())) != 0) { ms_dbg_a(t, 1, "Failed to register XML namespace href \"" + \ z->m_href + "\" prefix \"" + z->m_scope + "\"."); return; From d19d3b4785621cb4aaa9543b687d273fc2b00080 Mon Sep 17 00:00:00 2001 From: Ervin Hegedus Date: Mon, 11 Aug 2025 16:43:58 +0200 Subject: [PATCH 2/9] Supress uninitMemberVar warning in seclang-parser.hh --- test/cppcheck_suppressions.txt | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/cppcheck_suppressions.txt b/test/cppcheck_suppressions.txt index 2bdc15e8..0a8566bc 100644 --- a/test/cppcheck_suppressions.txt +++ b/test/cppcheck_suppressions.txt @@ -59,3 +59,6 @@ uselessCallsSubstr // Examples memleak:examples/using_bodies_in_chunks/simple_request.cc + +// supress seclang-parser.hh warnings +uninitMemberVar:src/parser/seclang-parser.hh From ea51a663f159338388fbb73f32e20930d45a7308 Mon Sep 17 00:00:00 2001 From: Ervin Hegedus Date: Mon, 11 Aug 2025 17:19:46 +0200 Subject: [PATCH 3/9] Move seclang-parser.hh supression to the right place --- test/cppcheck_suppressions.txt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/cppcheck_suppressions.txt b/test/cppcheck_suppressions.txt index 0a8566bc..8b2f2da1 100644 --- a/test/cppcheck_suppressions.txt +++ b/test/cppcheck_suppressions.txt @@ -31,6 +31,8 @@ accessMoved:seclang-parser.hh returnTempReference:seclang-parser.hh duplInheritedMember:seclang-parser.hh constVariableReference:seclang-parser.hh +uninitMemberVar:seclang-parser.hh + unreadVariable:src/operators/rx.cc unreadVariable:src/operators/rx_global.cc @@ -60,5 +62,3 @@ uselessCallsSubstr // Examples memleak:examples/using_bodies_in_chunks/simple_request.cc -// supress seclang-parser.hh warnings -uninitMemberVar:src/parser/seclang-parser.hh From 12809656a6680ba31ae689d7f200a94b8531fba1 Mon Sep 17 00:00:00 2001 From: Ervin Hegedus Date: Mon, 11 Aug 2025 22:02:37 +0200 Subject: [PATCH 4/9] Remove redundant parenthesis --- src/operators/fuzzy_hash.cc | 2 +- src/operators/inspect_file.cc | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/operators/fuzzy_hash.cc b/src/operators/fuzzy_hash.cc index 57b8c565..46ed5eb9 100644 --- a/src/operators/fuzzy_hash.cc +++ b/src/operators/fuzzy_hash.cc @@ -48,7 +48,7 @@ bool FuzzyHash::init(const std::string ¶m2, std::string *error) { std::string resource = utils::find_resource(file, param2, &err); iss = new std::ifstream(resource, std::ios::in); - if ((iss)->is_open() == false) { + if (iss->is_open() == false) { error->assign("Failed to open file: " + m_param + ". " + err); delete iss; return false; diff --git a/src/operators/inspect_file.cc b/src/operators/inspect_file.cc index 43767ff8..28c9c072 100644 --- a/src/operators/inspect_file.cc +++ b/src/operators/inspect_file.cc @@ -38,7 +38,7 @@ bool InspectFile::init(const std::string ¶m2, std::string *error) { m_file = utils::find_resource(m_param, param2, &err); iss = new std::ifstream(m_file, std::ios::in); - if ((iss)->is_open() == false) { + if (iss->is_open() == false) { error->assign("Failed to open file: " + m_param + ". " + err); delete iss; return false; From 47bc24a80863c185b48f7b042e187987eae39268 Mon Sep 17 00:00:00 2001 From: Ervin Hegedus Date: Mon, 11 Aug 2025 22:28:22 +0200 Subject: [PATCH 5/9] Decrease code nest level --- src/operators/pm_from_file.cc | 47 ++++++++++++++++++----------------- 1 file changed, 24 insertions(+), 23 deletions(-) diff --git a/src/operators/pm_from_file.cc b/src/operators/pm_from_file.cc index 07c0e900..52651e95 100644 --- a/src/operators/pm_from_file.cc +++ b/src/operators/pm_from_file.cc @@ -49,32 +49,33 @@ bool PmFromFile::init(const std::string &config, std::string *error) { std::vector tokens = split(m_param, ' '); for (const auto& token : tokens) { - if (! token.empty()) { + if (token.empty()) { + continue; + } - std::unique_ptr iss; + std::unique_ptr iss; - if (token.compare(0, 8, "https://") == 0) { - Utils::HttpsClient client; - bool ret = client.download(token); - if (ret == false) { - error->assign(client.error); - return false; - } - iss = std::make_unique(client.content); - } else { - std::string err; - std::string resource = utils::find_resource(token, config, &err); - auto file = std::make_unique(resource, std::ios::in); - if (file->is_open() == false) { - error->assign("Failed to open file: '" + token + "'. " + err); - return false; - } - iss = std::move(file); + if (token.compare(0, 8, "https://") == 0) { + Utils::HttpsClient client; + bool ret = client.download(token); + if (ret == false) { + error->assign(client.error); + return false; } - for (std::string line; std::getline(*iss, line); ) { - if (isComment(line) == false) { - acmp_add_pattern(m_p, line.c_str(), NULL, NULL, line.length()); - } + iss = std::make_unique(client.content); + } else { + std::string err; + std::string resource = utils::find_resource(token, config, &err); + auto file = std::make_unique(resource, std::ios::in); + if (file->is_open() == false) { + error->assign("Failed to open file: '" + token + "'. " + err); + return false; + } + iss = std::move(file); + } + for (std::string line; std::getline(*iss, line); ) { + if (isComment(line) == false) { + acmp_add_pattern(m_p, line.c_str(), NULL, NULL, line.length()); } } } From 9fea1ca454c77fefe5b1f8d8a20667a672d45a96 Mon Sep 17 00:00:00 2001 From: Ervin Hegedus Date: Mon, 11 Aug 2025 22:29:30 +0200 Subject: [PATCH 6/9] Change cast type (fix SonarCloud issues) --- src/operators/rbl.cc | 5 ++++- src/variables/xml.cc | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/operators/rbl.cc b/src/operators/rbl.cc index b09933c2..22fcfcff 100644 --- a/src/operators/rbl.cc +++ b/src/operators/rbl.cc @@ -226,9 +226,12 @@ bool Rbl::evaluate(Transaction *t, RuleWithActions *rule, return false; } + // NOSONAR + // SonarCloud suggested to use the init-statement to declare "addr" inside the if statement. + // I think that's not good here, because we need that in the else block struct sockaddr *addr = info->ai_addr; if (addr->sa_family == AF_INET) { // only IPv4 address is allowed - struct sockaddr_in *sin = reinterpret_cast(addr); + struct sockaddr_in *sin = (struct sockaddr_in *) addr; // cppcheck-suppress[dangerousTypeCast] furtherInfo(sin, ipStr, t, m_provider); } else { diff --git a/src/variables/xml.cc b/src/variables/xml.cc index 6a819be6..0a2d33a0 100644 --- a/src/variables/xml.cc +++ b/src/variables/xml.cc @@ -91,7 +91,7 @@ void XML::evaluate(Transaction *t, } else { std::vector acts = rule->getActionsByName("xmlns", t); for (auto &x : acts) { - actions::XmlNS *z = reinterpret_cast(x); + actions::XmlNS *z = static_cast(x); if (xmlXPathRegisterNs(xpathCtx, reinterpret_cast(z->m_scope.c_str()), reinterpret_cast(z->m_href.c_str())) != 0) { ms_dbg_a(t, 1, "Failed to register XML namespace href \"" + \ From e879711d87adf554b8e4d66575d00ba00119c21c Mon Sep 17 00:00:00 2001 From: Ervin Hegedus Date: Mon, 11 Aug 2025 23:05:32 +0200 Subject: [PATCH 7/9] Add extra SonarCloud supression msg; Change type to auto --- src/operators/rbl.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/operators/rbl.cc b/src/operators/rbl.cc index 22fcfcff..44a07bc8 100644 --- a/src/operators/rbl.cc +++ b/src/operators/rbl.cc @@ -230,8 +230,9 @@ bool Rbl::evaluate(Transaction *t, RuleWithActions *rule, // SonarCloud suggested to use the init-statement to declare "addr" inside the if statement. // I think that's not good here, because we need that in the else block struct sockaddr *addr = info->ai_addr; + // NOSONAR if (addr->sa_family == AF_INET) { // only IPv4 address is allowed - struct sockaddr_in *sin = (struct sockaddr_in *) addr; // cppcheck-suppress[dangerousTypeCast] + auto sin = (struct sockaddr_in *) addr; // cppcheck-suppress[dangerousTypeCast] furtherInfo(sin, ipStr, t, m_provider); } else { From 8b3269f4d199029a6f0bd786620f4e33faf2d821 Mon Sep 17 00:00:00 2001 From: Ervin Hegedus Date: Tue, 12 Aug 2025 21:05:31 +0200 Subject: [PATCH 8/9] Try to avoid any kind of type casting --- src/operators/rbl.cc | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/operators/rbl.cc b/src/operators/rbl.cc index 44a07bc8..894f9dc8 100644 --- a/src/operators/rbl.cc +++ b/src/operators/rbl.cc @@ -226,14 +226,14 @@ bool Rbl::evaluate(Transaction *t, RuleWithActions *rule, return false; } - // NOSONAR // SonarCloud suggested to use the init-statement to declare "addr" inside the if statement. // I think that's not good here, because we need that in the else block - struct sockaddr *addr = info->ai_addr; - // NOSONAR - if (addr->sa_family == AF_INET) { // only IPv4 address is allowed - auto sin = (struct sockaddr_in *) addr; // cppcheck-suppress[dangerousTypeCast] - furtherInfo(sin, ipStr, t, m_provider); + struct sockaddr *addr = info->ai_addr; // NOSONAR + if (addr->sa_family == AF_INET) { // NOSONAR + struct sockaddr_in sin{}; // initialize an empty struct; we don't need port info + memcpy(&sin.sin_addr, addr->sa_data + 2, sizeof(sin.sin_addr)); + sin.sin_family = AF_INET; + furtherInfo(&sin, ipStr, t, m_provider); } else { ms_dbg_a(t, 7, "Unsupported address family: " + std::to_string(addr->sa_family)); From 62cb73f31d5df203a5aaab845efeaaee90a15d35 Mon Sep 17 00:00:00 2001 From: Ervin Hegedus Date: Tue, 12 Aug 2025 21:36:52 +0200 Subject: [PATCH 9/9] Add const modifier to variable --- src/operators/rbl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/operators/rbl.cc b/src/operators/rbl.cc index 894f9dc8..9190407b 100644 --- a/src/operators/rbl.cc +++ b/src/operators/rbl.cc @@ -228,7 +228,7 @@ bool Rbl::evaluate(Transaction *t, RuleWithActions *rule, // SonarCloud suggested to use the init-statement to declare "addr" inside the if statement. // I think that's not good here, because we need that in the else block - struct sockaddr *addr = info->ai_addr; // NOSONAR + const struct sockaddr *addr = info->ai_addr; // NOSONAR if (addr->sa_family == AF_INET) { // NOSONAR struct sockaddr_in sin{}; // initialize an empty struct; we don't need port info memcpy(&sin.sin_addr, addr->sa_data + 2, sizeof(sin.sin_addr));