Conversation
|
Warning Review limit reachedNext included review available in 29 minutes. View limit detailsLimit details: You’ve used all 8 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 4 remain after this review. 📝 WalkthroughWalkthroughChangesNumeric TX update
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The += and -= branch of SetVar::evaluate() resolves the current value of
the target variable with m_variable->evaluate(), which fills a
std::vector<const VariableValue *> whose elements are owned by the
caller. The elements were deleted only after stoi() had converted the
first one:
value = stoi(l[0]->getValue());
for (auto &i : l) {
delete i;
}
stoi() throws std::invalid_argument when the current value does not
start with a number and std::out_of_range when it does not fit in an
int. Both exceptions are swallowed by the surrounding catch (...), which
only resets value to 0, so the delete loop is skipped and every
VariableValue in the vector is leaked.
This happens on any request that increments a variable holding a non
numeric value, for example a rule set that does
setvar:tx.score=+1 on a tx.score that was previously assigned a string,
so the leak is per request and unbounded in a long running process.
The value is now copied into a local std::string before the vector is
emptied, and stoi() is called afterwards. The behaviour is unchanged:
the conversion still runs inside the same try block and value stays 0
when it throws.
A regression test is added to collection-tx.json: tx.something is first
set to "not_a_number" and then incremented by 10, which must yield 10.
Running collection-tx.json under valgrind reports
152 bytes in 1 blocks are definitely lost
at operator new(unsigned long)
by modsecurity::collection::backend::InMemoryPerProcess::resolveMultiMatches(...)
by modsecurity::variables::Tx_DynamicElement::evaluate(...)
by modsecurity::actions::SetVar::evaluate(...) (set_var.cc:105)
before the change and "All heap blocks were freed -- no leaks are
possible" after it.
7c391e2 to
7ae9b07
Compare
|
1 participant
what
+=/-=branch ofSetVar::evaluate(), copy the current value into a local string and delete the resolvedVariableValueobjects before callingstoi().collection-tx.json:tx.somethingset tonot_a_number, thensetvar:tx.something=+10, expected value10.why
m_variable->evaluate()fills astd::vector<const VariableValue *>owned by the caller. The elements were deleted only afterstoi(l[0]->getValue()).stoithrows on a non-numeric or out-of-range value, the surroundingcatch (...)swallows it and resetsvalueto 0, and the delete loop is skipped, leaking every element.tx.score first assigned from an unresolved macro) leaks per request.stoistill runs inside the sametry, andvalueis still 0 when it throws.Evidence, unfixed tree,
valgrind --leak-check=full ./.libs/regression_tests test-cases/regression/collection-tx.json:With the fix:
All heap blocks were freed -- no leaks are possible;collection-tx.json7/7, whole regression directory 721 passed / 11 skipped.references
Summary by CodeRabbit
Bug Fixes
Tests