Fix memory leak in lmdb.cc + improve performance while reading collections in resolveMultiMatches #2520

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Open

ziollek wants to merge 4 commits into owasp-modsecurity:v3/master

from ziollek:v3/master

Open

Fix memory leak in lmdb.cc + improve performance while reading collections in resolveMultiMatches #2520

ziollek wants to merge 4 commits into owasp-modsecurity:v3/master from ziollek:v3/master

Conversation

ziollek

Copy link

Contributor

@ziollek ziollek commented Feb 24, 2021 •

edited

Loading

What have been done ?

Removed memory leak in lmdb.cc while passing data from MDB_val to VariableValue:

added new VariableValue constructors for rvalue strings
decreased memory footprint (just one data copying between lmdb and VariableValue thanks to above constructors)

Optimized fetching collection in resolveMultiMatches:

at the beginning, the cursor is moved to a key equal to or greater than the input key (MDB_SET_RANGE)
values are fetched via cursor as long as the input key is equal to the key fetched from lmdb

ziollek added 3 commits

February 24, 2021 12:38

@ziollek


 Remove memory leak in lmdb.cc while passing data from MDB_val to Vari...

064b626

...ableValue
- add new VariableValue constructors for rvalue strings
- decrease number of memory copies between lmdb and VariableValue

@ziollek


 Optimize fetching collection in resolveMultiMatches

f1c5f6b

- 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

@ziollek


 Fix static-check violations

440e3c9

smigacz

smigacz approved these changes

Feb 25, 2021

View reviewed changes

@zimmerle zimmerle self-requested a review

February 26, 2021 14:41

@zimmerle

Copy link

Contributor

zimmerle commented Feb 26, 2021

Hi @ziollek, thanks for the contribution.

I am wondering how to fit the rvalue change on VariableValue of the 3.1-experimental development branch -

https://github.com/SpiderLabs/ModSecurity/blob/9f47f1473c88a5b105ced2f11c87204a0abba424/headers/modsecurity/variable_value.h#L51-L59

Here is the key + value constructor -
https://github.com/SpiderLabs/ModSecurity/blob/9f47f1473c88a5b105ced2f11c87204a0abba424/headers/modsecurity/variable_value.h#L98-L108

Here is the collection + key + value -
https://github.com/SpiderLabs/ModSecurity/blob/9f47f1473c88a5b105ced2f11c87204a0abba424/headers/modsecurity/variable_value.h#L111-L184

Giving that changes on VariableValue, we already have the LMDB in this shape -
https://github.com/SpiderLabs/ModSecurity/blob/9f47f1473c88a5b105ced2f11c87204a0abba424/src/collection/backend/lmdb.cc#L498-L516

The benefit of MDB_SET_RANGE change is very clear. What do you think about the changes that we have on 3.1-experimental?

@zimmerle zimmerle self-assigned this

Feb 26, 2021

@zimmerle zimmerle added the 3.x label

Feb 26, 2021

@ziollek

Copy link

Contributor Author

ziollek commented Feb 26, 2021 •

edited

Loading

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:

https://github.com/SpiderLabs/ModSecurity/blob/9f47f1473c88a5b105ced2f11c87204a0abba424/headers/modsecurity/variable_value.h#L142-L151

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
new std::string(...) to std::make_unique<std::string>(...)

@zimmerle zimmerle mentioned this pull request

Mar 17, 2021

ModSecurity memory leak owasp-modsecurity/ModSecurity-nginx#236

Closed

@zimmerle

Copy link

Contributor

zimmerle commented Mar 19, 2021

Different methods from lmdb back-end are performing this string copy.

and

https://github.com/SpiderLabs/ModSecurity/blob/4127c1bf52d2b30a5c2c3e641b8085fd9a720f43/src/collection/backend/lmdb.cc#L510-L515

resolveRegularExpression -

https://github.com/SpiderLabs/ModSecurity/blob/4127c1bf52d2b30a5c2c3e641b8085fd9a720f43/src/collection/backend/lmdb.cc#L571-L575

They are using the constructors -

Method	Constructor	Leak on v3/master
resolveFirst	https://github.com/SpiderLabs/ModSecurity/blob/4127c1bf52d2b30a5c2c3e641b8085fd9a720f43/headers/modsecurity/variable_value.h#L42-L48	Likely
resolveSingleMatch	https://github.com/SpiderLabs/ModSecurity/blob/4127c1bf52d2b30a5c2c3e641b8085fd9a720f43/headers/modsecurity/variable_value.h#L42-L48	Likely
resolveMultiMatches (a)	https://github.com/SpiderLabs/ModSecurity/blob/4127c1bf52d2b30a5c2c3e641b8085fd9a720f43/headers/modsecurity/variable_value.h#L50-L57	likely
resolveMultiMatches (b)	https://github.com/SpiderLabs/ModSecurity/blob/4127c1bf52d2b30a5c2c3e641b8085fd9a720f43/headers/modsecurity/variable_value.h#L50-L57	Likely
resolveRegularExpression	https://github.com/SpiderLabs/ModSecurity/blob/4127c1bf52d2b30a5c2c3e641b8085fd9a720f43/headers/modsecurity/variable_value.h#L42-L48	Likely

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 -

Transaction

https://github.com/SpiderLabs/ModSecurity/blob/4127c1bf52d2b30a5c2c3e641b8085fd9a720f43/src/transaction.cc#L913-L918

https://github.com/SpiderLabs/ModSecurity/blob/4127c1bf52d2b30a5c2c3e641b8085fd9a720f43/src/transaction.cc#L1533-L1539

https://github.com/SpiderLabs/ModSecurity/blob/4127c1bf52d2b30a5c2c3e641b8085fd9a720f43/src/transaction.cc#L1571-L1576

https://github.com/SpiderLabs/ModSecurity/blob/4127c1bf52d2b30a5c2c3e641b8085fd9a720f43/src/transaction.cc#L1670-L1674

https://github.com/SpiderLabs/ModSecurity/blob/4127c1bf52d2b30a5c2c3e641b8085fd9a720f43/src/transaction.cc#L1700-L1704

RunTimeString

https://github.com/SpiderLabs/ModSecurity/blob/4127c1bf52d2b30a5c2c3e641b8085fd9a720f43/src/run_time_string.cc#L63-L66

RuleWithOperator

https://github.com/SpiderLabs/ModSecurity/blob/4127c1bf52d2b30a5c2c3e641b8085fd9a720f43/src/rule_with_operator.cc#L278-L338

As 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.

sobczak-m

sobczak-m reviewed

Mar 24, 2021

View reviewed changes

src/collection/backend/lmdb.cc

data.mv_size)));

}

string2val(var, &key);

rc = mdb_cursor_get(cursor, &key, &data, MDB_SET_RANGE);

Copy link

@sobczak-m sobczak-m Mar 24, 2021 •

edited

Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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)

Copy link

Contributor

@zimmerle zimmerle May 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Workable link for @sobczak-m comment's - http://www.lmdb.tech/doc/group__mdb.html#gga1206b2af8b95e7f6b0ef6b28708c9127af9feb0557c2954dbf7732eee5e1b59e7

sobczak-m

sobczak-m reviewed

Apr 13, 2021

View reviewed changes

src/collection/backend/lmdb.cc

} else {

break;

}

} while ((rc = mdb_cursor_get(cursor, &key, &data, MDB_NEXT)) == 0);

Copy link

@sobczak-m sobczak-m Apr 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replace MDB_NEXT to MDB_NEXT_DUB to iterate only over current key

Copy link

Contributor

@zimmerle zimmerle May 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MDB_NEXT_DUP -
http://www.lmdb.tech/doc/group__mdb.html#gga1206b2af8b95e7f6b0ef6b28708c9127a37d22eecd320d77fbba9f905483f99e5

@zimmerle

Copy link

Contributor

zimmerle commented May 28, 2021

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

@ziollek


 Merge branch 'SpiderLabs:v3/master' into v3/master

d980bef

Labels

3.x

4 participants

@ziollek @zimmerle @smigacz @sobczak-m

Fix memory leak in lmdb.cc + improve performance while reading collections in resolveMultiMatches #2520

Are you sure you want to change the base?

Fix memory leak in lmdb.cc + improve performance while reading collections in resolveMultiMatches #2520

Uh oh!

Conversation

@ziollek ziollek commented Feb 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zimmerle commented Feb 26, 2021

Uh oh!

ziollek commented Feb 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zimmerle commented Mar 19, 2021

resolveFirst -

resolveSingleMatch -

resolveMultiMatches -

resolveRegularExpression -

Transaction

RunTimeString

RuleWithOperator

Uh oh!

@sobczak-m sobczak-m Mar 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

@zimmerle zimmerle May 27, 2021

Choose a reason for hiding this comment

Uh oh!

@sobczak-m sobczak-m Apr 13, 2021

Choose a reason for hiding this comment

Uh oh!

@zimmerle zimmerle May 27, 2021

Choose a reason for hiding this comment

Uh oh!

zimmerle commented May 28, 2021

Uh oh!

Uh oh!

@ziollek ziollek commented Feb 24, 2021 •

edited

Loading

ziollek commented Feb 26, 2021 •

edited

Loading

@sobczak-m sobczak-m Mar 24, 2021 •

edited

Loading