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

Truncate value of increment operator #6342

Merged
merged 3 commits into from
Jun 26, 2024

Conversation

francois-berder
Copy link
Contributor

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

lib/valueflow.cpp Outdated Show resolved Hide resolved
@chrchr-github
Copy link
Collaborator

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

@firewave
Copy link
Collaborator

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.

lib/valueflow.cpp Outdated Show resolved Hide resolved
@francois-berder
Copy link
Contributor Author

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

@francois-berder
Copy link
Contributor Author

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
Copy link
Collaborator

firewave commented May 1, 2024

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 francois-berder force-pushed the fix-inc-valueflow branch 2 times, most recently from 66f3e86 to 90803f5 Compare May 3, 2024 14:46
@francois-berder
Copy link
Contributor Author

@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
Copy link
Collaborator

firewave commented May 7, 2024

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
Copy link
Contributor Author

francois-berder commented May 10, 2024

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
Copy link
Owner

danmar commented May 21, 2024

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
Copy link
Owner

danmar commented May 21, 2024

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

@pfultz2
Copy link
Contributor

pfultz2 commented May 21, 2024

@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
Copy link
Owner

danmar commented May 22, 2024

@pfultz2 Thank you!

@francois-berder francois-berder force-pushed the fix-inc-valueflow branch 2 times, most recently from 29c11de to 1581a7f Compare June 4, 2024 12:54
@chrchr-github
Copy link
Collaborator

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

@francois-berder
Copy link
Contributor Author

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
Copy link
Contributor Author

@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
Copy link
Collaborator

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.

lib/vf_common.cpp Outdated Show resolved Hide resolved
@francois-berder
Copy link
Contributor Author

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

@chrchr-github
Copy link
Collaborator

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

Copy link
Owner

@danmar danmar left a comment

Choose a reason for hiding this comment

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

lgtm

@chrchr-github chrchr-github merged commit c85f889 into danmar:main Jun 26, 2024
63 checks passed
@francois-berder francois-berder deleted the fix-inc-valueflow branch June 27, 2024 08:48
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.

5 participants