Resolve memory leak (bison-generated position.filename) by martinhsv · Pull Request #2876 · owasp-modsecurity/ModSecurity (original) (raw)

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.