Skip to content

Fix #15184 FN zerodiv with compound assignment - #9006

Open
chrchr-github wants to merge 3 commits into
cppcheck-opensource:mainfrom
chrchr-github:chr_15184
Open

chrchr-github wants to merge 3 commits into
cppcheck-opensource:mainfrom
chrchr-github:chr_15184

Conversation

@chrchr-github

Copy link
Copy Markdown
Collaborator

No description provided.

@dmcppcheck dmcppcheck left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

AI review

The value of a compound assignment is not truncated to the type of its left operand, so the PR creates new false positives. The new branch computes c += 1 with the arithmetic for c + 1, and the result keeps that type instead of the type of c. With unsigned char c = 255;, the PR gives (c += 1) the known value 256 where the real value is 0. With unsigned int u = 0;, it gives (u -= 1) the known value -1. This causes a new false knownConditionTrueFalse and a new false arrayIndexOutOfBounds, and in such cases a real division by zero is still missed. The result needs a cast to the type of astOperand1(), for example through setTokenValueCast or castValue. For plain int operands, the cases that the PR is about now work for all the compound operators. The unit test covers only += and does not cover narrow or unsigned left operands.

Testing

I built cppcheck and testrunner from pr/, and cppcheck from base/.

Tests: ./testrunner TestValueFlow TestCondition TestOther TestUninitVar TestNullPointer TestBufferOverrun ran 829 tests and none failed.

Intended behaviour: 1 / (i OP= k) with known int operands now gives zerodiv for += -= *= &= >>= %= /=. Base reports nothing for these.

False positive 1, knownConditionTrueFalse (t5.c):

int f(void) {
    unsigned char c = 255;
    int x = (c += 1);
    if (x == 0)
        return 1;
    return 0;
}

Command: cppcheck -q --enable=style t5.c. Base gives only unreadVariable. The PR adds:

t5.c:4:11: style: Condition 'x==0' is always false [knownConditionTrueFalse]
t5.c:3:16: note: Assignment 'x=c+=1', assigned value is 256

In fact x is 0, so the condition is always true.

False positive 2, arrayIndexOutOfBounds (t3.c):

void f3(int a[10]) {
    unsigned char c = 255;
    a[c += 1] = 0;
}

Command: cppcheck -q --enable=all t3.c. The PR reports the following, and base does not:

t3.c:11:6: error: Array 'a[10]' accessed at index 256, which is out of bounds. [arrayIndexOutOfBounds]

The real index is 0.

Wrong values in --debug: the PR shows += always 256 for unsigned char c = 255; (c += 1) and -= always -1 for unsigned int u = 0; (u -= 1). Base has only !<=-1 there. Because of this wrong value, unsigned char c = 255; return 10 / (c += 1); still gives no zerodiv.


Automated review of 98b5640 by Claude. The pull request was built and tested locally.

Comment thread lib/vf_settokenvalue.cpp

// Calculations..
else if ((parent->isArithmeticalOp() || parent->isComparisonOp() || (parent->tokType() == Token::eBitOp) ||
else if ((parent->isArithmeticalOp() || parent->isComparisonOp() || (parent->tokType() == Token::eBitOp) || (Token::Match(parent, "%assign%") && parent->str().size() > 1) ||

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

AI review

For compound assignments, the computed result has to be converted to the type of the left operand. Today the value of c += 1 is computed like the value of c + 1, which is promoted to int, so it is never truncated.

int f(void) {
    unsigned char c = 255;
    int x = (c += 1);   // PR: `+=` always 256 (should be 0)
    if (x == 0)         // PR: new FP knownConditionTrueFalse "always false"
        return 1;
    return 0;
}
void g(int a[10]) { unsigned char c = 255; a[c += 1] = 0; } // PR: new FP arrayIndexOutOfBounds index 256

unsigned int u = 0; (u -= 1) likewise gets the value -1. Casting the result with castValue or setTokenValueCast, using the valueType() of parent->astOperand1(), would fix this. A test with an unsigned char or unsigned left operand would cover it.

@chrchr-github

Copy link
Copy Markdown
Collaborator Author

It looks like the problem already exists with regular assignments: https://trac.cppcheck.net/ticket/15186

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants