removeContradiction() Avoid use-after-free on multiple remove

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 <[email protected]> Signed-off-by: Dirk Müller <[email protected]>
danmar · Dec 17, 2023 · 944e838 · 944e838
1 parent db66105
commit 944e838
Show file tree

Hide file tree

Showing 2 changed files with 22 additions and 2 deletions.
diff --git a/lib/token.cpp b/lib/token.cpp
@@ -1997,8 +1997,8 @@ static bool removeContradiction(std::list<ValueFlow::Value>& 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
@@ -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)