Resolve memory leak (bison-generated position.filename)

src/parser/driver.cc

@@ -34,6 +34,7 @@ Driver::Driver()
 
 
 Driver::~Driver() {
+
     while (loc.empty() == false) {
         yy::location *a = loc.back();
         loc.pop_back();
@@ -129,9 +130,11 @@ int Driver::parse(const std::string &f, const std::string &ref) {
     m_lastRule = nullptr;
     loc.push_back(new yy::location());
     if (ref.empty()) {
-        loc.back()->begin.filename = loc.back()->end.filename = new std::string("<<reference missing or not informed>>");
+        m_filenames.push_back("<<reference missing or not informed>>");
+        loc.back()->begin.filename = loc.back()->end.filename = &(m_filenames.back());
     } else {
-        loc.back()->begin.filename = loc.back()->end.filename = new std::string(ref);
+        m_filenames.push_back(ref);
+        loc.back()->begin.filename = loc.back()->end.filename = &(m_filenames.back());
     }
 
     if (f.empty()) {

src/parser/driver.h

@@ -53,14 +53,6 @@ typedef struct Driver_t Driver;
 #endif
 
 
-/**
- *
- * FIXME: There is a memory leak in the filename at yy::location.
- *        The filename should be converted into a shared string to
- *        save memory or be associated with the life cycle of the
- *        driver class.
- *
- **/
 class Driver : public RulesSetProperties {
  public:
     Driver();
@@ -92,6 +84,13 @@ class Driver : public RulesSetProperties {
     RuleWithActions *m_lastRule;
 
     RulesSetPhases m_rulesSetPhases;
+
+    // Retain a list of new'd filenames so that they are available during the lifetime
+    // of the Driver object, but so that they will get cleaned up by the Driver
+    // destructor. This is to resolve a memory leak of  yy.position.filename in location.hh.
+    // Ordinarily other solutions would have been preferable, but location.hh is a
+    // bison-generated file, which makes some alternative solutions impractical.
+    std::list<std::string> m_filenames;
 };
 
 

src/parser/seclang-parser.yy

@@ -319,7 +319,8 @@ using namespace modsecurity::operators;
 %initial-action
 {
   // Initialize the initial location.
-  @$.begin.filename = @$.end.filename = new std::string(driver.file);
+  driver.m_filenames.push_back(driver.file);
+  @$.begin.filename = @$.end.filename = &(driver.m_filenames.back());
 };
 %define parse.trace
 %define parse.error verbose

src/parser/seclang-scanner.cc

@@ -5106,7 +5106,7 @@ static const flex_int16_t yy_rule_linenum[544] =
      1177, 1178, 1179, 1180, 1182, 1183, 1184, 1185, 1187, 1188,
      1189, 1190, 1192, 1194, 1195, 1197, 1198, 1199, 1200, 1202,
      1207, 1208, 1209, 1213, 1214, 1215, 1220, 1222, 1223, 1224,
-     1243, 1271, 1301
+     1243, 1272, 1303
     } ;
 
 /* The intent behind this definition is that it'll catch
@@ -8546,7 +8546,8 @@ YY_RULE_SETUP
         std::string err;
         std::string f = modsecurity::utils::find_resource(s, *driver.loc.back()->end.filename, &err);
         driver.loc.push_back(new yy::location());
-        driver.loc.back()->begin.filename = driver.loc.back()->end.filename = new std::string(f);
+        driver.m_filenames.push_back(f);
+        driver.loc.back()->begin.filename = driver.loc.back()->end.filename = &(driver.m_filenames.back());
         yyin = fopen(f.c_str(), "r" );
         if (!yyin) {
             BEGIN(INITIAL);
@@ -8560,7 +8561,7 @@ YY_RULE_SETUP
 	YY_BREAK
 case 542:
 YY_RULE_SETUP
-#line 1271 "seclang-scanner.ll"
+#line 1272 "seclang-scanner.ll"
 {
     std::string err;
     const char *tmpStr = yytext + strlen("include");
@@ -8577,7 +8578,8 @@ YY_RULE_SETUP
     for (auto& s: files) {
         std::string f = modsecurity::utils::find_resource(s, *driver.loc.back()->end.filename, &err);
         driver.loc.push_back(new yy::location());
-        driver.loc.back()->begin.filename = driver.loc.back()->end.filename = new std::string(f);
+        driver.m_filenames.push_back(f);
+        driver.loc.back()->begin.filename = driver.loc.back()->end.filename = &(driver.m_filenames.back());
 
         yyin = fopen(f.c_str(), "r" );
         if (!yyin) {
@@ -8594,7 +8596,7 @@ YY_RULE_SETUP
 case 543:
 /* rule 543 can match eol */
 YY_RULE_SETUP
-#line 1301 "seclang-scanner.ll"
+#line 1303 "seclang-scanner.ll"
 {
     HttpsClient c;
     std::string key;
@@ -8610,7 +8612,8 @@ YY_RULE_SETUP
     c.setKey(key);
 
     driver.loc.push_back(new yy::location());
-    driver.loc.back()->begin.filename = driver.loc.back()->end.filename = new std::string(url);
+    driver.m_filenames.push_back(url);
+    driver.loc.back()->begin.filename = driver.loc.back()->end.filename = &(driver.m_filenames.back());
     YY_BUFFER_STATE temp = YY_CURRENT_BUFFER;
     yypush_buffer_state(temp);
 
@@ -8632,10 +8635,10 @@ YY_RULE_SETUP
 	YY_BREAK
 case 544:
 YY_RULE_SETUP
-#line 1337 "seclang-scanner.ll"
+#line 1340 "seclang-scanner.ll"
 ECHO;
 	YY_BREAK
-#line 8639 "seclang-scanner.cc"
+#line 8642 "seclang-scanner.cc"
 
 	case YY_END_OF_BUFFER:
 		{
@@ -9740,7 +9743,7 @@ void yyfree (void * ptr )
 
 /* %ok-for-header */
 
-#line 1337 "seclang-scanner.ll"
+#line 1340 "seclang-scanner.ll"
 
 
 namespace modsecurity {

src/parser/seclang-scanner.ll

@@ -1255,7 +1255,8 @@ EQUALS_MINUS                            (?i:=\-)
         std::string err;
         std::string f = modsecurity::utils::find_resource(s, *driver.loc.back()->end.filename, &err);
         driver.loc.push_back(new yy::location());
-        driver.loc.back()->begin.filename = driver.loc.back()->end.filename = new std::string(f);
+        driver.m_filenames.push_back(f);
+        driver.loc.back()->begin.filename = driver.loc.back()->end.filename = &(driver.m_filenames.back());
         yyin = fopen(f.c_str(), "r" );
         if (!yyin) {
             BEGIN(INITIAL);
@@ -1283,7 +1284,8 @@ EQUALS_MINUS                            (?i:=\-)
     for (auto& s: files) {
         std::string f = modsecurity::utils::find_resource(s, *driver.loc.back()->end.filename, &err);
         driver.loc.push_back(new yy::location());
-        driver.loc.back()->begin.filename = driver.loc.back()->end.filename = new std::string(f);
+        driver.m_filenames.push_back(f);
+        driver.loc.back()->begin.filename = driver.loc.back()->end.filename = &(driver.m_filenames.back());
 
         yyin = fopen(f.c_str(), "r" );
         if (!yyin) {
@@ -1312,7 +1314,8 @@ EQUALS_MINUS                            (?i:=\-)
     c.setKey(key);
 
     driver.loc.push_back(new yy::location());
-    driver.loc.back()->begin.filename = driver.loc.back()->end.filename = new std::string(url);
+    driver.m_filenames.push_back(url);
+    driver.loc.back()->begin.filename = driver.loc.back()->end.filename = &(driver.m_filenames.back());
     YY_BUFFER_STATE temp = YY_CURRENT_BUFFER;
     yypush_buffer_state(temp);
 

test/cppcheck_suppressions.txt

@@ -60,7 +60,7 @@ ctunullpointer:src/rule_with_operator.cc:135
 ctunullpointer:src/rule_with_operator.cc:95
 passedByValue:src/variables/global.h:109
 passedByValue:src/variables/global.h:110
-passedByValue:src/parser/driver.cc:45
+passedByValue:src/parser/driver.cc:46
 passedByValue:test/common/modsecurity_test.cc:49
 passedByValue:test/common/modsecurity_test.cc:98
 unreadVariable:src/rule_with_operator.cc:219