Skip to content

Updated Code from PR #2304 #3371

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.

Sign up for GitHub

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

Draft
wants to merge 2 commits into
base: v3/master
Choose a base branch
from
Draft

Updated Code from PR #2304 #3371

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7da14cc
CC: https://github.com/owasp-modsecurity/ModSecurity/pull/2304
akama-aka May 6, 2025
b0a5694
This code fixes the errors from https://github.com/owasp-modsecurity/…
akama-aka May 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions headers/modsecurity/audit_log.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,7 @@ class AuditLog {
bool setType(AuditLogType audit_type);

bool init(std::string *error);
bool reopen(std::string *error);
virtual bool close();

bool saveIfRelevant(Transaction *transaction);
Expand Down
1 change: 1 addition & 0 deletions headers/modsecurity/rules_set.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ int msc_rules_add_file(RulesSet *rules, const char *file, const char **error);
int msc_rules_add(RulesSet *rules, const char *plain_rules, const char **error);
void msc_rules_error_cleanup(const char *error);
int msc_rules_cleanup(RulesSet *rules);
int msc_rules_reopen_audit_log(RulesSet *rules, const char **error);

#ifdef __cplusplus
}
Expand Down
6 changes: 6 additions & 0 deletions src/audit_log/audit_log.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -369,6 +369,12 @@ bool AuditLog::merge(AuditLog *from, std::string *error) {
return init(error);
}

bool AuditLog::reopen(std::string *error) {
if (m_writer != NULL) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As Sonarcloud shows here you should use nullptr instead of NULL. I see there are many other NULL literals in this file - feel free to replace them.

return m_writer->reopen(error);
}
return true;
}

} // namespace audit_log
} // namespace modsecurity
3 changes: 3 additions & 0 deletions src/audit_log/writer/https.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,9 @@ class Https : public Writer {
bool init(std::string *error) override;
bool write(Transaction *transaction, int parts,
std::string *error) override;
bool reopen(std::string *error) override {
return true;
}
};

} // namespace writer
Expand Down
28 changes: 28 additions & 0 deletions src/audit_log/writer/parallel.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,34 @@ bool Parallel::write(Transaction *transaction, int parts, std::string *error) {
return true;
}

bool Parallel::reopen(std::string *error) {
bool success1 = true;
bool success2 = true;
std::string error1;
std::string error2;

if(!m_audit->m_path1.empty()) {
success1 = utils::SharedFiles::getInstance().reopen(m_audit->m_path1, &error1);
}
if (!m_audit->m_path2.empty()) {
success2 = utils::SharedFiles::getInstance().reopen(m_audit->m_path2, &error2);
}
std::stringstream errorStream;
if (!success1 || !success2) {
errorStream << "There was an error reopening parallel audit logs.";

if (!success1 && !error1.empty()) {
errorStream << " " << error1;
}
if (!success2 && !error2.empty()) {
errorStream << " " << error2;
}

*error = errorStream.str();
}
return success1 && success2;
}

} // namespace writer
} // namespace audit_log
} // namespace modsecurity
1 change: 1 addition & 0 deletions src/audit_log/writer/parallel.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ class Parallel : public Writer {
bool init(std::string *error) override;
bool write(Transaction *transaction, int parts,
std::string *error) override;
bool reopen(std::string *error) override;


/**
Expand Down
13 changes: 13 additions & 0 deletions src/audit_log/writer/serial.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,19 @@ bool Serial::write(Transaction *transaction, int parts, std::string *error) {
error);
}

bool Serial::reopen(std::string *error) {
bool success;

std::string errorDetail;
success = utils::SharedFiles::getInstance().reopen(m_audit->m_path1, &errorDetail);
if (!success) {
*error = "There was an error reopening the serial audit log. ";
error->append(errorDetail);
}

return success;
}

} // namespace writer
} // namespace audit_log
} // namespace modsecurity
1 change: 1 addition & 0 deletions src/audit_log/writer/serial.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ class Serial : public Writer {
bool init(std::string *error) override;
bool write(Transaction *transaction, int parts,
std::string *error) override;
bool reopen(std::string *error) override;
};

} // namespace writer
Expand Down
1 change: 1 addition & 0 deletions src/audit_log/writer/writer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ class Writer {
virtual bool init(std::string *error) = 0;
virtual bool write(Transaction *transaction, int parts,
std::string *error) = 0;
virtual bool reopen(std::string *error) = 0;

static void generateBoundary(std::string *boundary);

Expand Down
19 changes: 19 additions & 0 deletions src/rules_set.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,6 +325,25 @@ extern "C" void msc_rules_error_cleanup(const char *error) {
free((void*) error);
}

extern "C" int msc_rules_reopen_audit_log(RulesSet *rules, const char **error) {
bool succeeded = true;
std::string errorStr;

if (rules->m_auditLog != NULL) {
succeeded = rules->m_auditLog->reopen(&errorStr);
}

if (!succeeded) {
if (!errorStr.empty()) {
*error = strdup(errorStr.c_str());
} else {
// Guarantee an error message is always assigned in the event of a failure
*error = strdup("Unknown error reopening audit log");
}
}

return succeeded ? 0 : -1;
}

Comment on lines +328 to 347
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is a completely valid solution, but as you can see in other "C" API functions, the authors tied to avoid all the logic and use C++ types (like here the bool and std::string).

My suggestion here: create a thin wrapper, an "extra" function.

You should add a new function to RulesSet class with name reopen(), move this body there, and in this "C" API you have to call only that function.

extern "C" int msc_rules_cleanup(RulesSet *rules) {
delete rules;
Expand Down
19 changes: 18 additions & 1 deletion src/utils/shared_files.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
*/

#include "src/utils/shared_files.h"

#include <cstring>
#include <fcntl.h>
#ifdef WIN32
#include <algorithm>
Expand Down Expand Up @@ -101,6 +101,23 @@ void SharedFiles::close(const std::string& fileName) {
}
}

bool SharedFiles::reopen(const std::string& fileName, std::string *error) {
auto it = m_handlers.find(fileName);
if (it == m_handlers.end()) {
return open(fileName, error);
}

FILE* new_fp = fopen(fileName.c_str(), "a+");
if (new_fp == nullptr) {
error->assign("Failed to reopen file: " + fileName + " - " + strerror(errno));
return false;
}

fclose(it->second.fp);
it->second.fp = new_fp;

return true;
}

bool SharedFiles::write(const std::string& fileName,
const std::string &msg, std::string *error) {
Expand Down
1 change: 1 addition & 0 deletions src/utils/shared_files.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ namespace utils {
class SharedFiles {
public:
bool open(const std::string& fileName, std::string *error);
bool reopen(const std::string& filename, std::string *error);
void close(const std::string& fileName);
bool write(const std::string& fileName, const std::string &msg,
std::string *error);
Expand Down
7 changes: 6 additions & 1 deletion test/cppcheck_suppressions.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,12 @@ shiftNegative:src/utils/msc_tree.cc

//
// ModSecurity v3 code...
//
//

functionConst:src/utils/shared_files.h:60
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally: please don't use this file to collect suppressions. If you need to add some suppression, please use an inline one.

But here: why it this necessary? It seems in that file in line 60 there is only a condition...

useInitializationList:src/utils/shared_files.h:88
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar here, but it seems that file only has 77 lines, so adding line 88 makes no sense.



variableScope:src/operators/rx.cc
variableScope:src/operators/rx_global.cc

Expand Down
Loading