This is an automated email from the ASF dual-hosted git repository. swebb2066 pushed a commit to branch handle_renamed_or_duplicate_named_appender in repository https://gitbox.apache.org/repos/asf/logging-log4cxx.git
commit a0e880754efcb300fa329af7f8aedfc829c54776 Author: Stephen Webb <[email protected]> AuthorDate: Wed Aug 19 15:33:26 2026 +1000 Prevent fault when buffered FileAppenders have the same name --- src/main/cpp/fileappender.cpp | 22 +++++++++--- .../include/log4cxx/private/fileappender_priv.h | 40 ++++++++++++++++++++++ src/test/cpp/fileappendertest.cpp | 29 ++++++++++++++++ 3 files changed, 87 insertions(+), 4 deletions(-) diff --git a/src/main/cpp/fileappender.cpp b/src/main/cpp/fileappender.cpp index 06c298dc..54215e8c 100644 --- a/src/main/cpp/fileappender.cpp +++ b/src/main/cpp/fileappender.cpp @@ -80,7 +80,9 @@ FileAppender::FileAppender(std::unique_ptr<FileAppenderPriv> priv) FileAppender::~FileAppender() { if (auto p = _priv->taskManager.lock()) - p->value().removePeriodicTask(getName()); + p->value().removePeriodicTask(_priv->flushTaskName); + if (_priv->flushTask) + _priv->flushTask->detach(); // Blocks until any in-flight flush completes } void FileAppender::setAppend(bool fileAppend1) @@ -196,15 +198,27 @@ void FileAppender::activateOptionsInternal() { _priv->activateOptions(); if (auto p = _priv->taskManager.lock()) - p->value().removePeriodicTask(getName()); + p->value().removePeriodicTask(_priv->flushTaskName); if (!_priv->bufferedIO) ; else if (0 < _priv->bufferedSeconds) { auto taskManager = ThreadUtility::instancePtr(); - taskManager->value().addPeriodicTask(getName() - , std::bind(&WriterAppenderPriv::flush, _priv) + if (!_priv->flushTask) + { + _priv->flushTask = std::make_shared<FileAppenderPriv::FlushTask>(); + _priv->flushTask->priv = _priv; + } + // Key the task by a unique name: the appender may be renamed after + // activation or share its name with another appender, and the + // destructor must deregister this task, not another appender's. + _priv->flushTaskName = getName(); + _priv->flushTaskName.append(1, (logchar) 0x23 /* '#' */); + StringHelper::toString(reinterpret_cast<size_t>(m_priv.get()), _priv->flushTaskName); + auto flushTask = _priv->flushTask; + taskManager->value().addPeriodicTask(_priv->flushTaskName + , [flushTask]() { flushTask->run(); } , std::chrono::seconds(_priv->bufferedSeconds) ); _priv->taskManager = taskManager; diff --git a/src/main/include/log4cxx/private/fileappender_priv.h b/src/main/include/log4cxx/private/fileappender_priv.h index c0fa78b2..7aff2289 100644 --- a/src/main/include/log4cxx/private/fileappender_priv.h +++ b/src/main/include/log4cxx/private/fileappender_priv.h @@ -21,6 +21,8 @@ #include <log4cxx/private/writerappender_priv.h> #include <log4cxx/fileappender.h> #include <log4cxx/helpers/threadutility.h> +#include <memory> +#include <mutex> namespace LOG4CXX_NS { @@ -68,6 +70,44 @@ struct FileAppender::FileAppenderPriv : public WriterAppender::WriterAppenderPri */ int bufferedSeconds{ 5 }; + /** + Control block shared with the periodic output buffer flush task. + The flush task only reaches this appender through the control block, + and the pointer in it is cleared (under the control block mutex) + before this structure is destroyed, so a flush that is already + executing when the appender is destroyed can never touch freed + memory and a flush scheduled afterwards is a no-op. + Only used when <code>bufferedIO == true</code>. + */ + struct FlushTask + { + std::mutex mtx; + WriterAppenderPriv* priv{ nullptr }; + + void run() + { + std::lock_guard<std::mutex> lock(mtx); + if (priv) + priv->flush(); + } + + /** Detach from the appender, blocking until any in-flight flush completes. */ + void detach() + { + std::lock_guard<std::mutex> lock(mtx); + priv = nullptr; + } + }; + std::shared_ptr<FlushTask> flushTask; + + /** + The (unique) name the periodic flush task was registered under. + Captured at registration time so the task is deregistered correctly + even if the appender is renamed after activateOptions() or shares + its name with another appender. + */ + LogString flushTaskName; + /** Manages asynchronous output buffer flush. Only used when <code>bufferedIO == true</code>. diff --git a/src/test/cpp/fileappendertest.cpp b/src/test/cpp/fileappendertest.cpp index f6704479..d2a35bcb 100644 --- a/src/test/cpp/fileappendertest.cpp +++ b/src/test/cpp/fileappendertest.cpp @@ -23,6 +23,7 @@ #include <log4cxx/helpers/transcoder.h> #include <log4cxx/helpers/exception.h> #include <log4cxx/helpers/fileoutputstream.h> +#include <log4cxx/helpers/threadutility.h> #include <log4cxx/rolling/rollingfileappender.h> #include <log4cxx/rolling/timebasedrollingpolicy.h> #include "logunit.h" @@ -64,6 +65,7 @@ LOGUNIT_CLASS(FileAppenderTest) LOGUNIT_TEST(testDirectoryCreation); LOGUNIT_TEST(testgetSetThreshold); LOGUNIT_TEST(testIsAsSevereAsThreshold); + LOGUNIT_TEST(testFlushTaskRemovedAfterRename); LOGUNIT_TEST(testPeriodicFlush); LOGUNIT_TEST(writeFinalBufferOutput); LOGUNIT_TEST(checkFinalBufferOutput); @@ -125,6 +127,33 @@ public: LOGUNIT_ASSERT(appender->isAsSevereAsThreshold(debug)); } + /** + * The periodic flush task must not outlive its appender: + * it references the appender's output buffer. Renaming the appender + * after activateOptions() previously orphaned the task (it was keyed + * by the mutable appender name), leaving it to flush through freed + * memory every bufferedSeconds after the appender was destroyed. + */ + void testFlushTaskRemovedAfterRename() + { + LogString initialName(LOG4CXX_STR("flushTaskRemovalTest")); + { + auto appender = std::make_shared<FileAppender>(); + appender->setName(initialName); + appender->setFile(LOG4CXX_STR("output/newdir/flushtask.log")); + appender->setLayout(std::make_shared<PatternLayout>(LOG4CXX_STR("%m%n"))); + appender->setBufferedIO(true); + appender->setBufferedSeconds(1); + appender->activateOptions(); + appender->setName(LOG4CXX_STR("someOtherName")); + } + // No flush task registered under any name may survive the appender + LOGUNIT_ASSERT(!ThreadUtility::instance()->hasPeriodicTask(initialName)); + LOGUNIT_ASSERT(!ThreadUtility::instance()->hasPeriodicTask(LOG4CXX_STR("someOtherName"))); + // Wait for background thread to stop + ThreadUtility::instance()->removeAllPeriodicTasks(); + } + // Check a file is periodically flushed void testPeriodicFlush() {
