Truncate value of increment operator

[francois-berder]

This PR fixes a similar bug than #11591 fixed in PR #6331.

[chrchr-github]

There should probably be a check for value_size < 8 in truncateIntValue(), given that bigint is only 64 bits.

[firewave]

There should probably be a check for value_size < 8 in truncateIntValue(), given that bigint is only 64 bits.

That check should be dependent on the actual size of the type instead of being hard-coded.
I have some local changes which replace change the bigint alias to a 128-byte type. This might not end up in production code but actually exposes a lot of places where we need to be more type safe.

[danmar]

this will not work well.
if int according to the platform configuration is 16-bits then the max value should be 0xffff
but max value will be 0xffffffff if I compile cppcheck on a normal platform where int is 32 bits.

[francois-berder]

CI is now failing due to errors not related to this PR.

[francois-berder]

There should probably be a check for value_size < 8 in truncateIntValue(), given that bigint is only 64 bits.

That check should be dependent on the actual size of the type instead of being hard-coded. I have some local changes which replace change the bigint alias to a 128-byte type. This might not end up in production code but actually exposes a lot of places where we need to be more type safe.

I think my last commits fix this. However, I was quite surprise to find that the function was called with value_size set to 0.

[firewave]

CI is now failing due to errors not related to this PR.

Yes, there was a bad merge. It rarely happens. Please rebase.

[francois-berder]

@firewave

I do not really understand how the CI error in undefined behaviour sanitizer build has anything to do with my changes.

Run selfcheck_options="-q -j$(nproc) --std=c++11 --template=selfcheck --showtime=top5_summary -D__GNUC__ --error-exitcode=1 --inline-suppr --suppressions-list=.selfcheck_suppressions --library=gnu --inconclusive --enable=style,performance,portability,warning,missingInclude,internal --exception-handling --debug-warnings --check-level=exhaustive"

Overall time: 0s
lib/token.cpp:992:25: style: Condition '--depth==0' is always false [knownConditionTrueFalse]
            if (--depth == 0)
                        ^
lib/token.cpp:1034:25: style: Condition '--depth==0' is always false [knownConditionTrueFalse]
            if (--depth == 0)

I had a quick look at the code and it looks like a false positive.

[firewave]

I do not really understand how the CI error in undefined behaviour sanitizer build has anything to do with my changes.

That is the selfcheck/bootstrap/dogfood. We run our code on our own codebase on each commit.

As you changed something related to increment/decrement in the ValueFlow you changed the behavior which introduced this false positive.

If you add --debug you get information on the generated ValueFlow and see what changed. --debug-warning might also give some additional information.

That's the furthest I can help you as I have no idea how the actual logic in the code works. So somebody else needs to chime in if you cannot find the issue yourself.

You previous change also introduced a false positive albeit not with our code.

[francois-berder]

That is the selfcheck/bootstrap/dogfood. We run our code on our own codebase on each commit.

OK. I have not had the time to fix this issue. However, I found that this diff hides this false positive:

diff --git a/lib/token.cpp b/lib/token.cpp
index 5e5f7226e..c959ccedd 100644
--- a/lib/token.cpp
+++ b/lib/token.cpp
@@ -989,7 +989,8 @@ const Token * Token::findClosingBracket() const
                  (templateParameter ? templateParameters.find(closing->strAt(-1)) == templateParameters.end() : true))
             ++depth;
         else if (closing->str() == ">") {
-            if (--depth == 0)
+            --depth;
+            if (depth == 0)
                 return closing;
         } else if (closing->str() == ">>" || closing->str() == ">>=") {
             if (!isDecl && depth == 1)
@@ -1031,7 +1032,8 @@ const Token * Token::findOpeningBracket() const
         else if (opening->str() == ">")
             ++depth;
         else if (opening->str() == "<") {
-            if (--depth == 0)
+            --depth;
+            if (depth == 0)
                 return opening;
         }
     }

[danmar]

OK. I have not had the time to fix this issue. However, I found that this diff hides this false positive:

hmm sorry it does not feel very comfortable. So users can get false positives after these changes.

Maybe your changes exposed something else in valueflow that does not work very well.. unfortunately I don't feel I can help very much right now neither.

[danmar]

please feel free to ping us if you don't a response after couple of days.

[pfultz2]

@firewave

I do not really understand how the CI error in undefined behaviour sanitizer build has anything to do with my changes.

Run selfcheck_options="-q -j$(nproc) --std=c++11 --template=selfcheck --showtime=top5_summary -D__GNUC__ --error-exitcode=1 --inline-suppr --suppressions-list=.selfcheck_suppressions --library=gnu --inconclusive --enable=style,performance,portability,warning,missingInclude,internal --exception-handling --debug-warnings --check-level=exhaustive"

Overall time: 0s
lib/token.cpp:992:25: style: Condition '--depth==0' is always false [knownConditionTrueFalse]
            if (--depth == 0)
                        ^
lib/token.cpp:1034:25: style: Condition '--depth==0' is always false [knownConditionTrueFalse]
            if (--depth == 0)

I had a quick look at the code and it looks like a false positive.

This is very likely truncating an impossible value which can be out of the range of the integer. So for unsigned integers we set the value to !<=-1 and then if we decrement it and truncate it would become !<=0 which is why its saying its always false.

Probably should add some valueflow tests for unsigned integers.

[danmar]

@pfultz2 Thank you!

[chrchr-github]

Does this fix https://trac.cppcheck.net/ticket/12742?

[francois-berder]

Does this fix https://trac.cppcheck.net/ticket/12742?

I could not reproduce the bug.

$ ./cmake.output/bin/cppcheck --enable=warning --debug tickets/12742.c 
Checking tickets/12742.c ...


##file tickets/12742.c
1: void f ( const std :: vector < int > & v ) {
2: for ( unsigned int i@var1 = 0 ; i@var1 <@exprUNIQUE v .@exprUNIQUE size (@exprUNIQUE ) ;@exprUNIQUE ) {
3: (@exprUNIQUE void ) v@exprUNIQUE [@exprUNIQUE ++@exprUNIQUE i@var1 ] ;
4: }
5: }



##Value flow
File tickets/12742.c
Line 1
  < always {!<=-1,!>=2}
  > always {!<=-1,!>=2}
  & always !0
Line 2
  i always !<=-1
  = always 0
  0 always 0
  i {!<=-1,0}
  < always {!<=-1,!>=2}
Line 3
  ++ always !<=0
  i always !<=-1

[francois-berder]

@danmar @chrchr-github @pfultz2 @firewave

I believe this PR is now ready for review.

When I have to invert the value bound, I increment (or decrement) the value by 2 to ensure that the bound stays correct.

[chrchr-github]

Does this fix https://trac.cppcheck.net/ticket/12742?

I could not reproduce the bug.

It looks like you are checking in C mode. You can change the file extension or use --language=cpp.

[danmar]

I don't understand this calculation.

The unsigned max value when value_size is 1 should be 255, shouldn't it?