Skip to content

Support reopening audit and debug logs #3521

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

Open
fremail wants to merge 3 commits into
base: v3/master
Choose a base branch
from
Open

Support reopening audit and debug logs #3521

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
544a2f5
Support reopening audit and debug logs
fremail Mar 23, 2026
69cec3a
Fix linter issues
fremail Mar 23, 2026
329efd4
Fix linter issues: use fopen_s on windows and fopen on other OS
fremail Mar 23, 2026
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 @@ -168,6 +168,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/debug_log.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ class DebugLog {
virtual bool isLogLevelSet();
virtual void setDebugLogLevel(int level);
virtual void setDebugLogFile(const std::string &fileName, std::string *error);
virtual bool reopenDebugLogFile(std::string *error);
virtual const std::string& getDebugLogFile();
Copy link

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

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

Adding a new virtual method to DebugLog changes the class vtable layout, which is an ABI break for downstream C++ consumers that link against libmodsecurity but were compiled against an older header. Since headers/modsecurity/*.h are installed public headers (src/Makefile.am pkginclude_HEADERS), consider making this method non-virtual (if polymorphic override isn’t required) or otherwise ensure an intentional ABI/SONAME bump and document it.

Suggested change
virtual const std::string& getDebugLogFile();
const std::string& getDebugLogFile();

Copilot uses AI. Check for mistakes.
virtual int getDebugLogLevel();

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 @@ -105,6 +105,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_logs(RulesSet *rules, const char **error);

#ifdef __cplusplus
}
Expand Down
8 changes: 8 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 @@ -325,6 +325,14 @@ bool AuditLog::saveIfRelevant(Transaction *transaction, int parts) {
}


bool AuditLog::reopen(std::string *error) {
if (m_writer != nullptr) {
return m_writer->reopen(error);
}
return true;
}


bool AuditLog::close() {
return true;
}
Expand Down
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
30 changes: 30 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,36 @@ 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);
}

if (!success1 || !success2) {
std::string msg = "Failed to reopen parallel audit logs.";
if (!success1 && !error1.empty()) {
msg += " " + error1;
}
if (!success2 && !error2.empty()) {
msg += " " + error2;
}
error->assign(msg);
}

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
11 changes: 11 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,17 @@ bool Serial::write(Transaction *transaction, int parts, std::string *error) {
error);
}


bool Serial::reopen(std::string *error) {
bool success = utils::SharedFiles::getInstance().reopen(
m_audit->m_path1, error);
if (!success) {
std::string detail = *error;
error->assign("Failed to reopen serial audit log. " + detail);
}
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
8 changes: 8 additions & 0 deletions src/debug_log/debug_log.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,14 @@ void DebugLog::setDebugLogFile(const std::string& fileName,
}


bool DebugLog::reopenDebugLogFile(std::string *error) {
if (!isLogFileSet()) {
return true;
}
return DebugLogWriter::reopen(m_fileName, error);
}


void DebugLog::setDebugLogLevel(int level) {
m_debugLevel = level;
}
Expand Down
5 changes: 5 additions & 0 deletions src/debug_log/debug_log_writer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,11 @@ void DebugLogWriter::close(const std::string& fileName) {
}


bool DebugLogWriter::reopen(const std::string& fileName, std::string *error) {
return utils::SharedFiles::getInstance().reopen(fileName, error);
}


void DebugLogWriter::write_log(const std::string& fileName,
const std::string &msg) {
std::string err;
Expand Down
1 change: 1 addition & 0 deletions src/debug_log/debug_log_writer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ class DebugLogWriter {
static void write_log(const std::string& file, const std::string& msg);
static void close(const std::string& m_fileName);
static int open(const std::string& m_fileName, std::string *error);
static bool reopen(const std::string& fileName, std::string *error);

private:
DebugLogWriter() = default;
Expand Down
36 changes: 36 additions & 0 deletions src/rules_set.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,3 +347,39 @@ extern "C" int msc_rules_cleanup(RulesSet *rules) {

} // namespace modsecurity


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

if (rules->m_auditLog != nullptr) {
std::string auditError;
if (!rules->m_auditLog->reopen(&auditError)) {
succeeded = false;
errorStr += auditError;
}
}

if (rules->m_debugLog != nullptr) {
std::string debugError;
if (!rules->m_debugLog->reopenDebugLogFile(&debugError)) {
succeeded = false;
if (!errorStr.empty()) {
errorStr += " ";
}
errorStr += debugError;
}
}

if (!succeeded) {
if (!errorStr.empty()) {
*error = strdup(errorStr.c_str());
} else {
*error = strdup("Unknown error reopening logs");
}
}

return succeeded ? 0 : -1;
}
Comment on lines +383 to +384
Copy link

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

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

The C API generally returns 1 for success and 0 for failure for boolean operations (e.g., msc_update_status_code, msc_set_request_hostname), while rules APIs return non-negative on success and negative on failure. Returning 0 on success here is inconsistent and makes it easy for callers to mis-handle success as failure. Consider returning 1 on success and 0 (or a negative value consistently used in the rules API) on failure, and document the convention in the header/comments.

Copilot uses AI. Check for mistakes.

43 changes: 43 additions & 0 deletions src/utils/shared_files.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,16 @@
#include "src/utils/shared_files.h"

#include <fcntl.h>
#include <sys/types.h>
#include <sys/stat.h>
#ifdef WIN32
#include <algorithm>
#endif

#if defined(_MSC_VER) && !defined(_CRT_SECURE_NO_DEPRECATE)
#define _CRT_SECURE_NO_DEPRECATE 1

Check failure on line 26 in src/utils/shared_files.cc

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Replace this macro by "const", "constexpr" or an "enum". 


See more on https://sonarcloud.io/project/issues?id=owasp-modsecurity_ModSecurity&issues=AZ0b0HFcdghgBKdKjJ2J&open=AZ0b0HFcdghgBKdKjJ2J&pullRequest=3521
#endif
Comment on lines +25 to +27
Copy link

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

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

_CRT_SECURE_NO_DEPRECATE is defined after including src/utils/shared_files.h (which includes <stdio.h>), so it won’t suppress MSVC CRT deprecation warnings for those headers. If this macro is needed, it should be set via compiler definitions or before any CRT headers are included (typically in a central config header or build flags), not locally here.

Copilot uses AI. Check for mistakes.


namespace modsecurity {
namespace utils {
Expand Down Expand Up @@ -81,6 +87,43 @@
}


bool SharedFiles::reopen(const std::string& fileName, std::string *error) {
auto it = m_handlers.find(fileName);
if (it == m_handlers.end()) {
error->assign("Cannot find open file to reopen: " + fileName);
return false;
}

struct stat target_stat;
struct stat current_stat;

if (
stat(fileName.c_str(), &target_stat) == 0 &&
fstat(fileno(it->second.fp), &current_stat) == 0 &&
current_stat.st_dev == target_stat.st_dev &&
current_stat.st_ino == target_stat.st_ino
) {
return true;
}

FILE *fp;
#ifdef WIN32
fopen_s(&fp, fileName.c_str(), "a");
#else
fp = fopen(fileName.c_str(), "a");

Check warning on line 113 in src/utils/shared_files.cc

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

'fopen' is deprecated: This function or variable may be unsafe. Consider using fopen_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details. 


See more on https://sonarcloud.io/project/issues?id=owasp-modsecurity_ModSecurity&issues=AZ0b58L2uulwwnBCpnNl&open=AZ0b58L2uulwwnBCpnNl&pullRequest=3521
#endif
if (fp == nullptr) {
error->assign("Failed to reopen file: " + fileName);
return false;
}

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

Comment on lines +119 to +122
Copy link

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

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

SharedFiles::reopen() swaps it->second.fp without taking the same exclusive lock used by write(). If a thread is inside write() while another thread calls reopen(), fclose(it->second.fp) can race with fwrite()/fflush() on that FILE*, leading to UB/crashes. Consider acquiring the same per-file lock (fcntl F_WRLCK on POSIX / named mutex on WIN32) around the inode check and the close+swap, so reopen() is mutually exclusive with write() for that handler.

Suggested change
fclose(it->second.fp);
it->second.fp = fp;
// Acquire the same exclusive lock used by write() to avoid races on it->second.fp
#ifndef WIN32
struct flock lock {};
lock.l_start = lock.l_len = lock.l_whence = 0;
lock.l_type = F_WRLCK;
if (fcntl(fileno(it->second.fp), F_SETLKW, &lock) == -1) {
// Failed to acquire lock; close the newly opened file and report error
fclose(fp);
error->assign("Failed to lock file during reopen: " + fileName);
return false;
}
#else
DWORD dwWaitResult = WaitForSingleObject(it->second.hMutex, INFINITE);
if (dwWaitResult != WAIT_OBJECT_0) {
// Failed to acquire mutex; close the newly opened file and report error
fclose(fp);
error->assign("Couldn't lock shared file during reopen: " + fileName);
return false;
}
#endif
fclose(it->second.fp);
it->second.fp = fp;
// Release the exclusive lock
#ifndef WIN32
lock.l_type = F_UNLCK;
fcntl(fileno(it->second.fp), F_SETLKW, &lock);
#else
::ReleaseMutex(it->second.hMutex);
#endif

Copilot uses AI. Check for mistakes.
return true;
}


void SharedFiles::close(const std::string& fileName) {
if (fileName.empty())
return;
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
Loading