-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Go: Use value flow instead of taint flow for go/incorrect-integer-conversion
#16305
Conversation
s/DCA/QA? |
Probably the best way of reviewing this is spot-checking some of the alert changes in QA. (I've checked the DCA ones and they're fine.) |
Should we add back steps that are clearly taint-preserving for this query, like some arithmetic? |
@smowton It's a bit complicated, as technically any arithmetic might change how many bits are required to represent the number in question. So it opens us up to false positives. Have you seen any examples of alerts which are lost which would be found if we added arithmetic taint steps? (Also, in the longer term we should probably use the shared range analysis, which will do this for us.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. I'm afraid I haven't the time to trawl results from the QA run, so if you've done so and are satisfied with the results I'll just take your word for it and approve.
(add a change note though) |
On closer examination of the QA results, we are currently losing results because value flow through |
93a04d2
to
c5b2d9e
Compare
c5b2d9e
to
410543f
Compare
@smowton The results from this are good now that I've merged several other PRs fixing bugs in value flow. I compared them using MRVA on the repos that had any alert changes in the last QA run. I spotted another small bug in the query but it's unrelated so I'll do it in a separate PR. |
This should remove some FPs and also improve performance, especially on projects with lots of FPs. I have done a DCA run and the results look good. I will do a
DCAQA run.