Skip to content

Commit 0a21e4d

Browse files
swebb2066swebb2066
authored
Prevent abnormal termination when a variable is recursively referenced (#565)
1 parent 12fc6ed commit 0a21e4d

File tree

2 files changed

+128
-71
lines changed

2 files changed

+128
-71
lines changed

‎src/main/cpp/optionconverter.cpp‎

Lines changed: 109 additions & 71 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,114 @@
4444
#include <log4cxx/helpers/filewatchdog.h>
4545
#include <log4cxx/helpers/singletonholder.h>
4646

47+
namespace
48+
{
49+
using namespace LOG4CXX_NS;
50+
51+
/// For recursion checking
52+
struct LogStringChain
53+
{
54+
const LogString& item;
55+
const LogStringChain* parent;
56+
};
57+
58+
/// Is \c item referenced in \c path
59+
bool isRecursiveReference(const LogString& newkey, const LogStringChain* path)
60+
{
61+
bool result = false;
62+
if (path->item == newkey)
63+
result = true;
64+
else if (path->parent)
65+
result = isRecursiveReference(newkey, path->parent);
66+
return result;
67+
}
68+
69+
LogString substVarsSafely(const LogString& val, helpers::Properties& props, const LogStringChain* path = 0)
70+
{
71+
LogString sbuf;
72+
const logchar delimStartArray[] = { 0x24, 0x7B, 0 }; // '$', '{'
73+
const LogString delimStart(delimStartArray);
74+
const logchar delimStop = 0x7D; // '}';
75+
const size_t DELIM_START_LEN = 2;
76+
const size_t DELIM_STOP_LEN = 1;
77+
78+
size_t i = 0;
79+
80+
while (true)
81+
{
82+
size_t j = val.find(delimStart, i);
83+
84+
if (j == val.npos)
85+
{
86+
// no more variables
87+
if (i == 0)
88+
{
89+
// this is a simple string
90+
return val;
91+
}
92+
else
93+
{
94+
// add the tail string which contails no variables and return the result.
95+
sbuf.append(val.substr(i, val.length() - i));
96+
return sbuf;
97+
}
98+
}
99+
else
100+
{
101+
sbuf.append(val.substr(i, j - i));
102+
size_t k = val.find(delimStop, j);
103+
104+
if (k == val.npos)
105+
{
106+
LogString msg(1, (logchar) 0x22 /* '\"' */);
107+
msg.append(val);
108+
msg.append(LOG4CXX_STR("\" has no closing brace. Opening brace at position "));
109+
helpers::Pool p;
110+
helpers::StringHelper::toString(j, p, msg);
111+
msg.append(1, (logchar) 0x2E /* '.' */);
112+
throw helpers::IllegalArgumentException(msg);
113+
}
114+
else
115+
{
116+
j += DELIM_START_LEN;
117+
LogString key = val.substr(j, k - j);
118+
if (path && isRecursiveReference(key, path))
119+
{
120+
LogString msg(LOG4CXX_STR("The variable ${"));
121+
msg.append(key);
122+
msg.append(LOG4CXX_STR("} is used recursively"));
123+
throw helpers::IllegalArgumentException(msg);
124+
}
125+
126+
// first try in System properties
127+
LogString replacement(helpers::OptionConverter::getSystemProperty(key, LogString()));
128+
129+
// then try props parameter
130+
if (replacement.empty())
131+
{
132+
replacement = props.getProperty(key);
133+
}
134+
135+
if (!replacement.empty())
136+
{
137+
// Do variable substitution on the replacement string
138+
// such that we can solve "Hello ${x2}" as "Hello p1"
139+
// the where the properties are
140+
// x1=p1
141+
// x2=${x1}
142+
LogStringChain current{ key, path };
143+
LogString recursiveReplacement = substVarsSafely(replacement, props, &current);
144+
sbuf.append(recursiveReplacement);
145+
}
146+
147+
i = k + DELIM_STOP_LEN;
148+
}
149+
}
150+
}
151+
}
152+
153+
} // namespace
154+
47155
namespace LOG4CXX_NS
48156
{
49157

@@ -227,77 +335,7 @@ LogString OptionConverter::findAndSubst(const LogString& key, Properties& props)
227335

228336
LogString OptionConverter::substVars(const LogString& val, Properties& props)
229337
{
230-
LogString sbuf;
231-
const logchar delimStartArray[] = { 0x24, 0x7B, 0 }; // '$', '{'
232-
const LogString delimStart(delimStartArray);
233-
const logchar delimStop = 0x7D; // '}';
234-
const size_t DELIM_START_LEN = 2;
235-
const size_t DELIM_STOP_LEN = 1;
236-
237-
size_t i = 0;
238-
239-
while (true)
240-
{
241-
size_t j = val.find(delimStart, i);
242-
243-
if (j == val.npos)
244-
{
245-
// no more variables
246-
if (i == 0)
247-
{
248-
// this is a simple string
249-
return val;
250-
}
251-
else
252-
{
253-
// add the tail string which contails no variables and return the result.
254-
sbuf.append(val.substr(i, val.length() - i));
255-
return sbuf;
256-
}
257-
}
258-
else
259-
{
260-
sbuf.append(val.substr(i, j - i));
261-
size_t k = val.find(delimStop, j);
262-
263-
if (k == val.npos)
264-
{
265-
LogString msg(1, (logchar) 0x22 /* '\"' */);
266-
msg.append(val);
267-
msg.append(LOG4CXX_STR("\" has no closing brace. Opening brace at position "));
268-
Pool p;
269-
StringHelper::toString(j, p, msg);
270-
msg.append(1, (logchar) 0x2E /* '.' */);
271-
throw IllegalArgumentException(msg);
272-
}
273-
else
274-
{
275-
j += DELIM_START_LEN;
276-
LogString key = val.substr(j, k - j);
277-
// first try in System properties
278-
LogString replacement(getSystemProperty(key, LogString()));
279-
280-
// then try props parameter
281-
if (replacement.empty())
282-
{
283-
replacement = props.getProperty(key);
284-
}
285-
286-
if (!replacement.empty())
287-
{
288-
// Do variable substitution on the replacement string
289-
// such that we can solve "Hello ${x2}" as "Hello p1"
290-
// the where the properties are
291-
// x1=p1
292-
// x2=${x1}
293-
LogString recursiveReplacement = substVars(replacement, props);
294-
sbuf.append(recursiveReplacement);
295-
}
296-
297-
i = k + DELIM_STOP_LEN;
298-
}
299-
}
300-
}
338+
return substVarsSafely(val, props);
301339
}
302340

303341
LogString OptionConverter::getSystemProperty(const LogString& key, const LogString& def)

‎src/test/cpp/helpers/optionconvertertestcase.cpp‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
#include <log4cxx/helpers/system.h>
2121
#include <log4cxx/helpers/transcoder.h>
2222
#include <log4cxx/helpers/pool.h>
23+
#include <log4cxx/helpers/loglog.h>
2324
#include "../testchar.h"
2425
#include "../insertwide.h"
2526
#include "../logunit.h"
@@ -44,6 +45,7 @@ LOGUNIT_CLASS(OptionConverterTestCase)
4445
LOGUNIT_TEST(varSubstTest3);
4546
LOGUNIT_TEST(varSubstTest4);
4647
LOGUNIT_TEST(varSubstTest5);
48+
LOGUNIT_TEST(varSubstRecursiveReferenceTest);
4749
LOGUNIT_TEST(testTmpDir);
4850
#if APR_HAS_USER
4951
LOGUNIT_TEST(testUserHome);
@@ -144,6 +146,23 @@ LOGUNIT_CLASS(OptionConverterTestCase)
144146
LOGUNIT_ASSERT_EQUAL((LogString) LOG4CXX_STR("x1"), res);
145147
}
146148

149+
void varSubstRecursiveReferenceTest()
150+
{
151+
Properties props;
152+
props.setProperty(LOG4CXX_STR("p1"), LOG4CXX_STR("${p2}"));
153+
props.setProperty(LOG4CXX_STR("p2"), LOG4CXX_STR("${p1}"));
154+
try
155+
{
156+
OptionConverter::substVars(LOG4CXX_STR("${p1}"), props);
157+
LOGUNIT_ASSERT(false);
158+
}
159+
catch (IllegalArgumentException& ex)
160+
{
161+
LOG4CXX_DECODE_CHAR(lsMsg, ex.what());
162+
LogLog::debug(lsMsg);
163+
}
164+
}
165+
147166
void testTmpDir()
148167
{
149168
LogString actual(OptionConverter::substVars(

0 commit comments

Comments
 (0)