Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

V3/remove this throw call transaction h #3104

Open
wants to merge 17 commits into
base: v3/master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ jobs:
- name: build.sh
run: ./build.sh
- name: configure ${{ matrix.configure.label }}
run: ./configure ${{ matrix.configure.opt }}
run: ./configure ${{ matrix.configure.opt }} --enable-assertions=yes
- uses: ammaraskar/gcc-problem-matcher@master
- name: make
run: make -j `nproc`
Expand Down Expand Up @@ -66,7 +66,7 @@ jobs:
- name: build.sh
run: ./build.sh
- name: configure ${{ matrix.configure.label }}
run: ./configure ${{ matrix.configure.opt }}
run: ./configure ${{ matrix.configure.opt }} --enable-assertions=yes
- uses: ammaraskar/gcc-problem-matcher@master
- name: make
run: make -j `sysctl -n hw.logicalcpu`
Expand Down
13 changes: 12 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,12 @@ $ cd test
$ ./regression-tests
$ ./unit-tests
```
Please take care that the '<path/to/modsecurity>/test/cppcheck_suppressions.txt'
file contains hardcoded 'filename:line number' suppressions. If you modify a
file with explicit suppressions, you must update the 'cppcheck_suppressions.txt'
accordingly to ensure the suppression references remain aligned with the
intended lines of code. Failing to do so might trigger 'make check-static' to
break the build with errors that are logically unrelated to your modifications.

### Debugging

Expand All @@ -231,11 +237,16 @@ CFLAGS to disable the compilation optimization parameters:
```shell
$ export CFLAGS="-g -O0"
$ ./build.sh
$ ./configure
$ ./configure --enable-assertions=yes
gberkes marked this conversation as resolved.
Show resolved Hide resolved
$ make
$ sudo make install
```
"Assertions allow us to document assumptions and to spot violations early in the
development process. What is more, assertions allow us to spot violations with a
minimum of effort." https://dl.acm.org/doi/pdf/10.1145/240964.240969

It is recommended to use assertions where applicable, and to enable them with
'--enable-assertions=yes' during the testing and debugging workflow.

## Reporting Issues

Expand Down
29 changes: 29 additions & 0 deletions configure.ac
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,19 @@ MSC_GIT_VERSION=msc_version_git
AC_SUBST([MSC_GIT_VERSION])



AC_ARG_ENABLE(assertions,
[AS_HELP_STRING([--enable-assertions],[Turn on assertions feature: undefine NDEBUG])],

[case "${enableval}" in
yes) assertions=true ;;
no) assertions=false ;;
*) AC_MSG_ERROR(bad value ${enableval} for --enable-assertions) ;;
esac],

[assertions=false]
)

AC_ARG_ENABLE(debug-logs,
[AS_HELP_STRING([--disable-debug-logs],[Turn off the SecDebugLog feature])],

Expand Down Expand Up @@ -355,6 +368,14 @@ if test "$aflFuzzer" == "true"; then
GLOBAL_CPPFLAGS="$GLOBAL_CPPFLAGS $FUZZ_CPPCFLAGS"
$buildExamples = false
fi

case $assertions in
false) ASSERTIONS_CPPCFLAGS="-DNDEBUG" ;;
true) ASSERTIONS_CPPCFLAGS="-UNDEBUG" ;;
*) AC_MSG_ERROR(bad value ${assertions} for assertions) ;;
esac
GLOBAL_CPPFLAGS="$GLOBAL_CPPFLAGS $ASSERTIONS_CPPCFLAGS"

AC_SUBST(GLOBAL_LDADD)
AC_SUBST(GLOBAL_CPPFLAGS)

Expand Down Expand Up @@ -589,6 +610,14 @@ if test $buildTestUtilities = true; then
else
echo " + Test Utilities ....disabled"
fi


if test $assertions = true; then
echo " + Assertions ....enabled"
else
echo " + Assertions ....disabled"
fi

if test $debugLogs = true; then
echo " + SecDebugLog ....enabled"
else
Expand Down
8 changes: 3 additions & 5 deletions headers/modsecurity/transaction.h
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
*/

#ifdef __cplusplus
#include <cassert>
#include <ctime>
#include <fstream>
#include <iomanip>
Expand Down Expand Up @@ -307,11 +308,8 @@ class TransactionSecMarkerManagement {
}

std::shared_ptr<std::string> getCurrentMarker() const {
if (m_marker) {
return m_marker;
} else {
throw;
airween marked this conversation as resolved.
Show resolved Hide resolved
}
assert((m_marker != nullptr) && "You might have forgotten to call and evaluate isInsideAMarker() before calling getCurrentMarker().");
return m_marker;
}

void removeMarker() {
Expand Down
83 changes: 45 additions & 38 deletions src/rule_with_actions.cc
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

#include <stdio.h>

#include <cassert>
#include <algorithm>
#include <iostream>
#include <string>
Expand Down Expand Up @@ -86,45 +87,51 @@ RuleWithActions::RuleWithActions(

if (actions) {
for (Action *a : *actions) {
if (a->action_kind == Action::ConfigurationKind) {
a->evaluate(this, NULL);
delete a;

} else if (a->action_kind == Action::RunTimeOnlyIfMatchKind) {
if (dynamic_cast<actions::Capture *>(a)) {
m_containsCaptureAction = true;
delete a;
} else if (dynamic_cast<actions::MultiMatch *>(a)) {
m_containsMultiMatchAction = true;
switch (a->action_kind) {
case Action::ConfigurationKind:
a->evaluate(this, NULL);
delete a;
} else if (dynamic_cast<actions::Severity *>(a)) {
m_severity = dynamic_cast<actions::Severity *>(a);
} else if (dynamic_cast<actions::LogData *>(a)) {
m_logData = dynamic_cast<actions::LogData*>(a);
} else if (dynamic_cast<actions::Msg *>(a)) {
m_msg = dynamic_cast<actions::Msg*>(a);
} else if (dynamic_cast<actions::SetVar *>(a)) {
m_actionsSetVar.push_back(
dynamic_cast<actions::SetVar *>(a));
} else if (dynamic_cast<actions::Tag *>(a)) {
m_actionsTag.push_back(dynamic_cast<actions::Tag *>(a));
} else if (dynamic_cast<actions::Block *>(a)) {
m_actionsRuntimePos.push_back(a);
m_containsStaticBlockAction = true;
} else if (a->isDisruptive() == true) {
if (m_disruptiveAction != nullptr) {
delete m_disruptiveAction;
m_disruptiveAction = nullptr;
break;
case Action::RunTimeOnlyIfMatchKind:
if (dynamic_cast<actions::Capture *>(a)) {
m_containsCaptureAction = true;
delete a;
} else if (dynamic_cast<actions::MultiMatch *>(a)) {
m_containsMultiMatchAction = true;
delete a;
} else if (dynamic_cast<actions::Severity *>(a)) {
m_severity = dynamic_cast<actions::Severity *>(a);
} else if (dynamic_cast<actions::LogData *>(a)) {
m_logData = dynamic_cast<actions::LogData*>(a);
} else if (dynamic_cast<actions::Msg *>(a)) {
m_msg = dynamic_cast<actions::Msg*>(a);
} else if (dynamic_cast<actions::SetVar *>(a)) {
m_actionsSetVar.push_back(
dynamic_cast<actions::SetVar *>(a));
} else if (dynamic_cast<actions::Tag *>(a)) {
m_actionsTag.push_back(dynamic_cast<actions::Tag *>(a));
} else if (dynamic_cast<actions::Block *>(a)) {
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;
} else {
m_actionsRuntimePos.push_back(a);
}
} else {
delete a;
std::cout << "General failure, action: " << a->m_name;
std::cout << " has an unknown type." << std::endl;
throw;
airween marked this conversation as resolved.
Show resolved Hide resolved
break;
default:
std::cout << "General failure, action: " << a->m_name;
std::cout << " has an unknown type." << std::endl;
delete a;
#ifdef NDEBUG
break;
#else
assert(false);
#endif
}
}
delete actions;
Expand Down Expand Up @@ -241,7 +248,7 @@ void RuleWithActions::executeActionsAfterFullMatch(Transaction *trans,
bool containsBlock, std::shared_ptr<RuleMessage> ruleMessage) {
bool disruptiveAlreadyExecuted = false;

for (auto &a : trans->m_rules->m_defaultActions[getPhase()]) {
for (const auto &a : trans->m_rules->m_defaultActions[getPhase()]) { // cppcheck_suppressions.txt:55
if (a.get()->action_kind != actions::Action::RunTimeOnlyIfMatchKind) {
continue;
}
Expand Down
4 changes: 1 addition & 3 deletions test/cppcheck_suppressions.txt
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,7 @@ noConstructor:src/variables/variable.h:152
danglingTempReference:src/modsecurity.cc:206
knownConditionTrueFalse:src/operators/validate_url_encoding.cc:77
knownConditionTrueFalse:src/operators/verify_svnr.cc:87
rethrowNoCurrentException:headers/modsecurity/transaction.h:313
rethrowNoCurrentException:src/rule_with_actions.cc:127
ctunullpointer:src/rule_with_actions.cc:244
ctunullpointer:src/rule_with_actions.cc:251
ctunullpointer:src/rule_with_operator.cc:135
ctunullpointer:src/rule_with_operator.cc:95
passedByValue:test/common/modsecurity_test.cc:49
Expand Down