From 944e8385cb5a224e762030e50d0b72f9565fc4bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dirk=20M=C3=BCller?= Date: Tue, 28 Nov 2023 00:03:24 +0100 Subject: [PATCH] removeContradiction() Avoid use-after-free on multiple remove MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit As reported in https://sourceforge.net/p/cppcheck/discussion/general/thread/fa43fb8ab1/ removeContradiction() minValue/maxValue.remove(..) can access free'd memory as it removes all matching values by iterating over the complete list. Creating a full copy instead of a reference avoids this issue. Co-Authored-By: Daniel Marjamäki Signed-off-by: Dirk Müller --- lib/token.cpp | 4 ++-- test/testtoken.cpp | 20 ++++++++++++++++++++ 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/lib/token.cpp b/lib/token.cpp index be6d7730e971..711f64a00e39 100644 --- a/lib/token.cpp +++ b/lib/token.cpp @@ -1997,8 +1997,8 @@ static bool removeContradiction(std::list& values) auto compare = [](const ValueFlow::Value& x, const ValueFlow::Value& y) { return x.compareValue(y, less{}); }; - const ValueFlow::Value& maxValue = std::max(x, y, compare); - const ValueFlow::Value& minValue = std::min(x, y, compare); + ValueFlow::Value maxValue = std::max(x, y, compare); + ValueFlow::Value minValue = std::min(x, y, compare); // TODO: Adjust non-points instead of removing them if (maxValue.isImpossible() && maxValue.bound == ValueFlow::Value::Bound::Upper) { values.remove(minValue); diff --git a/test/testtoken.cpp b/test/testtoken.cpp index 827f6f20c435..1ebf285129dd 100644 --- a/test/testtoken.cpp +++ b/test/testtoken.cpp @@ -1128,6 +1128,26 @@ class TestToken : public TestFixture { ASSERT_EQUALS(true, token.addValue(v2)); ASSERT_EQUALS(false, token.hasKnownIntValue()); } + + void addValueRemoveContradictionCrash() const { + Token tok; + tok.str(":"); + tok.addValue(ValueFlow::Value(0)); + tok.addValue(ValueFlow::Value(8)); + + // !<=-1 + ValueFlow::Value impossibleValue(-1); + impossibleValue.valueKind = ValueFlow::Value::ValueKind::Impossible; + impossibleValue.bound = ValueFlow::Value::Bound::Upper; + tok.addValue(impossibleValue); // Do not crash + + // !>=2 + ValueFlow::Value impossibleValue2(2); + impossibleValue2.valueKind = ValueFlow::Value::ValueKind::Impossible; + impossibleValue2.bound = ValueFlow::Value::Bound::Lower; + tok.addValue(impossibleValue2); // Do not crash + } + }; REGISTER_TEST(TestToken)