Resolve memory leak (bison-generated position.filename)

[martinhsv]

This is a proposed fix to address the most prominent remaining memory leak that can occur when doing nginx reload (but not restart).

The size of the memory leak is proportional to the number of '.conf' filenames being read in by the bison parser.

[airween]

Just a short summary: I checked both current master and patched version with help of ftwrunner (that's the new version) through Valgrind.

The results were:

before the patch:

 2,675 bytes in 38 blocks are indirectly lost in loss record 6 of 7
    at 0x4838DEF: operator new(unsigned long) (vg_replace_malloc.c:342)
    by 0x493CBAE: void std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_construct<char*>(char*, char*, std::forward_iterator_tag) [clone .isra.0] (basic_string.tcc:219)
    by 0x4947CDE: _M_construct_aux<char*> (basic_string.h:247)
    by 0x4947CDE: _M_construct<char*> (basic_string.h:266)
    by 0x4947CDE: basic_string (basic_string.h:451)
    by 0x4947CDE: yylex(modsecurity::Parser::Driver&) (seclang-scanner.ll:1259)
    by 0x49122C3: yy::seclang_parser::parse() (seclang-parser.cc:1372)
    by 0x49690FD: modsecurity::Parser::Driver::parse(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (driver.cc:145)
    by 0x4969419: modsecurity::Parser::Driver::parseFile(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (driver.cc:189)
    by 0x4988E6C: modsecurity::RulesSet::loadFromUri(char const*) (rules_set.cc:53)
    by 0x4988FF2: msc_rules_add_file (rules_set.cc:296)
    by 0x10E9D0: ftw_engine_create_rules_set_msc (ftwmodsecurity.c:40)
    by 0x10E060: ftw_engine_init (engines.c:232)
    by 0x10ADBA: main (main.c:205)
 
 3,891 (1,216 direct, 2,675 indirect) bytes in 38 blocks are definitely lost in loss record 7 of 7
    at 0x4838DEF: operator new(unsigned long) (vg_replace_malloc.c:342)
    by 0x4947CB4: yylex(modsecurity::Parser::Driver&) (seclang-scanner.ll:1259)
    by 0x49122C3: yy::seclang_parser::parse() (seclang-parser.cc:1372)
    by 0x49690FD: modsecurity::Parser::Driver::parse(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (driver.cc:145)
    by 0x4969419: modsecurity::Parser::Driver::parseFile(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (driver.cc:189)
    by 0x4988E6C: modsecurity::RulesSet::loadFromUri(char const*) (rules_set.cc:53)
    by 0x4988FF2: msc_rules_add_file (rules_set.cc:296)
    by 0x10E9D0: ftw_engine_create_rules_set_msc (ftwmodsecurity.c:40)
    by 0x10E060: ftw_engine_init (engines.c:232)
    by 0x10ADBA: main (main.c:205)
 
 LEAK SUMMARY:
    definitely lost: 1,280 bytes in 40 blocks
    indirectly lost: 2,712 bytes in 39 blocks
      possibly lost: 0 bytes in 0 blocks
    still reachable: 192 bytes in 12 blocks
         suppressed: 0 bytes in 0 blocks

after applying the patch:

 LEAK SUMMARY:
    definitely lost: 0 bytes in 0 blocks
    indirectly lost: 0 bytes in 0 blocks
      possibly lost: 0 bytes in 0 blocks
    still reachable: 192 bytes in 12 blocks
         suppressed: 0 bytes in 0 blocks

The still reachable section is because of the libssh2_init, but that's outside the libmodsecurity3.

Btw Valgrind also reports that there are few conditional jumps, like this:

 Conditional jump or move depends on uninitialised value(s)
    at 0x86AEE5A: ???
    by 0x1FFEFFF93F: ???
  Uninitialised value was created by a heap allocation
    at 0x4838DEF: operator new(unsigned long) (vg_replace_malloc.c:342)
    by 0x4990974: modsecurity::RuleWithActions::executeTransformations(modsecurity::Transaction*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::list<std::pair<std::shared_ptr<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::shared_ptr<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >, std::allocator<std::pair<std::shared_ptr<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::shared_ptr<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > > >&) (rule_with_actions.cc:436)
    by 0x4996B05: modsecurity::RuleWithOperator::evaluate(modsecurity::Transaction*, std::shared_ptr<modsecurity::RuleMessage>) (rule_with_operator.cc:308)
    by 0x4991A6C: modsecurity::RuleWithActions::evaluate(modsecurity::Transaction*) (rule_with_actions.cc:177)
    by 0x49874B4: modsecurity::RulesSet::evaluate(int, modsecurity::Transaction*) (rules_set.cc:210)
    by 0x496B974: modsecurity::Transaction::processRequestHeaders() (transaction.cc:580)
    by 0x10EB2D: ftw_engine_runtest_msc (ftwmodsecurity.c:94)
    by 0x10E81E: engine_runtest (engines.c:469)
    by 0x10AC95: main (main.c:233)

in two places: one of them is in line 436 in rules_with_actions.cc, the other one is in line 361.

[dvershinin]

As this issue affects a lot of users of ModSecurity in production, is it possible to release 3.0.9 early? I see a couple of issues in the backlog for 3.0.9, but I think the priority should be given to having fixes like this released.

[martinhsv]

Hello @dvershinin ,

Just curious: have you confirmed that the existing fixes resolve the matter in your installation(s)?

[dvershinin]

@martinhsv not that I can tell because we rely on strictly packaged installations based on tagged releases, so haven't really tested whether these changes resolve memory leaks but indeed those who reload nginx often are the ones affected.