Skip to content

Conversation

@paldepind
Copy link
Contributor

@paldepind paldepind commented Nov 28, 2024

Exclude some data flow inconsistency errors where the root cause is not in the data flow library.

Adds a new AST consistency metric that tracks AST nodes without a location.

I'm not sure if the change in this PR alone is enough for the new metric to show up on DCA. @geoffw0 most likely knows if additional setup is needed.

@github-actions github-actions bot added the Rust Pull requests that update Rust code label Nov 28, 2024
@paldepind paldepind force-pushed the rust-df-inconsistency-no-location branch from 87e663c to a2f6ca1 Compare November 28, 2024 10:52
@paldepind paldepind force-pushed the rust-df-inconsistency-no-location branch from a2f6ca1 to b05d290 Compare November 28, 2024 11:46
@paldepind paldepind marked this pull request as ready for review November 28, 2024 11:56
@geoffw0
Copy link
Contributor

geoffw0 commented Nov 29, 2024

I'm not sure if the change in this PR alone is enough for the new metric to show up on DCA

DCA pulls data from the rust/summary/summary-statistics query. I believe you need to update the getTotalDataFlowInconsistencies predicate in src\queries\summary\Stats.qll which it uses - changes similar to what you did in DataFlowConsistencyCounts.ql should do it.

Otherwise LGTM.

private module Input implements InputSig<Location, RustDataFlow> { }

import MakeConsistency<Location, RustDataFlow, RustTaintTracking, Input>
import codeql.rust.dataflow.internal.DataFlowConsistency
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding this file has got rid of some needless duplication. 👍

@paldepind
Copy link
Contributor Author

Thanks for the guidance @geoffw0. I missed the Stats.qll. I think that's why the DCA run didn't show any change in data flow inconsistencies. I've updated that as well and started a new run.

@paldepind
Copy link
Contributor Author

DCA results are now looking as expected: Almost all data flow inconsistencies are gone and AST inconsistencies are up.

Copy link
Contributor

@geoffw0 geoffw0 left a comment

Choose a reason for hiding this comment

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

Notably we now have more new AST inconsistencies than we've lost data flow inconsistencies, but I think that's expected and reflects where the problem really is. And we have over 5 million AST inconsistencies on windows-rs, which is spectacular, but nothing appears to have broken as a result.

It would [obviously] be good to prioritize fixing a majority of those AST inconsistencies fairly soon.

👍

@paldepind paldepind merged commit 10be890 into github:main Dec 2, 2024
15 checks passed
@paldepind
Copy link
Contributor Author

Just to note, after this PR I think we want to keep an eye on the data flow inconsistencies in DCA and make sure that it doesn't go up. The DCA projects (not including Rust) all sits at 0 or 1 inconsistencies, so increases should be noticeable.

@paldepind paldepind deleted the rust-df-inconsistency-no-location branch December 2, 2024 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Rust Pull requests that update Rust code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants