diff --git a/src/main/cpp/optionconverter.cpp b/src/main/cpp/optionconverter.cpp index 17fc221db..e065d535b 100644 --- a/src/main/cpp/optionconverter.cpp +++ b/src/main/cpp/optionconverter.cpp @@ -43,31 +43,27 @@ #include #include #include +#include namespace { using namespace LOG4CXX_NS; -/// For recursion checking -struct LogStringChain -{ - const LogString& item; - const LogStringChain* parent; -}; +// FIX: Stack Overflow Protection & Complexity Optimization +// Old implementation used linked list (O(N^2)) and had no depth limit. +// New implementation uses std::vector (O(N) search) and hard limit. -/// Is \c item referenced in \c path -bool isRecursiveReference(const LogString& newkey, const LogStringChain* path) -{ - bool result = false; - if (path->item == newkey) - result = true; - else if (path->parent) - result = isRecursiveReference(newkey, path->parent); - return result; -} +const int MAX_SUBST_DEPTH = 20; // Hard limit to prevent Stack Overflow -LogString substVarsSafely(const LogString& val, helpers::Properties& props, const LogStringChain* path = 0) +LogString substVarsSafely(const LogString& val, helpers::Properties& props, std::vector& history) { + // 1. Recursion Guard + if (history.size() > MAX_SUBST_DEPTH) + { + helpers::LogLog::warn(LOG4CXX_STR("Variable substitution depth limit exceeded. Stopping recursion.")); + return val; // Return as-is to stop expansion + } + LogString sbuf; const logchar delimStartArray[] = { 0x24, 0x7B, 0 }; // '$', '{' const LogString delimStart(delimStartArray); @@ -86,12 +82,10 @@ LogString substVarsSafely(const LogString& val, helpers::Properties& props, cons // no more variables if (i == 0) { - // this is a simple string return val; } else { - // add the tail string which contails no variables and return the result. sbuf.append(val.substr(i, val.length() - i)); return sbuf; } @@ -115,7 +109,20 @@ LogString substVarsSafely(const LogString& val, helpers::Properties& props, cons { j += DELIM_START_LEN; LogString key = val.substr(j, k - j); - if (path && isRecursiveReference(key, path)) + + // FIX: Optimized Cycle Detection (Linear Scan on small vector) + // Much faster than pointer chasing on linked list + bool isCycle = false; + for (const auto& seenKey : history) + { + if (seenKey == key) + { + isCycle = true; + break; + } + } + + if (isCycle) { LogString msg(LOG4CXX_STR("The variable ${")); msg.append(key); @@ -134,13 +141,11 @@ LogString substVarsSafely(const LogString& val, helpers::Properties& props, cons if (!replacement.empty()) { - // Do variable substitution on the replacement string - // such that we can solve "Hello ${x2}" as "Hello p1" - // the where the properties are - // x1=p1 - // x2=${x1} - LogStringChain current{ key, path }; - LogString recursiveReplacement = substVarsSafely(replacement, props, ¤t); + // Recurse with history vector + history.push_back(key); + LogString recursiveReplacement = substVarsSafely(replacement, props, history); + history.pop_back(); // Backtrack + sbuf.append(recursiveReplacement); } @@ -335,7 +340,8 @@ LogString OptionConverter::findAndSubst(const LogString& key, Properties& props) LogString OptionConverter::substVars(const LogString& val, Properties& props) { - return substVarsSafely(val, props); + std::vector history; + return substVarsSafely(val, props, history); } LogString OptionConverter::getSystemProperty(const LogString& key, const LogString& def) diff --git a/src/test/cpp/helpers/optionconvertertestcase.cpp b/src/test/cpp/helpers/optionconvertertestcase.cpp index f809aaf9c..40d39345c 100644 --- a/src/test/cpp/helpers/optionconvertertestcase.cpp +++ b/src/test/cpp/helpers/optionconvertertestcase.cpp @@ -29,6 +29,7 @@ #include #include #include +#include using namespace log4cxx; @@ -46,6 +47,7 @@ LOGUNIT_CLASS(OptionConverterTestCase) LOGUNIT_TEST(varSubstTest4); LOGUNIT_TEST(varSubstTest5); LOGUNIT_TEST(varSubstRecursiveReferenceTest); + LOGUNIT_TEST(varSubstDepthLimitTest); LOGUNIT_TEST(testTmpDir); #if APR_HAS_USER LOGUNIT_TEST(testUserHome); @@ -163,6 +165,30 @@ LOGUNIT_CLASS(OptionConverterTestCase) } } + /** + * tests that recursion stops after MAX_SUBST_DEPTH (20) to prevent Stack Overflow. + */ + void varSubstDepthLimitTest() + { + Properties props; + Pool p; + + // create a chain of 25 variables: p0 -> p1 -> ... -> p25 + for(int i = 0; i < 25; ++i) { + LogString key = LOG4CXX_STR("p") + StringHelper::toString(i, p); + LogString val = LOG4CXX_STR("${p") + StringHelper::toString(i+1, p) + LOG4CXX_STR("}"); + props.setProperty(key, val); + } + props.setProperty(LOG4CXX_STR("p25"), LOG4CXX_STR("LeafValue")); + + // try to resolve p0. Should stop at p20 and return "${p21}" (graceful stop) + // instead of crashing or going all the way to "LeafValue". + LogString result = OptionConverter::substVars(LOG4CXX_STR("${p0}"), props); + + LOGUNIT_ASSERT(result != LOG4CXX_STR("LeafValue")); + LOGUNIT_ASSERT_EQUAL((LogString)LOG4CXX_STR("${p21}"), result); + } + void testTmpDir() { LogString actual(OptionConverter::substVars(