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 6e4ab3ea Prevent fault when buffered FileAppenders have the same name
(#733)
6e4ab3ea is described below
commit 6e4ab3ea1997dc61b875807ee9b7e9ffc54595a5
Author: Stephen Webb <[email protected]>
AuthorDate: Thu Aug 20 18:00:12 2026 +1000
Prevent fault when buffered FileAppenders have the same name (#733)
---
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()
{
