This is an automated email from the ASF dual-hosted git repository.
swebb2066 pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/logging-log4cxx.git
The following commit(s) were added to refs/heads/master by this push:
new 5d060d93 Improve MultiprocessRollingFileAppender robustness (#741)
5d060d93 is described below
commit 5d060d93409141b9556869fcd5ae9a3b2c200d33
Author: Stephen Webb <[email protected]>
AuthorDate: Thu Aug 27 10:29:24 2026 +1000
Improve MultiprocessRollingFileAppender robustness (#741)
---
src/main/cpp/multiprocessrollingfileappender.cpp | 24 +++++++++++++++++++++++-
src/main/cpp/rollingfileappender.cpp | 9 +++++++++
src/main/cpp/timebasedrollingpolicy.cpp | 10 ++++++++--
3 files changed, 40 insertions(+), 3 deletions(-)
diff --git a/src/main/cpp/multiprocessrollingfileappender.cpp
b/src/main/cpp/multiprocessrollingfileappender.cpp
index d024178d..ad9fb570 100644
--- a/src/main/cpp/multiprocessrollingfileappender.cpp
+++ b/src/main/cpp/multiprocessrollingfileappender.cpp
@@ -90,7 +90,11 @@ public: // Support classes
}
}
std::string lockFileName = filePrefix + ".lock";
- auto stat = apr_file_open(&m_parent->lock_file,
lockFileName.c_str(), APR_CREATE | APR_READ | APR_WRITE, APR_OS_DEFAULT,
m_parent->pool.getAPRPool());
+ // Create the lock file with owner-only
permissions: the cooperating
+ // processes run under the same uid, and any
other local user able to
+ // take a shared fcntl lock on a world-readable
lock file could block
+ // rollover (and with it every logging thread)
indefinitely.
+ auto stat = apr_file_open(&m_parent->lock_file,
lockFileName.c_str(), APR_CREATE | APR_READ | APR_WRITE, APR_FPROT_UREAD |
APR_FPROT_UWRITE, m_parent->pool.getAPRPool());
if (stat != APR_SUCCESS)
{
LogString err;
@@ -153,6 +157,12 @@ void MultiprocessRollingFileAppender::activateOptions(
LOG4CXX_ACTIVATE_OPTIONS_
{
if (_priv->activateOptions())
{
+ // FileAppender::activateOptionsInternal() closes any existing
writer,
+ // destroying the FileOutputStream (and the pool) that owns the
cached
+ // apr_file_t, before opening the new file (which may fail).
Clear the
+ // cache first so a failed open cannot leave it dangling;
+ // createWriter() re-caches the handle on success.
+ _priv->log_file = NULL;
FileAppender::activateOptionsInternal();
}
@@ -283,6 +293,13 @@ bool
MultiprocessRollingFileAppender::synchronizedRollover(const TriggeringPolic
else if (auto rollover1 =
_priv->rollingPolicy->rollover(fileName, getAppend()))
{
_priv->close();
+ // The cached apr_file_t was allocated from the pool of
the
+ // FileOutputStream that close() just destroyed. Clear
it now:
+ // any failure (e.g. ENOSPC/EACCES opening the new
active file
+ // throws, and subAppend merely logs the exception)
would leave a
+ // dangling handle that
isAlreadyRolled()/getCurrentFileSize()
+ // pass to apr_file_info_get() on the next append.
+ _priv->log_file = NULL;
if (rollover1->getActiveFileName() == fileName)
{
bool success = true; // A synchronous action is
not required
@@ -387,6 +404,11 @@ bool
MultiprocessRollingFileAppender::synchronizedRollover(Pool& p, const Trigge
void MultiprocessRollingFileAppender::reopenFile(const LogString& fileName)
{
_priv->close();
+ // Clear the cached apr_file_t: it was allocated from the pool of the
+ // FileOutputStream that close() just destroyed, and the
FileOutputStream
+ // constructor below throws on open failure (leaving this appender
without
+ // a writer until a later reopen succeeds).
+ _priv->log_file = NULL;
#if USING_ROLLOVER_REQUIRED_CHECK_IS_FASTER
if (auto pTimeBased =
LOG4CXX_NS::cast<TimeBasedRollingPolicy>(_priv->rollingPolicy))
pTimeBased->loadLastFileName();
diff --git a/src/main/cpp/rollingfileappender.cpp
b/src/main/cpp/rollingfileappender.cpp
index b8408646..164cc418 100644
--- a/src/main/cpp/rollingfileappender.cpp
+++ b/src/main/cpp/rollingfileappender.cpp
@@ -81,6 +81,7 @@ void RollingFileAppender::setOption(const LogString& option,
const LogString& va
int RollingFileAppender::getMaxBackupIndex() const
{
+ std::lock_guard<std::recursive_mutex> lock(_priv->mutex);
int result = 1;
if (auto fwrp =
LOG4CXX_NS::cast<FixedWindowRollingPolicy>(_priv->rollingPolicy))
result = fwrp->getMaxIndex();
@@ -89,6 +90,7 @@ int RollingFileAppender::getMaxBackupIndex() const
void RollingFileAppender::setMaxBackupIndex(int maxBackups)
{
+ std::lock_guard<std::recursive_mutex> lock(_priv->mutex);
auto fwrp =
LOG4CXX_NS::cast<FixedWindowRollingPolicy>(_priv->rollingPolicy);
if (!fwrp)
{
@@ -101,6 +103,7 @@ void RollingFileAppender::setMaxBackupIndex(int maxBackups)
size_t RollingFileAppender::getMaximumFileSize() const
{
+ std::lock_guard<std::recursive_mutex> lock(_priv->mutex);
size_t result = 10 * 1024 * 1024;
if (auto sbtp =
LOG4CXX_NS::cast<SizeBasedTriggeringPolicy>(_priv->triggeringPolicy))
result = sbtp->getMaxFileSize();
@@ -109,6 +112,7 @@ size_t RollingFileAppender::getMaximumFileSize() const
void RollingFileAppender::setMaximumFileSize(size_t maxFileSize)
{
+ std::lock_guard<std::recursive_mutex> lock(_priv->mutex);
auto sbtp =
LOG4CXX_NS::cast<SizeBasedTriggeringPolicy>(_priv->triggeringPolicy);
if (!sbtp)
{
@@ -163,6 +167,7 @@ LogString RollingFileAppender::makeFileNamePattern(const
LogString& datePattern)
void RollingFileAppender::setDatePattern(const LogString& newPattern)
{
+ std::lock_guard<std::recursive_mutex> lock(_priv->mutex);
auto tbrp =
LOG4CXX_NS::cast<TimeBasedRollingPolicy>(_priv->rollingPolicy);
if (!tbrp)
{
@@ -484,6 +489,7 @@ void RollingFileAppender::subAppend(
LOG4CXX_APPEND_FORMAL_PARAMETERS )
*/
RollingPolicyPtr RollingFileAppender::getRollingPolicy() const
{
+ std::lock_guard<std::recursive_mutex> lock(_priv->mutex);
return _priv->rollingPolicy;
}
@@ -492,6 +498,7 @@ RollingPolicyPtr RollingFileAppender::getRollingPolicy()
const
*/
TriggeringPolicyPtr RollingFileAppender::getTriggeringPolicy() const
{
+ std::lock_guard<std::recursive_mutex> lock(_priv->mutex);
return _priv->triggeringPolicy;
}
@@ -500,6 +507,7 @@ TriggeringPolicyPtr
RollingFileAppender::getTriggeringPolicy() const
*/
void RollingFileAppender::setRollingPolicy(const RollingPolicyPtr& policy)
{
+ std::lock_guard<std::recursive_mutex> lock(_priv->mutex);
_priv->rollingPolicy = policy;
}
@@ -508,6 +516,7 @@ void RollingFileAppender::setRollingPolicy(const
RollingPolicyPtr& policy)
*/
void RollingFileAppender::setTriggeringPolicy(const TriggeringPolicyPtr&
policy)
{
+ std::lock_guard<std::recursive_mutex> lock(_priv->mutex);
_priv->triggeringPolicy = policy;
}
diff --git a/src/main/cpp/timebasedrollingpolicy.cpp
b/src/main/cpp/timebasedrollingpolicy.cpp
index eaef45ca..0592df66 100644
--- a/src/main/cpp/timebasedrollingpolicy.cpp
+++ b/src/main/cpp/timebasedrollingpolicy.cpp
@@ -201,7 +201,12 @@ int TimeBasedRollingPolicy::createMMapFile(const
std::string& fileName, LOG4CXX_
{
m_priv->_mapFileName = createFile(fileName, MMAP_FILE_SUFFIX, pool);
- apr_status_t stat = apr_file_open(&m_priv->_file_map,
m_priv->_mapFileName.c_str(), APR_CREATE | APR_READ | APR_WRITE,
APR_OS_DEFAULT, m_priv->_mmapPool.getAPRPool());
+ // Create the coordination file with owner-only permissions: the
+ // cooperating processes share the same uid (embedded in the file name),
+ // and any other local user able to take a shared fcntl lock on a
+ // world-readable coordination file could block rollover (and with it
+ // every logging thread) indefinitely.
+ apr_status_t stat = apr_file_open(&m_priv->_file_map,
m_priv->_mapFileName.c_str(), APR_CREATE | APR_READ | APR_WRITE,
APR_FPROT_UREAD | APR_FPROT_UWRITE, m_priv->_mmapPool.getAPRPool());
if (stat != APR_SUCCESS)
{
@@ -407,7 +412,8 @@ RolloverDescriptionPtr TimeBasedRollingPolicy::rollover(
LOG4CXX_ROLLING_POLICY_
{
LOG4CXX_ENCODE_CHAR(mapFile,
m_priv->_fileNamePattern);
const std::string lockname =
createFile(mapFile, LOCK_FILE_SUFFIX, m_priv->_mmapPool);
- apr_status_t stat =
apr_file_open(&m_priv->_lock_file, lockname.c_str(), APR_CREATE | APR_READ |
APR_WRITE, APR_OS_DEFAULT, m_priv->_mmapPool.getAPRPool());
+ // Owner-only permissions: see the comment in
createMMapFile.
+ apr_status_t stat =
apr_file_open(&m_priv->_lock_file, lockname.c_str(), APR_CREATE | APR_READ |
APR_WRITE, APR_FPROT_UREAD | APR_FPROT_UWRITE, m_priv->_mmapPool.getAPRPool());
if (stat != APR_SUCCESS)
{
