Fix memory leak in lmdb.cc + improve performance while reading collections in resolveMultiMatches #2520
Conversation
…ableValue - add new VariableValue constructors for rvalue strings - decrease number of memory copies between lmdb and VariableValue
- move cursor to first key equal or greather than input key (MDB_SET_RANGE) - break while if input key is not equal key fetched from lmdb
|
Hi @ziollek, thanks for the contribution. I am wondering how to fit the rvalue change on VariableValue of the 3.1-experimental development branch - Here is the key + value constructor - Here is the collection + key + value - Giving that changes on VariableValue, we already have the LMDB in this shape - The benefit of MDB_SET_RANGE change is very clear. What do you think about the changes that we have on 3.1-experimental? |
|
Hi @zimmerle - thanks for your comments. I wasn't aware of VariableValue changes in 3.1-experimental. Despite of internal changes in variable_value.h i'm afraid, there is still a memory leak in lmdb.cc - because of using new std::string. In effect, below constructor is called: Above constructor (unlike constructors quoted in your comment) don't 'move' allocated in lmdb memory - it just copies it. Luckily it could be easily fixed by replacing |
|
Different methods from lmdb back-end are performing this string copy. resolveFirst -resolveSingleMatch -resolveMultiMatches -and resolveRegularExpression -They are using the constructors -
The class VariableValue has four std::string members: m_collection, m_key, m_keyWithCollection, and m_value the attribution of their values happens on the constructor for VariableValue. m_value could be also set via setValue. All attributions are based on the std::string copy constructor. Later the VariableValue is used at - TransactionRunTimeStringRuleWithOperatorAs an alternative the inMemoryBackEnd back-end for collections does not duplicates/copy the strings. Investigating the best approach to tackle this without too much modification on the v3/master code. |
| data.mv_size))); | ||
| } | ||
| string2val(var, &key); | ||
| rc = mdb_cursor_get(cursor, &key, &data, MDB_SET_RANGE); |
There was a problem hiding this comment.
@ziollek replace MDB_SET_RANGE to MDB_SET_KEY
MDB_SET_RANGE - getting first key greater than or equal to specified key http://www.lmdb.tech/doc/group__mdb.html#ga1206b2af8b95e7f6b0ef6b28708c9127
If you are looking for none exist samplekey you get key containing samplekey like samplekey2 (if such key exist)
There was a problem hiding this comment.
Workable link for @sobczak-m comment's - http://www.lmdb.tech/doc/group__mdb.html#gga1206b2af8b95e7f6b0ef6b28708c9127af9feb0557c2954dbf7732eee5e1b59e7
| } else { | ||
| break; | ||
| } | ||
| } while ((rc = mdb_cursor_get(cursor, &key, &data, MDB_NEXT)) == 0); |
There was a problem hiding this comment.
replace MDB_NEXT to MDB_NEXT_DUB to iterate only over current key
There was a problem hiding this comment.
|
I see a benefit of having less copies around, studying the feasibility to have this pull request partially merged on the upcoming 3.0.5. On QA - https://github.com/SpiderLabs/ModSecurity/actions/runs/883266568 |
What have been done ?