Skip to content

C++: allow read steps at the sink in IR taint test#12079

Merged
MathiasVP merged 2 commits intogithub:mathiasvp/replace-ast-with-ir-use-usedataflowfrom
rdmarsh2:rdmarsh2/use-use-taint-test-reads
Feb 8, 2023
Merged

C++: allow read steps at the sink in IR taint test#12079
MathiasVP merged 2 commits intogithub:mathiasvp/replace-ast-with-ir-use-usedataflowfrom
rdmarsh2:rdmarsh2/use-use-taint-test-reads

Conversation

@rdmarsh2
Copy link
Copy Markdown
Contributor

@rdmarsh2 rdmarsh2 commented Feb 2, 2023

This resolves a few missing taint sinks, but I'm not sure if they were testing special handling for single-field structs before. @MathiasVP IIRC you did something special for single-field structs when adding field flow into the def-use IR dataflow, should we reimplement that instead of adding this case?

@github-actions github-actions Bot added the C++ label Feb 2, 2023
@rdmarsh2 rdmarsh2 force-pushed the rdmarsh2/use-use-taint-test-reads branch from f7c7c2d to ad8e82a Compare February 3, 2023 16:39
Comment thread cpp/ql/test/library-tests/dataflow/taint-tests/taint.ql Outdated
@MathiasVP
Copy link
Copy Markdown
Contributor

@MathiasVP IIRC you did something special for single-field structs when adding field flow into the def-use IR dataflow, should we reimplement that instead of adding this case?

The single-field struct thing was really only there to work-around the fact that writes to a single-field struct didn't generate ChiInstructions. Now that we don't depend on ChiInstructions for PostUpdateNodes we don't need to special-case single-field structs anymore.

So I like the solution in this PR a lot more!

@MathiasVP MathiasVP marked this pull request as ready for review February 8, 2023 13:09
@MathiasVP MathiasVP requested a review from a team as a code owner February 8, 2023 13:09
@MathiasVP
Copy link
Copy Markdown
Contributor

In the spirit of getting this PR merged soon I've pushed a commit with my suggestion @rdmarsh2. I hope that's alright.

Copy link
Copy Markdown
Contributor

@MathiasVP MathiasVP left a comment

Choose a reason for hiding this comment

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

LGTM!

@MathiasVP MathiasVP merged commit 946e301 into github:mathiasvp/replace-ast-with-ir-use-usedataflow Feb 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants