Skip to content

Commit ca531fd

Browse files
zimmerlezimmerle
committed
Having RunTimeString in a better shape
This is an effort towards better understanding the issues reported on #2376
1 parent e2bf130 commit ca531fd

File tree

5 files changed

+118
-69
lines changed

5 files changed

+118
-69
lines changed

‎src/operators/operator.cc

+1-1
Original file line numberDiff line numberDiff line change
@@ -122,7 +122,7 @@ bool Operator::evaluate(Transaction *transaction,
122122
Operator *Operator::instantiate(std::string op, std::string param_str) {
123123
std::string op_ = utils::string::tolower(op);
124124
std::unique_ptr<RunTimeString> param(new RunTimeString());
125-
param->appendText(param_str);
125+
param->append(param_str);
126126

127127
IF_MATCH(beginswith) { return new BeginsWith(std::move(param)); }
128128
IF_MATCH(contains) { return new Contains(std::move(param)); }

‎src/parser/seclang-parser.cc

+4-4
Original file line numberDiff line numberDiff line change
@@ -5621,7 +5621,7 @@ namespace yy {
56215621
case 438: // run_time_string: run_time_string "FREE_TEXT_QUOTE_MACRO_EXPANSION"
56225622
#line 3036 "seclang-parser.yy"
56235623
{
5624-
yystack_[1].value.as < std::unique_ptr<RunTimeString> > ()->appendText(yystack_[0].value.as < std::string > ());
5624+
yystack_[1].value.as < std::unique_ptr<RunTimeString> > ()->append(yystack_[0].value.as < std::string > ());
56255625
yylhs.value.as < std::unique_ptr<RunTimeString> > () = std::move(yystack_[1].value.as < std::unique_ptr<RunTimeString> > ());
56265626
}
56275627
#line 5628 "seclang-parser.cc"
@@ -5630,7 +5630,7 @@ namespace yy {
56305630
case 439: // run_time_string: run_time_string var
56315631
#line 3041 "seclang-parser.yy"
56325632
{
5633-
yystack_[1].value.as < std::unique_ptr<RunTimeString> > ()->appendVar(std::move(yystack_[0].value.as < std::unique_ptr<Variable> > ()));
5633+
yystack_[1].value.as < std::unique_ptr<RunTimeString> > ()->append(std::move(yystack_[0].value.as < std::unique_ptr<Variable> > ()));
56345634
yylhs.value.as < std::unique_ptr<RunTimeString> > () = std::move(yystack_[1].value.as < std::unique_ptr<RunTimeString> > ());
56355635
}
56365636
#line 5637 "seclang-parser.cc"
@@ -5640,7 +5640,7 @@ namespace yy {
56405640
#line 3046 "seclang-parser.yy"
56415641
{
56425642
std::unique_ptr<RunTimeString> r(new RunTimeString());
5643-
r->appendText(yystack_[0].value.as < std::string > ());
5643+
r->append(yystack_[0].value.as < std::string > ());
56445644
yylhs.value.as < std::unique_ptr<RunTimeString> > () = std::move(r);
56455645
}
56465646
#line 5647 "seclang-parser.cc"
@@ -5650,7 +5650,7 @@ namespace yy {
56505650
#line 3052 "seclang-parser.yy"
56515651
{
56525652
std::unique_ptr<RunTimeString> r(new RunTimeString());
5653-
r->appendVar(std::move(yystack_[0].value.as < std::unique_ptr<Variable> > ()));
5653+
r->append(std::move(yystack_[0].value.as < std::unique_ptr<Variable> > ()));
56545654
yylhs.value.as < std::unique_ptr<RunTimeString> > () = std::move(r);
56555655
}
56565656
#line 5657 "seclang-parser.cc"

‎src/parser/seclang-parser.yy

+4-4
Original file line numberDiff line numberDiff line change
@@ -3034,24 +3034,24 @@ setvar_action:
30343034
run_time_string:
30353035
run_time_string FREE_TEXT_QUOTE_MACRO_EXPANSION
30363036
{
3037-
$1->appendText($2);
3037+
$1->append($2);
30383038
$$ = std::move($1);
30393039
}
30403040
| run_time_string var
30413041
{
3042-
$1->appendVar(std::move($2));
3042+
$1->append(std::move($2));
30433043
$$ = std::move($1);
30443044
}
30453045
| FREE_TEXT_QUOTE_MACRO_EXPANSION
30463046
{
30473047
std::unique_ptr<RunTimeString> r(new RunTimeString());
3048-
r->appendText($1);
3048+
r->append($1);
30493049
$$ = std::move(r);
30503050
}
30513051
| var
30523052
{
30533053
std::unique_ptr<RunTimeString> r(new RunTimeString());
3054-
r->appendVar(std::move($1));
3054+
r->append(std::move($1));
30553055
$$ = std::move(r);
30563056
}
30573057
;

‎src/run_time_string.cc

+6-22
Original file line numberDiff line numberDiff line change
@@ -31,38 +31,22 @@
3131
namespace modsecurity {
3232

3333

34-
void RunTimeString::appendText(const std::string &text) {
35-
std::unique_ptr<RunTimeElementHolder> r(new RunTimeElementHolder);
36-
r->m_string = text;
37-
m_elements.push_back(std::move(r));
34+
void RunTimeString::append(const std::string &text) {
35+
m_elements.emplace_back(new ElementHolder(text));
3836
}
3937

4038

41-
void RunTimeString::appendVar(
42-
std::unique_ptr<modsecurity::variables::Variable> var) {
43-
std::unique_ptr<RunTimeElementHolder> r(new RunTimeElementHolder);
44-
r->m_variable = std::move(var);
45-
m_elements.push_back(std::move(r));
39+
void RunTimeString::append(std::unique_ptr<Variable> var) {
40+
m_elements.emplace_back(new ElementHolder(std::move(var)));
4641
m_containsMacro = true;
4742
}
4843

4944

50-
std::string RunTimeString::evaluate(Transaction *transaction) {
45+
std::string RunTimeString::evaluate(/* const */ Transaction *transaction) const noexcept {
5146
std::string retString;
5247
// FIXME: Educated guess the size of retString based on the size of the elements.
5348
for (auto &element : m_elements) {
54-
if (element->m_string.size() > 0) {
55-
retString.append(element->m_string);
56-
} else if (element->m_variable != nullptr && transaction != nullptr) {
57-
std::vector<const VariableValue *> l;
58-
element->m_variable->evaluate(transaction, &l);
59-
if (!l.empty()) {
60-
retString.append(l[0]->getValue());
61-
}
62-
for (auto &i : l) {
63-
delete i;
64-
}
65-
}
49+
element->appendValueTo(transaction, retString);
6650
}
6751
return retString;
6852
}

‎src/run_time_string.h

+103-38
Original file line numberDiff line numberDiff line change
@@ -33,33 +33,11 @@
3333

3434
namespace modsecurity {
3535

36-
class RunTimeElementHolder {
37-
public:
38-
RunTimeElementHolder()
39-
: m_string(""),
40-
m_variable(nullptr)
41-
{ };
42-
43-
44-
RunTimeElementHolder(const RunTimeElementHolder &other)
45-
: m_string(other.m_string),
46-
m_variable(other.m_variable) {
47-
variables::RuleVariable *rv = dynamic_cast<variables::RuleVariable *>(m_variable.get());
48-
if (rv != nullptr) {
49-
auto nrv = rv->clone();
50-
rv = dynamic_cast<variables::RuleVariable *>(nrv);
51-
rv->populate(nullptr);
52-
m_variable = std::unique_ptr<variables::Variable>(nrv);
53-
}
54-
};
55-
56-
/* protected: */
57-
std::string m_string;
58-
std::shared_ptr<variables::Variable> m_variable;
59-
};
60-
6136
class RunTimeString {
6237
public:
38+
using Variable = variables::Variable;
39+
using RuleVariable = variables::RuleVariable;
40+
6341
RunTimeString()
6442
: m_containsMacro(false),
6543
m_elements()
@@ -71,37 +49,124 @@ class RunTimeString {
7149
m_elements()
7250
{
7351
for (auto &m : other.m_elements) {
74-
m_elements.push_back(std::unique_ptr<RunTimeElementHolder>(new RunTimeElementHolder(*m.get())));
52+
m_elements.emplace_back(new ElementHolder(*m.get()));
7553
}
7654
};
7755

56+
RunTimeString& operator=(RunTimeString other)
57+
{
58+
m_containsMacro = other.m_containsMacro;
59+
for (auto &m : other.m_elements) {
60+
m_elements.emplace_back(new ElementHolder(*m.get()));
61+
}
62+
return *this;
63+
}
7864

79-
void appendText(const std::string &text);
80-
void appendVar(std::unique_ptr<modsecurity::variables::Variable> var);
8165

66+
void append(const std::string &text);
67+
void append(std::unique_ptr<Variable> var);
8268

83-
std::string evaluate(Transaction *t);
8469

85-
inline std::string evaluate() {
86-
return evaluate(NULL);
87-
}
70+
/*
71+
*
72+
* FIXME: Transaction should be const here. Variables resolution does
73+
* not change anything on transaction instance.
74+
*
75+
*/
76+
std::string evaluate(/* const */ Transaction *t = nullptr) const noexcept;
8877

8978

90-
inline bool containsMacro() const { return m_containsMacro; }
79+
inline bool containsMacro() const noexcept {
80+
return m_containsMacro;
81+
}
9182

9283

93-
void populate(RuleWithActions *rule) {
84+
void populate(RuleWithActions *rule) noexcept {
9485
for (auto &a : m_elements) {
95-
modsecurity::variables::RuleVariable *vrule = dynamic_cast<variables::RuleVariable *>(a->m_variable.get());
86+
a->populate(rule);
87+
}
88+
}
89+
90+
91+
class ElementHolder {
92+
public:
93+
ElementHolder()
94+
: m_string(""),
95+
m_variable(nullptr)
96+
{ };
97+
98+
explicit ElementHolder(std::unique_ptr<Variable> variable)
99+
: m_string(""),
100+
m_variable(std::move(variable))
101+
{ };
102+
103+
explicit ElementHolder(const std::string &str)
104+
: m_string(str),
105+
m_variable(nullptr)
106+
{ };
107+
108+
ElementHolder(const ElementHolder &other)
109+
: m_string(other.m_string),
110+
m_variable(nullptr) {
111+
RuleVariable *rv = dynamic_cast<RuleVariable *>(other.m_variable.get());
112+
if (rv != nullptr) {
113+
auto nrv = rv->clone();
114+
rv = dynamic_cast<RuleVariable *>(nrv);
115+
rv->populate(nullptr);
116+
m_variable = std::unique_ptr<Variable>(nrv);
117+
/* m_variable = nullptr; */
118+
} else {
119+
m_variable = other.m_variable;
120+
}
121+
122+
};
123+
124+
125+
void appendValueTo(/* const */ Transaction *transaction, std::string &v) const noexcept {
126+
if (m_variable && transaction) {
127+
std::vector<const VariableValue *> l;
128+
m_variable->evaluate(transaction, &l);
129+
if (!l.empty()) {
130+
v.append(l[0]->getValue());
131+
}
132+
for (auto &i : l) {
133+
delete i;
134+
}
135+
136+
return;
137+
}
138+
139+
v.append(m_string);
140+
}
141+
142+
143+
void populate(RuleWithActions *rule) noexcept {
144+
if (!m_variable) {
145+
return;
146+
}
147+
148+
RuleVariable *vrule = dynamic_cast<RuleVariable *>(m_variable.get());
96149
if (vrule != nullptr) {
97150
vrule->populate(rule);
98151
}
99152
}
100-
}
101153

154+
private:
155+
std::string m_string;
156+
/*
157+
*
158+
* FIXME: In the current state m_variable should be a unique_ptr. There
159+
* is no copy for variables, thus having a shared pointer here.
160+
* As an optimization we can have it as a shared_ptr to reduce the
161+
* memory footprint in anchored variables.
162+
*
163+
*/
164+
std::shared_ptr<Variable> m_variable;
165+
};
166+
102167
private:
103-
bool m_containsMacro;
104-
std::list<std::unique_ptr<RunTimeElementHolder>> m_elements;
168+
bool m_containsMacro:1;
169+
std::vector<std::unique_ptr<ElementHolder>> m_elements;
105170
};
106171

107172

0 commit comments

Comments
 (0)