cppcheck Truncate value of increment operator

This PR fixes a similar bug than #11591 fixed in PR https://github.com/danmar/cppcheck/pull/6331.

Apr 25 '24 13:04 francois-berder

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

Apr 26 '24 09:04 chrchr-github

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.

Apr 26 '24 12:04 firewave

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

May 01 '24 14:05 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.

May 01 '24 14:05 francois-berder

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

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

May 01 '24 18:05 firewave

@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.

May 07 '24 07:05 francois-berder

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.

May 07 '24 09:05 firewave

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;
         }
     }

May 10 '24 16:05 francois-berder

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.

May 21 '24 18:05 danmar

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

May 21 '24 18:05 danmar

@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.

May 21 '24 21:05 pfultz2

@pfultz2 Thank you!

May 22 '24 07:05 danmar

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

Jun 05 '24 20:06 chrchr-github

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

Jun 07 '24 16:06 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.

Jun 07 '24 16:06 francois-berder

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.

Jun 07 '24 16:06 chrchr-github

@chrchr-github Looking at this bug it seems that my previous bugfix triggered an existing bug. So I would be happy to fix this issue in another PR. What do you think?

Jun 14 '24 15:06 francois-berder

@chrchr-github Looking at this bug it seems that my previous bugfix triggered an existing bug. So I would be happy to fix this issue in another PR. What do you think?

OK, I was just wondering if we could add a test in this PR.

Jun 14 '24 18:06 chrchr-github