github_mirrors/ModSecurity
3
0
Fork 1
mirror of https://github.com/owasp-modsecurity/ModSecurity.git synced 2025-08-16 16:06:12 +03:00
Code Issues Packages Projects Releases Wiki Activity

Refactor: Ensure safe error handling by removing isolated throw; statements.

Browse Source 
- SonarCloud analysis identified standalone `throw;` calls without accompanying `try-catch` blocks, used inconsistently as placeholders or for premature termination under specific conditions.
- Removed these `throw;` instances to prevent potential runtime issues in future development phases, where such configurations might inadvertently be created.
- Introduced `assert` statements as a more appropriate mechanism for asserting preconditions in the affected class member functions, ensuring clearer intent and safer code behavior during development.
- Refactor action_kind processing to use switch() instead of if-else chains; add assertion in default case.
- Fix SonarCloud issue: Make this variable a const reference.
https://sonarcloud.io/project/issues?resolved=false&pullRequest=3104&id=owasp-modsecurity_ModSecurity&open=AY8Vpgy4f6U6E7VKL4Cn
This commit is contained in:
gberkes 2024-08-04 22:04:07 +02:00
parent b6d218f72d
commit b4659959cd
2 changed files with 48 additions and 43 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
headers/modsecurity/transaction.h
View File

@ -14,6 +14,7 @@
*/ */
#ifdef __cplusplus #ifdef __cplusplus
#include <cassert>
#include <ctime> #include <ctime>
#include <fstream> #include <fstream>
#include <iomanip> #include <iomanip>
@ -307,11 +308,8 @@ class TransactionSecMarkerManagement {
} }
std::shared_ptr<std::string> getCurrentMarker() const { std::shared_ptr<std::string> getCurrentMarker() const {
if (m_marker) { assert((m_marker != nullptr) && "You might have forgotten to call and evaluate isInsideAMarker() before calling getCurrentMarker().");
return m_marker; return m_marker;
} else {
throw; // cppcheck-suppress rethrowNoCurrentException
}
} }
void removeMarker() { void removeMarker() {

83
src/rule_with_actions.cc
View File

@ -17,6 +17,7 @@
#include <stdio.h> #include <stdio.h>
#include <cassert>
#include <algorithm> #include <algorithm>
#include <iostream> #include <iostream>
#include <string> #include <string>
@ -86,45 +87,51 @@ RuleWithActions::RuleWithActions(
if (actions) { if (actions) {
for (Action *a : *actions) { for (Action *a : *actions) {
if (a->action_kind == Action::ConfigurationKind) { switch (a->action_kind) {
a->evaluate(this, NULL); case Action::ConfigurationKind:
delete a; a->evaluate(this, NULL);
} else if (a->action_kind == Action::RunTimeOnlyIfMatchKind) {
if (dynamic_cast<actions::Capture *>(a)) {
m_containsCaptureAction = true;
delete a; delete a;
} else if (dynamic_cast<actions::MultiMatch *>(a)) { break;
m_containsMultiMatchAction = true; case Action::RunTimeOnlyIfMatchKind:
delete a; if (dynamic_cast<actions::Capture *>(a)) {
} else if (dynamic_cast<actions::Severity *>(a)) { m_containsCaptureAction = true;
m_severity = dynamic_cast<actions::Severity *>(a); delete a;
} else if (dynamic_cast<actions::LogData *>(a)) { } else if (dynamic_cast<actions::MultiMatch *>(a)) {
m_logData = dynamic_cast<actions::LogData*>(a); m_containsMultiMatchAction = true;
} else if (dynamic_cast<actions::Msg *>(a)) { delete a;
m_msg = dynamic_cast<actions::Msg*>(a); } else if (dynamic_cast<actions::Severity *>(a)) {
} else if (dynamic_cast<actions::SetVar *>(a)) { m_severity = dynamic_cast<actions::Severity *>(a);
m_actionsSetVar.push_back( } else if (dynamic_cast<actions::LogData *>(a)) {
dynamic_cast<actions::SetVar *>(a)); m_logData = dynamic_cast<actions::LogData*>(a);
} else if (dynamic_cast<actions::Tag *>(a)) { } else if (dynamic_cast<actions::Msg *>(a)) {
m_actionsTag.push_back(dynamic_cast<actions::Tag *>(a)); m_msg = dynamic_cast<actions::Msg*>(a);
} else if (dynamic_cast<actions::Block *>(a)) { } else if (dynamic_cast<actions::SetVar *>(a)) {
m_actionsRuntimePos.push_back(a); m_actionsSetVar.push_back(
m_containsStaticBlockAction = true; dynamic_cast<actions::SetVar *>(a));
} else if (a->isDisruptive() == true) { } else if (dynamic_cast<actions::Tag *>(a)) {
if (m_disruptiveAction != nullptr) { m_actionsTag.push_back(dynamic_cast<actions::Tag *>(a));
delete m_disruptiveAction; } else if (dynamic_cast<actions::Block *>(a)) {
m_disruptiveAction = nullptr; m_actionsRuntimePos.push_back(a);
m_containsStaticBlockAction = true;
} else if (a->isDisruptive() == true) {
if (m_disruptiveAction != nullptr) {
delete m_disruptiveAction;
m_disruptiveAction = nullptr;
}
m_disruptiveAction = a;
} else {
m_actionsRuntimePos.push_back(a);
} }
m_disruptiveAction = a; break;
} else { default:
m_actionsRuntimePos.push_back(a); std::cout << "General failure, action: " << a->m_name;
} std::cout << " has an unknown type." << std::endl;
} else { delete a;
delete a; #ifdef NDEBUG
std::cout << "General failure, action: " << a->m_name; break;
std::cout << " has an unknown type." << std::endl; #else
throw; // cppcheck-suppress rethrowNoCurrentException assert(false);
#endif
} }
} }
delete actions; delete actions;
@ -239,7 +246,7 @@ void RuleWithActions::executeActionsAfterFullMatch(Transaction *trans,
bool containsBlock, std::shared_ptr<RuleMessage> ruleMessage) { bool containsBlock, std::shared_ptr<RuleMessage> ruleMessage) {
bool disruptiveAlreadyExecuted = false; bool disruptiveAlreadyExecuted = false;
for (auto &a : trans->m_rules->m_defaultActions[getPhase()]) { // cppcheck-suppress ctunullpointer for (const auto &a : trans->m_rules->m_defaultActions[getPhase()]) { // cppcheck-suppress ctunullpointer
if (a.get()->action_kind != actions::Action::RunTimeOnlyIfMatchKind) { if (a.get()->action_kind != actions::Action::RunTimeOnlyIfMatchKind) {
continue; continue;
} }