Skip to content

Commit cb6629d

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

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
@@ -121,7 +121,7 @@ bool Operator::evaluate(Transaction *transaction,
121121
Operator *Operator::instantiate(std::string op, std::string param_str) {
122122
std::string op_ = utils::string::tolower(op);
123123
std::unique_ptr<RunTimeString> param(new RunTimeString());
124-
param->appendText(param_str);
124+
param->append(param_str);
125125

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

‎src/parser/seclang-parser.cc

+4-4
Original file line numberDiff line numberDiff line change
@@ -5600,7 +5600,7 @@ namespace yy {
56005600
case 436:
56015601
#line 3024 "seclang-parser.yy"
56025602
{
5603-
yystack_[1].value.as < std::unique_ptr<RunTimeString> > ()->appendText(yystack_[0].value.as < std::string > ());
5603+
yystack_[1].value.as < std::unique_ptr<RunTimeString> > ()->append(yystack_[0].value.as < std::string > ());
56045604
yylhs.value.as < std::unique_ptr<RunTimeString> > () = std::move(yystack_[1].value.as < std::unique_ptr<RunTimeString> > ());
56055605
}
56065606
#line 5607 "seclang-parser.cc"
@@ -5609,7 +5609,7 @@ namespace yy {
56095609
case 437:
56105610
#line 3029 "seclang-parser.yy"
56115611
{
5612-
yystack_[1].value.as < std::unique_ptr<RunTimeString> > ()->appendVar(std::move(yystack_[0].value.as < std::unique_ptr<Variable> > ()));
5612+
yystack_[1].value.as < std::unique_ptr<RunTimeString> > ()->append(std::move(yystack_[0].value.as < std::unique_ptr<Variable> > ()));
56135613
yylhs.value.as < std::unique_ptr<RunTimeString> > () = std::move(yystack_[1].value.as < std::unique_ptr<RunTimeString> > ());
56145614
}
56155615
#line 5616 "seclang-parser.cc"
@@ -5619,7 +5619,7 @@ namespace yy {
56195619
#line 3034 "seclang-parser.yy"
56205620
{
56215621
std::unique_ptr<RunTimeString> r(new RunTimeString());
5622-
r->appendText(yystack_[0].value.as < std::string > ());
5622+
r->append(yystack_[0].value.as < std::string > ());
56235623
yylhs.value.as < std::unique_ptr<RunTimeString> > () = std::move(r);
56245624
}
56255625
#line 5626 "seclang-parser.cc"
@@ -5629,7 +5629,7 @@ namespace yy {
56295629
#line 3040 "seclang-parser.yy"
56305630
{
56315631
std::unique_ptr<RunTimeString> r(new RunTimeString());
5632-
r->appendVar(std::move(yystack_[0].value.as < std::unique_ptr<Variable> > ()));
5632+
r->append(std::move(yystack_[0].value.as < std::unique_ptr<Variable> > ()));
56335633
yylhs.value.as < std::unique_ptr<RunTimeString> > () = std::move(r);
56345634
}
56355635
#line 5636 "seclang-parser.cc"

‎src/parser/seclang-parser.yy

+4-4
Original file line numberDiff line numberDiff line change
@@ -3022,24 +3022,24 @@ setvar_action:
30223022
run_time_string:
30233023
run_time_string FREE_TEXT_QUOTE_MACRO_EXPANSION
30243024
{
3025-
$1->appendText($2);
3025+
$1->append($2);
30263026
$$ = std::move($1);
30273027
}
30283028
| run_time_string var
30293029
{
3030-
$1->appendVar(std::move($2));
3030+
$1->append(std::move($2));
30313031
$$ = std::move($1);
30323032
}
30333033
| FREE_TEXT_QUOTE_MACRO_EXPANSION
30343034
{
30353035
std::unique_ptr<RunTimeString> r(new RunTimeString());
3036-
r->appendText($1);
3036+
r->append($1);
30373037
$$ = std::move(r);
30383038
}
30393039
| var
30403040
{
30413041
std::unique_ptr<RunTimeString> r(new RunTimeString());
3042-
r->appendVar(std::move($1));
3042+
r->append(std::move($1));
30433043
$$ = std::move(r);
30443044
}
30453045
;

‎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)