Merge branch 'master' into fix-empty-appender-ref-recursion

apache · Feb 2, 2025 · 9641f75 · 9641f75
2 parents bad71da + 1d53672
commit 9641f75
Show file tree

Hide file tree

Showing 8 changed files with 60 additions and 4 deletions.
diff --git a/.github/workflows/log4cxx-ubuntu.yml b/.github/workflows/log4cxx-ubuntu.yml
@@ -36,6 +36,7 @@ jobs:
             multithread: OFF
             exitevents: OFF
             fuzzers: OFF
+            logchar: utf-8
           - name: ubuntu20-clang
             os: ubuntu-20.04
             cxx: clang++
@@ -46,6 +47,7 @@ jobs:
             multithread: OFF
             exitevents: OFF
             fuzzers: ON
+            logchar: utf-8
           - name: ubuntu22-gcc
             os: ubuntu-22.04
             cxx: g++
@@ -56,6 +58,7 @@ jobs:
             multithread: ON
             exitevents: ON
             fuzzers: OFF
+            logchar: utf-8
           - name: ubuntu22-clang
             os: ubuntu-22.04
             cxx: clang++
@@ -66,6 +69,7 @@ jobs:
             multithread: ON
             exitevents: OFF
             fuzzers: ON
+            logchar: wchar_t
 
     steps:
     - uses: actions/checkout@v4
@@ -103,6 +107,7 @@ jobs:
           -DLOG4CXX_MULTIPROCESS_ROLLING_FILE_APPENDER=${{ matrix.multiprocess }} \
           -DLOG4CXX_EVENTS_AT_EXIT=${{ matrix.exitevents }} \
           -DBUILD_FUZZERS=${{ matrix.fuzzers }} \
+          -DLOG4CXX_CHAR=${{ matrix.logchar }} \
           ..
         cmake --build .
 

diff --git a/src/fuzzers/cpp/TimeBasedRollingPolicyFuzzer.cpp b/src/fuzzers/cpp/TimeBasedRollingPolicyFuzzer.cpp
@@ -47,7 +47,7 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) {
     } else {
            tbrp->setFileNamePattern(LogString(LOG4CXX_STR("fuzz-%d{" DATE_PATTERN "}.zip")));
     }
-    rfa->setFile(LOG4CXX_STR(LOG4CXX_STR("test.log")));
+    rfa->setFile(LOG4CXX_STR("test.log"));
 
     tbrp->activateOptions(pool);
     rfa->setRollingPolicy(tbrp);

diff --git a/src/main/cpp/domconfigurator.cpp b/src/main/cpp/domconfigurator.cpp
@@ -224,6 +224,10 @@ AppenderPtr DOMConfigurator::parseAppender(Pool& p,
 	{
 		ObjectPtr instance = ObjectPtr(Loader::loadClass(className).newInstance());
 		AppenderPtr appender = LOG4CXX_NS::cast<Appender>(instance);
+		if(!appender){
+			LogLog::error(LOG4CXX_STR("Could not cast class of type [") + className + LOG4CXX_STR("] to appender"));
+			return AppenderPtr();
+		}
 		PropertySetter propSetter(appender);
 
 		appender->setName(subst(getAttribute(utf8Decoder, appenderElement, NAME_ATTR)));

diff --git a/src/main/cpp/optionconverter.cpp b/src/main/cpp/optionconverter.cpp
@@ -343,8 +343,13 @@ LevelPtr OptionConverter::toLevel(const LogString& value,
 
 	try
 	{
-		Level::LevelClass& levelClass =
-			(Level::LevelClass&)Loader::loadClass(clazz);
+		// Note: the dynamic_cast could fail across DLL boundaries.
+		// However, without the dynamic_cast a poorly formed XML file
+		// could attempt to load an invalid class as a filter, causing
+		// a crash.  If it can't be converted, a std::bad_cast should be
+		// thrown(and caught by the exception handler below)
+		const Level::LevelClass& levelClass =
+			dynamic_cast<const Level::LevelClass&>(Loader::loadClass(clazz));
 		return levelClass.toLevel(levelName);
 	}
 	catch (ClassNotFoundException&)
@@ -358,6 +363,12 @@ LevelPtr OptionConverter::toLevel(const LogString& value,
 			LOG4CXX_STR("class [") + clazz + LOG4CXX_STR("], level [") + levelName +
 			LOG4CXX_STR("] conversion) failed."), oops);
 	}
+	catch(const std::bad_cast&)
+	{
+		LogLog::warn(
+			LOG4CXX_STR("class [") + clazz + LOG4CXX_STR("] unable to be converted to "
+			"Level::LevelClass"));
+	}
 	catch (...)
 	{
 		LogLog::warn(

diff --git a/src/main/cpp/syslogappender.cpp b/src/main/cpp/syslogappender.cpp
@@ -334,8 +334,9 @@ void SyslogAppender::append(const spi::LoggingEventPtr& event, Pool& p)
 		for (auto const& item : packets)
 		{
 			// use of "%s" to avoid a security hole
+			LOG4CXX_ENCODE_CHAR(itemStr, item);
 			::syslog(_priv->syslogFacility | event->getLevel()->getSyslogEquivalent(),
-				"%s", item.c_str());
+				"%s", itemStr.c_str());
 		}
 
 		return;

diff --git a/src/test/cpp/xml/domtestcase.cpp b/src/test/cpp/xml/domtestcase.cpp
@@ -54,6 +54,8 @@ LOGUNIT_CLASS(DOMTestCase)
 	LOGUNIT_TEST(test3);
 	LOGUNIT_TEST(test4);
 	LOGUNIT_TEST(recursiveAppenderRef);
+	LOGUNIT_TEST(invalidAppender);
+	LOGUNIT_TEST(invalidLevel);
 	LOGUNIT_TEST_SUITE_END();
 
 	LoggerPtr root;
@@ -228,10 +230,25 @@ LOGUNIT_CLASS(DOMTestCase)
 		LOGUNIT_ASSERT(exists);
 	}
 
+
 	void recursiveAppenderRef()
 	{
 		// Load a bad XML file, make sure that we don't crash in endless recursion
 		DOMConfigurator::configure(LOG4CXX_TEST_STR("input/xml/DOMConfiguratorRecursive.xml"));
+  }
+
+	void invalidAppender()
+	{
+		// Load an XML file that attempts to use a levelmatchfilter as an appender.
+		// We should not crash when loading this file.
+		DOMConfigurator::configure(LOG4CXX_TEST_STR("input/xml/DOMInvalidAppender.xml"));
+  }
+
+	void invalidLevel()
+	{
+		// Load an XML file that attempts to use a filter as a level.
+		// We should not crash when loading this file.
+		DOMConfigurator::configure(LOG4CXX_TEST_STR("input/xml/DOMInvalidLevel.xml"));
 	}
 };
 

diff --git a/src/test/resources/input/xml/DOMInvalidAppender.xml b/src/test/resources/input/xml/DOMInvalidAppender.xml
@@ -0,0 +1,7 @@
+<log4j:configuration xmlns:log4j=' '>
+  <appender name="TEMP" class="levelmatchfilter"></appender>
+
+  <root>
+    <appender-ref ref="TEMP"/>
+  </root>
+</log4j:configuration>
diff --git a/src/test/resources/input/xml/DOMInvalidLevel.xml b/src/test/resources/input/xml/DOMInvalidLevel.xml
@@ -0,0 +1,11 @@
+<log4j:configuration xmlns:log4j=' '>
+  <appender name="TEMP" class="FileAppender">
+    <filter class="LevelMatchFilter">
+      <param name="LevelToMatch" value=" #Filter"/>
+    </filter>
+  </appender>
+
+  <root>
+    <appender-ref ref="TEMP"/>
+  </root>
+</log4j:configuration>