This is an automated email from the ASF dual-hosted git repository. swebb2066 pushed a commit to branch possible_use_after_free in repository https://gitbox.apache.org/repos/asf/logging-log4cxx.git
commit 0b55e166dd95edc5a0e6e14b8e175b3932816497 Author: Stephen Webb <[email protected]> AuthorDate: Wed Aug 26 13:07:28 2026 +1000 Improve MultiprocessRollingFileAppender robustness --- 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) {
