From 064b6267a068dfb1e43819a55b3bc4cc6f0d78cb Mon Sep 17 00:00:00 2001 From: Tomasz Ziolkowski Date: Wed, 24 Feb 2021 12:38:25 +0100 Subject: [PATCH 1/3] Remove memory leak in lmdb.cc while passing data from MDB_val to VariableValue - add new VariableValue constructors for rvalue strings - decrease number of memory copies between lmdb and VariableValue --- headers/modsecurity/variable_value.h | 15 +++++++++++++++ src/collection/backend/lmdb.cc | 13 +++++++------ 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/headers/modsecurity/variable_value.h b/headers/modsecurity/variable_value.h index f787177624..f3a3138f82 100644 --- a/headers/modsecurity/variable_value.h +++ b/headers/modsecurity/variable_value.h @@ -47,6 +47,13 @@ class VariableValue { m_value(value != nullptr?*value:"") { } + VariableValue(std::string&& key, + std::string&& value) + : m_collection(""), + m_key(std::move(key)), + m_value(std::move(value)) + { m_keyWithCollection = m_key; } + VariableValue(const std::string *collection, const std::string *key, const std::string *value) @@ -56,6 +63,14 @@ class VariableValue { m_value(*value) { } + VariableValue(const std::string *collection, + std::string&& key, + std::string&& value) + : m_key(std::move(key)), + m_collection(*collection), + m_value(std::move(value)) + { m_keyWithCollection = m_collection + ":" + m_key; } + explicit VariableValue(const VariableValue *o) : m_collection(o->m_collection), m_key(o->m_key), diff --git a/src/collection/backend/lmdb.cc b/src/collection/backend/lmdb.cc index 1f21e69d99..1e136cc802 100644 --- a/src/collection/backend/lmdb.cc +++ b/src/collection/backend/lmdb.cc @@ -291,6 +291,7 @@ void LMDB::resolveSingleMatch(const std::string& var, reinterpret_cast(mdb_value_ret.mv_data), mdb_value_ret.mv_size); VariableValue *v = new VariableValue(&var, a); + delete a; l->push_back(v); } @@ -498,9 +499,9 @@ void LMDB::resolveMultiMatches(const std::string& var, while ((rc = mdb_cursor_get(cursor, &key, &data, MDB_NEXT)) == 0) { l->insert(l->begin(), new VariableValue( &m_name, - new std::string(reinterpret_cast(key.mv_data), + std::string(reinterpret_cast(key.mv_data), key.mv_size), - new std::string(reinterpret_cast(data.mv_data), + std::string(reinterpret_cast(data.mv_data), data.mv_size))); } } else { @@ -509,9 +510,9 @@ void LMDB::resolveMultiMatches(const std::string& var, if (strncmp(var.c_str(), a, keySize) == 0) { l->insert(l->begin(), new VariableValue( &m_name, - new std::string(reinterpret_cast(key.mv_data), + std::string(reinterpret_cast(key.mv_data), key.mv_size), - new std::string(reinterpret_cast(data.mv_data), + std::string(reinterpret_cast(data.mv_data), data.mv_size))); } } @@ -569,9 +570,9 @@ void LMDB::resolveRegularExpression(const std::string& var, } VariableValue *v = new VariableValue( - new std::string(reinterpret_cast(key.mv_data), + std::string(reinterpret_cast(key.mv_data), key.mv_size), - new std::string(reinterpret_cast(data.mv_data), + std::string(reinterpret_cast(data.mv_data), data.mv_size)); l->insert(l->begin(), v); } From f1c5f6b73b5bb144b8ae7c2ddd6cbdb80999ebd4 Mon Sep 17 00:00:00 2001 From: Tomasz Ziolkowski Date: Wed, 24 Feb 2021 14:19:03 +0100 Subject: [PATCH 2/3] Optimize fetching collection in resolveMultiMatches - 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 --- src/collection/backend/lmdb.cc | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/src/collection/backend/lmdb.cc b/src/collection/backend/lmdb.cc index 1e136cc802..257a4c7b6a 100644 --- a/src/collection/backend/lmdb.cc +++ b/src/collection/backend/lmdb.cc @@ -505,16 +505,21 @@ void LMDB::resolveMultiMatches(const std::string& var, data.mv_size))); } } else { - while ((rc = mdb_cursor_get(cursor, &key, &data, MDB_NEXT)) == 0) { - char *a = reinterpret_cast(key.mv_data); - if (strncmp(var.c_str(), a, keySize) == 0) { - l->insert(l->begin(), new VariableValue( - &m_name, - std::string(reinterpret_cast(key.mv_data), - key.mv_size), - std::string(reinterpret_cast(data.mv_data), - data.mv_size))); - } + string2val(var, &key); + if (rc = mdb_cursor_get(cursor, &key, &data, MDB_SET_RANGE) == 0) { + do { + char *a = reinterpret_cast(key.mv_data); + if (strncmp(var.c_str(), a, keySize) == 0) { + l->insert(l->begin(), new VariableValue( + &m_name, + std::string(reinterpret_cast(key.mv_data), + key.mv_size), + std::string(reinterpret_cast(data.mv_data), + data.mv_size))); + } else { + break; + } + } while ((rc = mdb_cursor_get(cursor, &key, &data, MDB_NEXT)) == 0); } } From 440e3c9611078af88aecf68f5247a71183c4ed03 Mon Sep 17 00:00:00 2001 From: Tomasz Ziolkowski Date: Wed, 24 Feb 2021 17:38:43 +0100 Subject: [PATCH 3/3] Fix static-check violations --- headers/modsecurity/variable_value.h | 4 ++-- src/collection/backend/lmdb.cc | 3 ++- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/headers/modsecurity/variable_value.h b/headers/modsecurity/variable_value.h index f3a3138f82..6996098a1a 100644 --- a/headers/modsecurity/variable_value.h +++ b/headers/modsecurity/variable_value.h @@ -66,8 +66,8 @@ class VariableValue { VariableValue(const std::string *collection, std::string&& key, std::string&& value) - : m_key(std::move(key)), - m_collection(*collection), + : m_collection(*collection), + m_key(std::move(key)), m_value(std::move(value)) { m_keyWithCollection = m_collection + ":" + m_key; } diff --git a/src/collection/backend/lmdb.cc b/src/collection/backend/lmdb.cc index 257a4c7b6a..e863c62d6d 100644 --- a/src/collection/backend/lmdb.cc +++ b/src/collection/backend/lmdb.cc @@ -506,7 +506,8 @@ void LMDB::resolveMultiMatches(const std::string& var, } } else { string2val(var, &key); - if (rc = mdb_cursor_get(cursor, &key, &data, MDB_SET_RANGE) == 0) { + rc = mdb_cursor_get(cursor, &key, &data, MDB_SET_RANGE); + if (rc == 0) { do { char *a = reinterpret_cast(key.mv_data); if (strncmp(var.c_str(), a, keySize) == 0) {