Skip to content

Commit 7ac6bf7

Browse files
author
Felipe Zimmerle
committed
Fix memory issues while resolving variables
1 parent 003a8e8 commit 7ac6bf7

File tree

10 files changed

+115
-116
lines changed

10 files changed

+115
-116
lines changed

‎headers/modsecurity/anchored_variable.h

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,10 +58,12 @@ class AnchoredVariable {
5858
std::unique_ptr<std::string> resolveFirst();
5959

6060
Transaction *m_transaction;
61-
collection::Variable *m_var;
6261
int m_offset;
6362
std::string m_name;
6463
std::string m_value;
64+
65+
private:
66+
collection::Variable *m_var;
6567
};
6668

6769
} // namespace modsecurity

‎headers/modsecurity/collection/variable.h

Lines changed: 22 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -37,32 +37,36 @@ namespace collection {
3737
class Variable {
3838
public:
3939
explicit Variable(const std::string *key) :
40-
m_dynamic(true) {
41-
m_key.reset(new std::string(*key));
40+
m_key(""),
41+
m_value("") {
42+
m_key.assign(*key);
4243
}
4344
Variable(const std::string *key, const std::string *value) :
44-
m_dynamic(true) {
45-
m_key.reset(new std::string(*key));
46-
m_value.reset(new std::string(*value));
47-
48-
}
49-
explicit Variable(const std::shared_ptr<std::string> key) :
50-
m_dynamic(true) {
51-
m_key = key;
45+
m_key(""),
46+
m_value("") {
47+
m_key.assign(*key);
48+
m_value.assign(*value);
5249
}
53-
Variable(std::shared_ptr<std::string> key, std::shared_ptr<std::string> value) :
54-
m_dynamic(true) {
55-
m_key = key;
56-
m_value = value;
50+
Variable() :
51+
m_key(""),
52+
m_value("") { }
5753

54+
Variable(const Variable *o) :
55+
m_key(""),
56+
m_value("") {
57+
m_key.assign(o->m_key);
58+
m_value.assign(o->m_value);
59+
for (auto &i : o->m_orign) {
60+
std::unique_ptr<VariableOrigin> origin(new VariableOrigin());
61+
origin->m_offset = i->m_offset;
62+
origin->m_length = i->m_length;
63+
m_orign.push_back(std::move(origin));
5864
}
59-
~Variable() {
6065
}
6166

62-
std::shared_ptr<std::string> m_key;
63-
std::shared_ptr<std::string> m_value;
67+
std::string m_key;
68+
std::string m_value;
6469
std::list<std::unique_ptr<VariableOrigin>> m_orign;
65-
bool m_dynamic;
6670
};
6771

6872
} // namespace collection

‎src/anchored_set_variable.cc

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -55,11 +55,12 @@ void AnchoredSetVariable::set(const std::string &key,
5555
std::string *v = new std::string(value);
5656
std::string *k = new std::string(m_name + ":" + key);
5757
collection::Variable *var = new collection::Variable(k, v);
58+
delete v;
59+
delete k;
5860

5961
origin->m_offset = offset;
6062
origin->m_length = len;
6163

62-
var->m_dynamic = false;
6364
var->m_orign.push_back(std::move(origin));
6465
emplace(key, var);
6566
}
@@ -71,11 +72,12 @@ void AnchoredSetVariable::set(const std::string &key,
7172
std::string *v = new std::string(value);
7273
std::string *k = new std::string(m_name + ":" + key);
7374
collection::Variable *var = new collection::Variable(k, v);
75+
delete v;
76+
delete k;
7477

7578
origin->m_offset = offset;
7679
origin->m_length = value.size();
7780

78-
var->m_dynamic = false;
7981
var->m_orign.push_back(std::move(origin));
8082
emplace(key, var);
8183
}
@@ -84,7 +86,7 @@ void AnchoredSetVariable::set(const std::string &key,
8486
void AnchoredSetVariable::resolve(
8587
std::vector<const collection::Variable *> *l) {
8688
for (const auto& x : *this) {
87-
l->insert(l->begin(), x.second);
89+
l->insert(l->begin(), new collection::Variable(x.second));
8890
}
8991
}
9092

@@ -93,7 +95,7 @@ void AnchoredSetVariable::resolve(const std::string &key,
9395
std::vector<const collection::Variable *> *l) {
9496
auto range = this->equal_range(key);
9597
for (auto it = range.first; it != range.second; ++it) {
96-
l->push_back(it->second);
98+
l->push_back(new collection::Variable(it->second));
9799
}
98100
}
99101

@@ -103,7 +105,7 @@ std::unique_ptr<std::string> AnchoredSetVariable::resolveFirst(
103105
auto range = equal_range(key);
104106
for (auto it = range.first; it != range.second; ++it) {
105107
std::unique_ptr<std::string> b(new std::string());
106-
b->assign(*it->second->m_value);
108+
b->assign(it->second->m_value);
107109
return b;
108110
}
109111
return nullptr;
@@ -117,7 +119,7 @@ void AnchoredSetVariable::resolveRegularExpression(Utils::Regex *r,
117119
if (ret <= 0) {
118120
continue;
119121
}
120-
l->insert(l->begin(), x.second);
122+
l->insert(l->begin(), new collection::Variable(x.second));
121123
}
122124
}
123125

‎src/anchored_variable.cc

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -37,24 +37,19 @@ AnchoredVariable::AnchoredVariable(Transaction *t,
3737
m_value("") {
3838
m_name.append(name);
3939
m_var = new collection::Variable(&m_name);
40-
m_var->m_dynamic = false;
41-
m_var->m_value.reset(&m_value);
4240
}
4341

4442

4543
AnchoredVariable::~AnchoredVariable() {
46-
/*
4744
if (m_var) {
4845
delete (m_var);
4946
m_var = NULL;
5047
}
51-
*/
5248
}
5349

5450

5551
void AnchoredVariable::unset() {
5652
m_value.clear();
57-
m_var->m_orign.clear();
5853
}
5954

6055

@@ -116,11 +111,13 @@ void AnchoredVariable::append(const std::string &a, size_t offset,
116111

117112

118113
void AnchoredVariable::evaluate(std::vector<const collection::Variable *> *l) {
119-
if (m_name.empty() || m_var == NULL || m_var->m_key == NULL
120-
|| m_var->m_value == NULL || m_var->m_key->empty()) {
114+
if (m_name.empty() or m_value.empty()) {
121115
return;
122116
}
123-
l->push_back(m_var);
117+
118+
m_var->m_value.assign(m_value);
119+
collection::Variable *m_var2 = new collection::Variable(m_var);
120+
l->push_back(m_var2);
124121
}
125122

126123

‎src/rule.cc

Lines changed: 32 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -431,9 +431,9 @@ std::list<std::pair<std::shared_ptr<std::string>,
431431

432432
std::vector<std::unique_ptr<collection::Variable>> Rule::getFinalVars(
433433
Transaction *trans) {
434-
std::list<std::shared_ptr<std::string>> exclusions;
435-
std::list<std::shared_ptr<std::string>> exclusions_update_by_tag_remove;
436-
std::list<std::shared_ptr<std::string>> exclusions_update_by_id_remove;
434+
std::list<std::string> exclusions;
435+
std::list<std::string> exclusions_update_by_tag_remove;
436+
std::list<std::string> exclusions_update_by_id_remove;
437437
std::vector<Variables::Variable *> variables;
438438
std::vector<std::unique_ptr<collection::Variable>> finalVars;
439439

@@ -448,8 +448,9 @@ std::vector<std::unique_ptr<collection::Variable>> Rule::getFinalVars(
448448
a.second->evaluateInternal(trans, this, &z);
449449
for (auto &y : z) {
450450
exclusions_update_by_tag_remove.push_back(y->m_key);
451+
delete y;
451452
}
452-
exclusions_update_by_tag_remove.push_back(std::make_shared<std::string>(a.second->m_name));
453+
exclusions_update_by_tag_remove.push_back(a.second->m_name);
453454

454455
} else {
455456
Variable *b = a.second.get();
@@ -466,8 +467,9 @@ std::vector<std::unique_ptr<collection::Variable>> Rule::getFinalVars(
466467
a.second->evaluateInternal(trans, this, &z);
467468
for (auto &y : z) {
468469
exclusions_update_by_id_remove.push_back(y->m_key);
470+
delete y;
469471
}
470-
exclusions_update_by_id_remove.push_back(std::make_shared<std::string>(a.second->m_name));
472+
exclusions_update_by_id_remove.push_back(a.second->m_name);
471473
} else {
472474
Variable *b = a.second.get();
473475
variables.push_back(b);
@@ -481,8 +483,9 @@ std::vector<std::unique_ptr<collection::Variable>> Rule::getFinalVars(
481483
variable->evaluateInternal(trans, this, &z);
482484
for (auto &y : z) {
483485
exclusions.push_back(y->m_key);
486+
delete y;
484487
}
485-
// exclusions.push_back(std::make_shared<std::string>(&variable->m_name));
488+
exclusions.push_back(variable->m_name);
486489
}
487490
}
488491

@@ -497,73 +500,68 @@ std::vector<std::unique_ptr<collection::Variable>> Rule::getFinalVars(
497500

498501
variable->evaluateInternal(trans, this, &e);
499502
for (const collection::Variable *v : e) {
500-
const std::shared_ptr<std::string> key = v->m_key;
503+
std::string key = v->m_key;
504+
501505
if (std::find_if(exclusions.begin(), exclusions.end(),
502-
[key](std::shared_ptr<std::string> m) -> bool { return *key == *m.get(); })
506+
[key](std::string m) -> bool { return key == m; })
503507
!= exclusions.end()) {
504508
#ifndef NO_LOGS
505-
trans->debug(9, "Variable: " + *key +
509+
trans->debug(9, "Variable: " + key +
506510
" is part of the exclusion list, skipping...");
507511
#endif
508-
if (v->m_dynamic) {
509512
delete v;
510513
v = NULL;
511-
}
512514
continue;
513515
}
514516
if (std::find_if(exclusions_update_by_tag_remove.begin(),
515517
exclusions_update_by_tag_remove.end(),
516-
[key](std::shared_ptr<std::string> m) -> bool { return *key == *m.get(); })
518+
[key](std::string m) -> bool { return key == m; })
517519
!= exclusions_update_by_tag_remove.end()) {
518520
#ifndef NO_LOGS
519-
trans->debug(9, "Variable: " + *key +
521+
trans->debug(9, "Variable: " + key +
520522
" is part of the exclusion list (from update by tag" +
521523
"), skipping...");
522524
#endif
523-
if (v->m_dynamic) {
524525
delete v;
525526
v = NULL;
526-
}
527527
continue;
528528
}
529529

530530
if (std::find_if(exclusions_update_by_id_remove.begin(),
531531
exclusions_update_by_id_remove.end(),
532-
[key](std::shared_ptr<std::string> m) -> bool { return *key == *m.get(); })
532+
[key](std::string m) -> bool { return key == m; })
533533
!= exclusions_update_by_id_remove.end()) {
534534
#ifndef NO_LOGS
535-
trans->debug(9, "Variable: " + *key +
535+
trans->debug(9, "Variable: " + key +
536536
" is part of the exclusion list (from update by ID), skipping...");
537537
#endif
538-
if (v->m_dynamic) {
539538
delete v;
540539
v = NULL;
541-
}
542540
continue;
543541
}
544542

545543
for (auto &i : trans->m_ruleRemoveTargetByTag) {
546544
std::string tag = i.first;
547545
std::string args = i.second;
548-
size_t posa = key->find(":");
546+
size_t posa = key.find(":");
549547

550548
if (containsTag(tag, trans) == false) {
551549
continue;
552550
}
553551

554-
if (args == *key) {
552+
if (args == key) {
555553
#ifndef NO_LOGS
556-
trans->debug(9, "Variable: " + *key +
554+
trans->debug(9, "Variable: " + key +
557555
" was excluded by ruleRemoteTargetByTag...");
558556
#endif
559557
ignoreVariable = true;
560558
break;
561559
}
562560
if (posa != std::string::npos) {
563-
std::string var = std::string(*key, posa);
561+
std::string var = std::string(key, posa);
564562
if (var == args) {
565563
#ifndef NO_LOGS
566-
trans->debug(9, "Variable: " + *key +
564+
trans->debug(9, "Variable: " + key +
567565
" was excluded by ruleRemoteTargetByTag...");
568566
#endif
569567
ignoreVariable = true;
@@ -572,33 +570,31 @@ std::vector<std::unique_ptr<collection::Variable>> Rule::getFinalVars(
572570
}
573571
}
574572
if (ignoreVariable) {
575-
if (v->m_dynamic) {
576573
delete v;
577574
v = NULL;
578-
}
579575
continue;
580576
}
581577

582578
for (auto &i : trans->m_ruleRemoveTargetById) {
583579
int id = i.first;
584580
std::string args = i.second;
585-
size_t posa = key->find(":");
581+
size_t posa = key.find(":");
586582

587583
if (m_ruleId != id) {
588584
continue;
589585
}
590586

591-
if (args == *key) {
587+
if (args == key) {
592588
#ifndef NO_LOGS
593-
trans->debug(9, "Variable: " + *key +
589+
trans->debug(9, "Variable: " + key +
594590
" was excluded by ruleRemoveTargetById...");
595591
#endif
596592
ignoreVariable = true;
597593
break;
598594
}
599595
if (posa != std::string::npos) {
600-
if (key->size() > posa) {
601-
std::string var = std::string(*key, 0, posa);
596+
if (key.size() > posa) {
597+
std::string var = std::string(key, 0, posa);
602598
if (var == args) {
603599
#ifndef NO_LOGS
604600
trans->debug(9, "Variable: " + var +
@@ -611,27 +607,14 @@ std::vector<std::unique_ptr<collection::Variable>> Rule::getFinalVars(
611607
}
612608
}
613609
if (ignoreVariable) {
614-
if (v->m_dynamic) {
615610
delete v;
616611
v = NULL;
617-
}
618612
continue;
619613
}
620614

621-
std::unique_ptr<collection::Variable> var(new collection::Variable(
622-
new std::string(*v->m_key),
623-
new std::string(*v->m_value)));
624-
for (auto &i : v->m_orign) {
625-
std::unique_ptr<VariableOrigin> origin(new VariableOrigin());
626-
origin->m_offset = i->m_offset;
627-
origin->m_length = i->m_length;
628-
var->m_orign.push_back(std::move(origin));
629-
}
630-
631-
if (v->m_dynamic) {
632-
delete v;
633-
v = NULL;
634-
}
615+
std::unique_ptr<collection::Variable> var(new collection::Variable(v));
616+
delete v;
617+
v = NULL;
635618
finalVars.push_back(std::move(var));
636619
}
637620
}
@@ -772,8 +755,8 @@ bool Rule::evaluate(Transaction *trans,
772755
finalVars = getFinalVars(trans);
773756

774757
for (auto &v : finalVars) {
775-
const std::string value = *(v->m_value);
776-
const std::string key = *(v->m_key);
758+
const std::string value = v->m_value;
759+
const std::string key = v->m_key;
777760

778761
std::list<std::pair<std::shared_ptr<std::string>,
779762
std::shared_ptr<std::string>>> values;

0 commit comments

Comments
 (0)