This is an automated email from the ASF dual-hosted git repository. swebb2066 pushed a commit to branch prevent_recursive_vars in repository https://gitbox.apache.org/repos/asf/logging-log4cxx.git
commit 6d4931bdeb652ac1ded44ca845ad82dda15abd23 Author: Stephen Webb <[email protected]> AuthorDate: Sun Nov 23 19:04:44 2025 +1100 Prevent abnormal termination when a variable is recursively referenced --- src/main/cpp/optionconverter.cpp | 180 ++++++++++++++--------- src/test/cpp/helpers/optionconvertertestcase.cpp | 19 +++ 2 files changed, 128 insertions(+), 71 deletions(-) diff --git a/src/main/cpp/optionconverter.cpp b/src/main/cpp/optionconverter.cpp index 2b6cd619..17fc221d 100644 --- a/src/main/cpp/optionconverter.cpp +++ b/src/main/cpp/optionconverter.cpp @@ -44,6 +44,114 @@ #include <log4cxx/helpers/filewatchdog.h> #include <log4cxx/helpers/singletonholder.h> +namespace +{ +using namespace LOG4CXX_NS; + +/// For recursion checking +struct LogStringChain +{ + const LogString& item; + const LogStringChain* parent; +}; + +/// 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; +} + +LogString substVarsSafely(const LogString& val, helpers::Properties& props, const LogStringChain* path = 0) +{ + LogString sbuf; + const logchar delimStartArray[] = { 0x24, 0x7B, 0 }; // '$', '{' + const LogString delimStart(delimStartArray); + const logchar delimStop = 0x7D; // '}'; + const size_t DELIM_START_LEN = 2; + const size_t DELIM_STOP_LEN = 1; + + size_t i = 0; + + while (true) + { + size_t j = val.find(delimStart, i); + + if (j == val.npos) + { + // 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; + } + } + else + { + sbuf.append(val.substr(i, j - i)); + size_t k = val.find(delimStop, j); + + if (k == val.npos) + { + LogString msg(1, (logchar) 0x22 /* '\"' */); + msg.append(val); + msg.append(LOG4CXX_STR("\" has no closing brace. Opening brace at position ")); + helpers::Pool p; + helpers::StringHelper::toString(j, p, msg); + msg.append(1, (logchar) 0x2E /* '.' */); + throw helpers::IllegalArgumentException(msg); + } + else + { + j += DELIM_START_LEN; + LogString key = val.substr(j, k - j); + if (path && isRecursiveReference(key, path)) + { + LogString msg(LOG4CXX_STR("The variable ${")); + msg.append(key); + msg.append(LOG4CXX_STR("} is used recursively")); + throw helpers::IllegalArgumentException(msg); + } + + // first try in System properties + LogString replacement(helpers::OptionConverter::getSystemProperty(key, LogString())); + + // then try props parameter + if (replacement.empty()) + { + replacement = props.getProperty(key); + } + + 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); + sbuf.append(recursiveReplacement); + } + + i = k + DELIM_STOP_LEN; + } + } + } +} + +} // namespace + namespace LOG4CXX_NS { @@ -227,77 +335,7 @@ LogString OptionConverter::findAndSubst(const LogString& key, Properties& props) LogString OptionConverter::substVars(const LogString& val, Properties& props) { - LogString sbuf; - const logchar delimStartArray[] = { 0x24, 0x7B, 0 }; // '$', '{' - const LogString delimStart(delimStartArray); - const logchar delimStop = 0x7D; // '}'; - const size_t DELIM_START_LEN = 2; - const size_t DELIM_STOP_LEN = 1; - - size_t i = 0; - - while (true) - { - size_t j = val.find(delimStart, i); - - if (j == val.npos) - { - // 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; - } - } - else - { - sbuf.append(val.substr(i, j - i)); - size_t k = val.find(delimStop, j); - - if (k == val.npos) - { - LogString msg(1, (logchar) 0x22 /* '\"' */); - msg.append(val); - msg.append(LOG4CXX_STR("\" has no closing brace. Opening brace at position ")); - Pool p; - StringHelper::toString(j, p, msg); - msg.append(1, (logchar) 0x2E /* '.' */); - throw IllegalArgumentException(msg); - } - else - { - j += DELIM_START_LEN; - LogString key = val.substr(j, k - j); - // first try in System properties - LogString replacement(getSystemProperty(key, LogString())); - - // then try props parameter - if (replacement.empty()) - { - replacement = props.getProperty(key); - } - - 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} - LogString recursiveReplacement = substVars(replacement, props); - sbuf.append(recursiveReplacement); - } - - i = k + DELIM_STOP_LEN; - } - } - } + return substVarsSafely(val, props); } 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 d3c128a8..e152fc9a 100644 --- a/src/test/cpp/helpers/optionconvertertestcase.cpp +++ b/src/test/cpp/helpers/optionconvertertestcase.cpp @@ -20,6 +20,7 @@ #include <log4cxx/helpers/system.h> #include <log4cxx/helpers/transcoder.h> #include <log4cxx/helpers/pool.h> +#include <log4cxx/helpers/loglog.h> #include "../testchar.h" #include "../insertwide.h" #include "../logunit.h" @@ -44,6 +45,7 @@ LOGUNIT_CLASS(OptionConverterTestCase) LOGUNIT_TEST(varSubstTest3); LOGUNIT_TEST(varSubstTest4); LOGUNIT_TEST(varSubstTest5); + LOGUNIT_TEST(varSubstRecursiveReferenceTest); LOGUNIT_TEST(testTmpDir); #if APR_HAS_USER LOGUNIT_TEST(testUserHome); @@ -144,6 +146,23 @@ public: LOGUNIT_ASSERT_EQUAL((LogString) LOG4CXX_STR("x1"), res); } + void varSubstRecursiveReferenceTest() + { + Properties props; + props.setProperty(LOG4CXX_STR("p1"), LOG4CXX_STR("${p2}")); + props.setProperty(LOG4CXX_STR("p2"), LOG4CXX_STR("${p1}")); + try + { + OptionConverter::substVars(LOG4CXX_STR("${p1}"), props); + LOGUNIT_ASSERT(false); + } + catch (IllegalArgumentException& ex) + { + LOG4CXX_ENCODE_CHAR(lsMsg, ex.what()); + LogLog::debug(lsMsg); + } + } + void testTmpDir() { LogString actual(OptionConverter::substVars(
