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

C++: Rewrite cpp/unbounded-write away from DefaultTaintTracking #14669

Merged
merged 4 commits into from
Nov 10, 2023

Conversation

MathiasVP
Copy link
Contributor

@MathiasVP MathiasVP commented Nov 2, 2023

This PR rewrites the cpp/unbounded-write query away from DefaultTaintTracking. MRVA does report some result changes (both new results and disappearing results). The new results all look genuine, and were hidden by the bad default barriers in DefaultTaintTracking, and all of the disappearing results have been FPs being removed because we now realise that a variable is being overwritten along the path (which we didn't do before because DefaultTaintTracking conflated x and glval<x>).

CI is unhappy because of internal test changes. I've accepted these in the internal repo.

@github-actions github-actions bot added the C++ label Nov 2, 2023
@MathiasVP MathiasVP force-pushed the no-dtt-in-unbounded-write branch from 9908cf6 to 98d7ac8 Compare November 7, 2023 16:43
@MathiasVP MathiasVP force-pushed the no-dtt-in-unbounded-write branch from 98d7ac8 to e90803a Compare November 8, 2023 15:04
@MathiasVP MathiasVP marked this pull request as ready for review November 8, 2023 15:25
@MathiasVP MathiasVP requested a review from a team as a code owner November 8, 2023 15:25
@MathiasVP MathiasVP added no-change-note-required This PR does not need a change note depends on internal PR This PR should only be merged in sync with an internal Semmle PR labels Nov 8, 2023
@MathiasVP
Copy link
Contributor Author

As expected, DCA shows lots of changes (similar to what we saw on MRVA). Other than looking through most of the MRVA ones, I've verified that the kamailio__kamailio ones and the vim__vim ones are good changes. So I think the DCA run LGTM!

@jketema
Copy link
Contributor

jketema commented Nov 9, 2023

Can you summarise what you observed in MRVA?

@MathiasVP
Copy link
Contributor Author

MathiasVP commented Nov 10, 2023

Can you summarise what you observed in MRVA?

Sure.

  • The biggest difference is from simh/simh. On main we have 76 results, and after this rewrite there are 211 new results, and 35 lost results. All of the lost results here seem to be due to us now realizing that a variable is reassigned, and the new results seem genuine.
  • The second biggest difference is from joncampbell123/dosbox-x which has 306 results on main and loses 203 results after the rewrite. All of these appear to be DefaultTaintTracking FPs that are now removed.

Then there are various projects that lose or gain 1 result. I'm not super concerned about these since the many of these projects have >50 alerts on the query already.

After you noticed the problems with global variables (that are now fixed in #14736) I found a couple of instances of this as well, I think. I expect these to disappear once #14736 goes in. I don't think we need to wait for that PR to be merged before merging this, though

@MathiasVP MathiasVP merged commit 18c0bce into github:main Nov 10, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C++ depends on internal PR This PR should only be merged in sync with an internal Semmle PR no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants