Skip to content
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

Fix issue 9979: false positive: containerOutOfBounds with conditional resize #3136

Merged
merged 27 commits into from
Mar 30, 2021
Merged
Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
82810ed
Fix issue 9979: false positive: containerOutOfBounds with conditional…
pfultz2 Feb 18, 2021
e1375b8
Remove commented out code
pfultz2 Feb 18, 2021
bc9ebee
Add more tests
pfultz2 Feb 18, 2021
2a3e9ba
Remove adjacent values
pfultz2 Feb 19, 2021
b0f298b
Dont truncate impossible values
pfultz2 Feb 19, 2021
07ee205
Fix some tests
pfultz2 Feb 19, 2021
75e8916
Fix bug in removing contradictions
pfultz2 Feb 19, 2021
d7d4c44
Fix more bugs in removing contradictions
pfultz2 Feb 20, 2021
56a1408
Skip literals
pfultz2 Feb 20, 2021
c9a2d1a
Flip assert
pfultz2 Feb 20, 2021
680a4c6
Fix valueflow tests
pfultz2 Feb 20, 2021
64e3b01
Dont set literals
pfultz2 Feb 20, 2021
31b7067
Format
pfultz2 Feb 20, 2021
9a22bc8
Remove unnecessary headers
pfultz2 Feb 20, 2021
0ef3651
Fix uninitialized variables
pfultz2 Feb 22, 2021
649cb59
Merge
pfultz2 Feb 22, 2021
a53726d
Prevent UB in calculate function
pfultz2 Feb 22, 2021
64fd616
Fix cppcheck warning
pfultz2 Feb 22, 2021
305f03a
Merge branch 'main' into valueflow-infer-unsigned-compare
pfultz2 Feb 24, 2021
ad89530
Check value type
pfultz2 Feb 24, 2021
65f4519
Format
pfultz2 Feb 24, 2021
87e38aa
Merge branch 'main' into valueflow-infer-unsigned-compare
pfultz2 Feb 25, 2021
b3af86c
Make test todo
pfultz2 Feb 28, 2021
62d7377
Merge branch 'main' into valueflow-infer-unsigned-compare
pfultz2 Mar 1, 2021
0377a41
Remove else
pfultz2 Mar 30, 2021
c8fcd31
Merge branch 'main' into valueflow-infer-unsigned-compare
pfultz2 Mar 30, 2021
0ccd789
Rerun dmake
pfultz2 Mar 30, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion addons/test/misra/misra-test.c
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ void misra_7_2() {
unsigned short h = 0x8000U;
unsigned int i = 0x80000000; // 7.2
unsigned int j = 0x80000000U;
unsigned long long k = 0x8000000000000000; // 7.2
unsigned long long k = 0x8000000000000000; // TODO 7.2
unsigned long long l = 0x8000000000000000ULL;

unsigned int m = 1 + 0x80000000; // 7.2 10.4
Expand Down
5 changes: 5 additions & 0 deletions lib/astutils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,11 @@ bool astIsIntegral(const Token *tok, bool unknown)
return vt->isIntegral() && vt->pointer == 0U;
}

bool astIsUnsigned(const Token* tok)
{
return tok && tok->valueType() && tok->valueType()->sign == ValueType::UNSIGNED;
}

bool astIsFloat(const Token *tok, bool unknown)
{
const ValueType *vt = tok ? tok->valueType() : nullptr;
Expand Down
1 change: 1 addition & 0 deletions lib/astutils.h
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ bool astIsSignedChar(const Token *tok);
bool astIsUnknownSignChar(const Token *tok);
/** Is expression of integral type? */
bool astIsIntegral(const Token *tok, bool unknown);
bool astIsUnsigned(const Token* tok);
/** Is expression of floating point type? */
bool astIsFloat(const Token *tok, bool unknown);
/** Is expression of boolean type? */
Expand Down
43 changes: 29 additions & 14 deletions lib/programmemory.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#include "mathlib.h"
#include "symboldatabase.h"
#include "token.h"
#include "valueflow.h"
#include <algorithm>
#include <cassert>
#include <cstdio>
Expand Down Expand Up @@ -424,20 +425,34 @@ void execute(const Token *expr,

else if (expr->isComparisonOp()) {
MathLib::bigint result1(0), result2(0);
execute(expr->astOperand1(), programMemory, &result1, error);
execute(expr->astOperand2(), programMemory, &result2, error);
if (expr->str() == "<")
*result = result1 < result2;
else if (expr->str() == "<=")
*result = result1 <= result2;
else if (expr->str() == ">")
*result = result1 > result2;
else if (expr->str() == ">=")
*result = result1 >= result2;
else if (expr->str() == "==")
*result = result1 == result2;
else if (expr->str() == "!=")
*result = result1 != result2;
bool error1 = 0;
bool error2 = 0;
execute(expr->astOperand1(), programMemory, &result1, &error1);
execute(expr->astOperand2(), programMemory, &result2, &error2);
if (error1 && error2) {
*error = true;
} else if (error1 && !error2) {
ValueFlow::Value v = inferCondition(expr->str(), expr->astOperand1(), result2);
*error = !v.isKnown();
*result = v.intvalue;
} else if (!error1 && error2) {
ValueFlow::Value v = inferCondition(expr->str(), result1, expr->astOperand2());
*error = !v.isKnown();
*result = v.intvalue;
} else {
if (expr->str() == "<")
*result = result1 < result2;
else if (expr->str() == "<=")
*result = result1 <= result2;
else if (expr->str() == ">")
*result = result1 > result2;
else if (expr->str() == ">=")
*result = result1 >= result2;
else if (expr->str() == "==")
*result = result1 == result2;
else if (expr->str() == "!=")
*result = result1 != result2;
}
}

else if (expr->isAssignmentOp()) {
Expand Down
125 changes: 117 additions & 8 deletions lib/token.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,13 @@
#include "symboldatabase.h"
#include "tokenlist.h"
#include "utils.h"
#include "valueflow.h"

#include <algorithm>
#include <cassert>
#include <cctype>
#include <cstring>
#include <functional>
#include <iostream>
#include <map>
#include <set>
Expand Down Expand Up @@ -1947,6 +1949,27 @@ const Token *Token::getValueTokenDeadPointer() const
return nullptr;
}

static bool isAdjacent(const ValueFlow::Value& x, const ValueFlow::Value& y)
{
if (x.bound != ValueFlow::Value::Bound::Point && x.bound == y.bound) {
return true;
} else {
Copy link
Owner

Choose a reason for hiding this comment

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

This else is redundant

if (x.valueType == ValueFlow::Value::ValueType::FLOAT)
return false;
return std::abs(x.intvalue - y.intvalue) == 1;
}
}

static bool removePointValue(std::list<ValueFlow::Value>& values, ValueFlow::Value& x)
{
const bool isPoint = x.bound == ValueFlow::Value::Bound::Point;
if (!isPoint)
x.decreaseRange();
else
values.remove(x);
return isPoint;
}

static bool removeContradiction(std::list<ValueFlow::Value>& values)
{
bool result = false;
Expand All @@ -1962,26 +1985,110 @@ static bool removeContradiction(std::list<ValueFlow::Value>& values)
continue;
if (x.isImpossible() == y.isImpossible())
continue;
if (!x.equalValue(y))
if (!x.equalValue(y)) {
auto compare = [](const ValueFlow::Value& x, const ValueFlow::Value& y) {
return x.compareValue(y, ValueFlow::less{});
};
const ValueFlow::Value& maxValue = std::max(x, y, compare);
const 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);
return true;
}
if (minValue.isImpossible() && minValue.bound == ValueFlow::Value::Bound::Lower) {
values.remove(maxValue);
return true;
}
continue;
if (x.bound == y.bound ||
(x.bound != ValueFlow::Value::Bound::Point && y.bound != ValueFlow::Value::Bound::Point)) {
const bool removex = !x.isImpossible() || y.isKnown();
const bool removey = !y.isImpossible() || x.isKnown();
}
const bool removex = !x.isImpossible() || y.isKnown();
const bool removey = !y.isImpossible() || x.isKnown();
if (x.bound == y.bound) {
if (removex)
values.remove(x);
if (removey)
values.remove(y);
return true;
} else if (x.bound == ValueFlow::Value::Bound::Point) {
y.decreaseRange();
result = true;
} else {
result = removex || removey;
bool bail = false;
if (removex && removePointValue(values, x))
bail = true;
if (removey && removePointValue(values, y))
bail = true;
if (bail)
return true;
}
}
}
return result;
}

using ValueIterator = std::list<ValueFlow::Value>::iterator;

template <class Iterator>
static ValueIterator removeAdjacentValues(std::list<ValueFlow::Value>& values, ValueIterator x, Iterator start, Iterator last)
{
if (!isAdjacent(*x, **start))
return std::next(x);
auto it = std::adjacent_find(start, last, [](ValueIterator x, ValueIterator y) { return !isAdjacent(*x, *y); });
if (it == last)
it--;
(*it)->bound = x->bound;
std::for_each(start, it, [&](ValueIterator y) { values.erase(y); });
return values.erase(x);
}

static void mergeAdjacent(std::list<ValueFlow::Value>& values)
{
for (auto x = values.begin(); x != values.end();) {
if (x->isNonValue()) {
x++;
continue;
}
if (x->bound == ValueFlow::Value::Bound::Point) {
x++;
continue;
}
std::vector<ValueIterator> adjValues;
for (auto y = values.begin(); y != values.end(); y++) {
if (x == y)
continue;
if (y->isNonValue())
continue;
if (x->valueType != y->valueType)
continue;
if (x->valueKind != y->valueKind)
continue;
if (x->bound != y->bound) {
// No adjacent points for floating points
if (x->valueType == ValueFlow::Value::ValueType::FLOAT)
continue;
if (y->bound != ValueFlow::Value::Bound::Point)
continue;
}
if (x->bound == ValueFlow::Value::Bound::Lower && !y->compareValue(*x, ValueFlow::less{}))
continue;
if (x->bound == ValueFlow::Value::Bound::Upper && !x->compareValue(*y, ValueFlow::less{}))
continue;
adjValues.push_back(y);
}
if (adjValues.empty()) {
x++;
continue;
}
std::sort(adjValues.begin(), adjValues.end(), [&values](ValueIterator xx, ValueIterator yy) {
assert(xx != values.end() && yy != values.end());
return xx->compareValue(*yy, ValueFlow::less{});
});
if (x->bound == ValueFlow::Value::Bound::Lower)
x = removeAdjacentValues(values, x, adjValues.rbegin(), adjValues.rend());
else if (x->bound == ValueFlow::Value::Bound::Upper)
x = removeAdjacentValues(values, x, adjValues.begin(), adjValues.end());
}
}

static void removeOverlaps(std::list<ValueFlow::Value>& values)
{
for (ValueFlow::Value& x : values) {
Expand All @@ -2005,12 +2112,14 @@ static void removeOverlaps(std::list<ValueFlow::Value>& values)
return true;
});
}
mergeAdjacent(values);
}

// Removing contradictions is an NP-hard problem. Instead we run multiple
// passes to try to catch most contradictions
static void removeContradictions(std::list<ValueFlow::Value>& values)
{
removeOverlaps(values);
for (int i = 0; i < 4; i++) {
if (!removeContradiction(values))
return;
Expand Down
Loading