Skip to content

Commit 61e8b84

Browse files
pfultz2pfultz2
authored
Fix 11610: false negative: knownConditionTrueFalse with address of variable (#4883)
1 parent a753923 commit 61e8b84

8 files changed

Lines changed: 73 additions & 45 deletions

File tree

‎lib/astutils.cpp‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1591,7 +1591,8 @@ bool isSameExpression(bool cpp, bool macro, const Token *tok1, const Token *tok2
15911591
return false;
15921592
}
15931593
} else {
1594-
if (tok1->function() && !tok1->function()->isConst() && !tok1->function()->isAttributeConst() && !tok1->function()->isAttributePure())
1594+
if (!tok1->function()->isConst() && !tok1->function()->isAttributeConst() &&
1595+
!tok1->function()->isAttributePure())
15951596
return false;
15961597
}
15971598
}

‎lib/checkstl.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1965,7 +1965,7 @@ void CheckStl::string_c_str()
19651965
string_c_strError(tok);
19661966
} else if (printPerformance && tok->tokAt(1)->astOperand2() && Token::Match(tok->tokAt(1)->astOperand2()->tokAt(-3), "%var% . c_str|data ( ) ;")) {
19671967
const Token* vartok = tok->tokAt(1)->astOperand2()->tokAt(-3);
1968-
if (tok->variable() && tok->variable()->isStlStringType() && vartok->variable() && vartok->variable()->isStlStringType())
1968+
if (tok->variable()->isStlStringType() && vartok->variable() && vartok->variable()->isStlStringType())
19691969
string_c_strAssignment(tok);
19701970
}
19711971
} else if (printPerformance && tok->function() && Token::Match(tok, "%name% ( !!)") && tok->str() != scope.className) {

‎lib/checkuninitvar.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1612,7 +1612,7 @@ void CheckUninitVar::valueFlowUninit()
16121612
bool uninitderef = false;
16131613
if (tok->variable()) {
16141614
bool unknown;
1615-
const bool isarray = !tok->variable() || tok->variable()->isArray();
1615+
const bool isarray = tok->variable()->isArray();
16161616
const bool ispointer = astIsPointer(tok) && !isarray;
16171617
const bool deref = CheckNullPointer::isPointerDeRef(tok, unknown, mSettings);
16181618
if (ispointer && v->indirect == 1 && !deref)

‎lib/infer.cpp‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -269,6 +269,12 @@ static void addToErrorPath(ValueFlow::Value& value, const std::vector<const Valu
269269
[&](const ErrorPathItem& e) {
270270
return locations.insert(e.first).second;
271271
});
272+
std::copy_if(ref->debugPath.cbegin(),
273+
ref->debugPath.cend(),
274+
std::back_inserter(value.debugPath),
275+
[&](const ErrorPathItem& e) {
276+
return locations.insert(e.first).second;
277+
});
272278
}
273279
}
274280

‎lib/symboldatabase.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6138,7 +6138,7 @@ void SymbolDatabase::setValueType(Token* tok, const ValueType& valuetype, Source
61386138
if (!parent->astOperand1())
61396139
return;
61406140

6141-
const ValueType *vt1 = parent->astOperand1() ? parent->astOperand1()->valueType() : nullptr;
6141+
const ValueType *vt1 = parent->astOperand1()->valueType();
61426142
const ValueType *vt2 = parent->astOperand2() ? parent->astOperand2()->valueType() : nullptr;
61436143

61446144
if (vt1 && Token::Match(parent, "<<|>>")) {

‎lib/valueflow.cpp‎

Lines changed: 49 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -508,6 +508,24 @@ static bool isComputableValue(const Token* parent, const ValueFlow::Value& value
508508
return true;
509509
}
510510

511+
static Library::Container::Yield getContainerYield(Token* tok, const Settings* settings, Token** parent = nullptr)
512+
{
513+
if (Token::Match(tok, ". %name% (") && tok->astParent() == tok->tokAt(2) && tok->astOperand1() &&
514+
tok->astOperand1()->valueType()) {
515+
const Library::Container* c = getLibraryContainer(tok->astOperand1());
516+
if (parent)
517+
*parent = tok->astParent();
518+
return c ? c->getYield(tok->strAt(1)) : Library::Container::Yield::NO_YIELD;
519+
} else if (Token::Match(tok->previous(), "%name% (")) {
520+
if (parent)
521+
*parent = tok;
522+
if (const Library::Function* f = settings->library.getFunction(tok->previous())) {
523+
return f->containerYield;
524+
}
525+
}
526+
return Library::Container::Yield::NO_YIELD;
527+
}
528+
511529
/** Set token value for cast */
512530
static void setTokenValueCast(Token *parent, const ValueType &valueType, const ValueFlow::Value &value, const Settings *settings);
513531

@@ -663,36 +681,25 @@ static void setTokenValue(Token* tok,
663681
}
664682
}
665683
}
666-
else if (Token::Match(parent, ". %name% (") && parent->astParent() == parent->tokAt(2) &&
667-
parent->astOperand1() && parent->astOperand1()->valueType()) {
668-
const Library::Container* c = getLibraryContainer(parent->astOperand1());
669-
const Library::Container::Yield yields = c ? c->getYield(parent->strAt(1)) : Library::Container::Yield::NO_YIELD;
670-
if (yields == Library::Container::Yield::SIZE) {
671-
ValueFlow::Value v(value);
672-
v.valueType = ValueFlow::Value::ValueType::INT;
673-
setTokenValue(parent->astParent(), std::move(v), settings);
674-
} else if (yields == Library::Container::Yield::EMPTY) {
675-
ValueFlow::Value v(value);
676-
v.valueType = ValueFlow::Value::ValueType::INT;
677-
if (value.isImpossible() && value.intvalue == 0)
684+
Token* next = nullptr;
685+
const Library::Container::Yield yields = getContainerYield(parent, settings, &next);
686+
if (yields == Library::Container::Yield::SIZE) {
687+
ValueFlow::Value v(value);
688+
v.valueType = ValueFlow::Value::ValueType::INT;
689+
setTokenValue(next, std::move(v), settings);
690+
} else if (yields == Library::Container::Yield::EMPTY) {
691+
ValueFlow::Value v(value);
692+
v.valueType = ValueFlow::Value::ValueType::INT;
693+
v.bound = ValueFlow::Value::Bound::Point;
694+
if (value.isImpossible()) {
695+
if (value.intvalue == 0)
678696
v.setKnown();
679697
else
680-
v.intvalue = !v.intvalue;
681-
setTokenValue(parent->astParent(), std::move(v), settings);
682-
}
683-
} else if (Token::Match(parent->previous(), "%name% (")) {
684-
if (const Library::Function* f = settings->library.getFunction(parent->previous())) {
685-
if (f->containerYield == Library::Container::Yield::SIZE) {
686-
ValueFlow::Value v(value);
687-
v.valueType = ValueFlow::Value::ValueType::INT;
688-
setTokenValue(parent, std::move(v), settings);
689-
} else if (f->containerYield == Library::Container::Yield::EMPTY) {
690-
ValueFlow::Value v(value);
691-
v.intvalue = !v.intvalue;
692-
v.valueType = ValueFlow::Value::ValueType::INT;
693-
setTokenValue(parent, std::move(v), settings);
694-
}
698+
v.setPossible();
699+
} else {
700+
v.intvalue = !v.intvalue;
695701
}
702+
setTokenValue(next, std::move(v), settings);
696703
}
697704
return;
698705
}
@@ -917,7 +924,10 @@ static void setTokenValue(Token* tok,
917924
if (val.isImpossible() && val.intvalue != 0)
918925
continue;
919926
ValueFlow::Value v(val);
920-
v.intvalue = !v.intvalue;
927+
if (val.isImpossible())
928+
v.setKnown();
929+
else
930+
v.intvalue = !v.intvalue;
921931
setTokenValue(parent, std::move(v), settings);
922932
}
923933
}
@@ -1859,7 +1869,7 @@ static void valueFlowImpossibleValues(TokenList* tokenList, const Settings* sett
18591869
ValueFlow::Value value{0};
18601870
value.setImpossible();
18611871
setTokenValue(tok->linkAt(1)->next(), std::move(value), settings);
1862-
} else if (tokenList->isCPP() && Token::simpleMatch(tok, "this")) {
1872+
} else if ((tokenList->isCPP() && Token::simpleMatch(tok, "this")) || tok->isUnaryOp("&")) {
18631873
ValueFlow::Value value{0};
18641874
value.setImpossible();
18651875
setTokenValue(tok, std::move(value), settings);
@@ -6666,16 +6676,7 @@ static void valueFlowInferCondition(TokenList* tokenlist,
66666676
continue;
66676677
if (tok->hasKnownIntValue())
66686678
continue;
6669-
if (tok->variable() && (Token::Match(tok->astParent(), "?|&&|!|%oror%") ||
6670-
Token::Match(tok->astParent()->previous(), "if|while ("))) {
6671-
std::vector<ValueFlow::Value> result = infer(IntegralInferModel{}, "!=", tok->values(), 0);
6672-
if (result.size() != 1)
6673-
continue;
6674-
ValueFlow::Value value = result.front();
6675-
value.intvalue = 1;
6676-
value.bound = ValueFlow::Value::Bound::Point;
6677-
setTokenValue(tok, std::move(value), settings);
6678-
} else if (Token::Match(tok, "%comp%|-") && tok->astOperand1() && tok->astOperand2()) {
6679+
if (Token::Match(tok, "%comp%|-") && tok->astOperand1() && tok->astOperand2()) {
66796680
if (astIsIterator(tok->astOperand1()) || astIsIterator(tok->astOperand2())) {
66806681
static const std::array<ValuePtr<InferModel>, 2> iteratorModels = {EndIteratorInferModel{},
66816682
StartIteratorInferModel{}};
@@ -6694,6 +6695,15 @@ static void valueFlowInferCondition(TokenList* tokenlist,
66946695
setTokenValue(tok, std::move(value), settings);
66956696
}
66966697
}
6698+
} else if (Token::Match(tok->astParent(), "?|&&|!|%oror%") ||
6699+
Token::Match(tok->astParent()->previous(), "if|while (")) {
6700+
std::vector<ValueFlow::Value> result = infer(IntegralInferModel{}, "!=", tok->values(), 0);
6701+
if (result.size() != 1)
6702+
continue;
6703+
ValueFlow::Value value = result.front();
6704+
value.intvalue = 1;
6705+
value.bound = ValueFlow::Value::Bound::Point;
6706+
setTokenValue(tok, std::move(value), settings);
66976707
}
66986708
}
66996709
}

‎test/testcondition.cpp‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -353,11 +353,14 @@ class TestCondition : public TestFixture {
353353
ASSERT_EQUALS("", errout.str());
354354

355355
// no crash on unary operator& (#5643)
356+
// #11610
356357
check("SdrObject* ApplyGraphicToObject() {\n"
357358
" if (&rHitObject) {}\n"
358359
" else if (rHitObject.IsClosedObj() && !&rHitObject) { }\n"
359360
"}");
360-
ASSERT_EQUALS("", errout.str());
361+
ASSERT_EQUALS("[test.cpp:2]: (style) Condition '&rHitObject' is always true\n"
362+
"[test.cpp:3]: (style) Condition '!&rHitObject' is always false\n",
363+
errout.str());
361364

362365
// #5695: increment
363366
check("void f(int a0, int n) {\n"
@@ -3800,6 +3803,7 @@ class TestCondition : public TestFixture {
38003803
"}\n");
38013804
ASSERT_EQUALS("", errout.str());
38023805

3806+
// TODO: if (!v) is a known condition as well
38033807
check("struct a {\n"
38043808
" int *b();\n"
38053809
"};\n"
@@ -3813,7 +3817,7 @@ class TestCondition : public TestFixture {
38133817
" if (v == nullptr && e) {}\n"
38143818
" return d;\n"
38153819
"}\n");
3816-
ASSERT_EQUALS("", errout.str());
3820+
ASSERT_EQUALS("[test.cpp:11]: (style) Condition 'e' is always true\n", errout.str());
38173821

38183822
// #10037
38193823
check("struct a {\n"

‎test/testvalueflow.cpp‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1072,6 +1072,13 @@ class TestValueFlow : public TestFixture {
10721072
"}";
10731073
ASSERT_EQUALS(false, testValueOfX(code, 3U, 0));
10741074

1075+
code = "void f(int i) {\n"
1076+
" int * p = &i;\n"
1077+
" bool x = !p || i;\n"
1078+
" bool a = x;\n"
1079+
"}\n";
1080+
ASSERT_EQUALS(false, testValueOfX(code, 4U, 1));
1081+
10751082
code = "bool f(const uint16_t * const p) {\n"
10761083
" const uint8_t x = (uint8_t)(*p & 0x01E0U) >> 5U;\n"
10771084
" return x != 0;\n"

0 commit comments

Comments
 (0)