Skip to content

C++: Add PostUpdateNode for updates to structs with no chi instructions #3295

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

Merged
merged 5 commits into from
Apr 23, 2020

Conversation

MathiasVP
Copy link
Contributor

@MathiasVP MathiasVP commented Apr 19, 2020

In #3118 we added basic IR field flow by attaching PostUpdateNodes to chi instructions. However, updating a field in a struct doesn't always generate a chi instruction. In particular, writing to a struct with a single field doesn't generate a chi instruction. This means we don't catch the following dataflow:

struct A {
  int i;
};
void foo() {
  A a;
  a.i = source();
  A a2 = a;
  sink(a2.i);
}

But we catch it if A is replaced with a struct with more than 1 field like:

struct A {
  int i, j;
};

One problem with adding a PostUpdateNode to the assignment a.i = source() is that the dataflow library requires a predicate getPreUpdateNode that returns the node before the update to its state. For field updates generating a chi instruction we can pick the total operand of the chi (as this represents the memory allocated to the struct), but without a chi instruction it's definately to define a meaningful result of this predicate. So this PR simply implements this predicate as none().

I don't know if this breaks something in the dataflow library, but it does catch the above flow even when A contains a single field.

This PR also adds an additional rule to simpleInstructionLocalFlowStep in order to propagate dataflow to loads with inexact memory operands that arise from stores to single field structs. This rule could be avoided if the overlap was considered MustExactlyOverlap.

@MathiasVP MathiasVP added C++ WIP This is a work-in-progress, do not merge yet! labels Apr 19, 2020
@MathiasVP MathiasVP force-pushed the field-flow-single-struct branch from ae1a30e to a6e619c Compare April 20, 2020 06:52
@MathiasVP
Copy link
Contributor Author

Here's the relevant CPP-differences: https://jenkins.internal.semmle.com/job/Changes/job/CPP-Differences/1051/

I'll investigate what's up with boost, drogon and json. There are no result changes, which I guess is unsurprising (I wouldn't expect many real-world cases of structs with a single field).

@MathiasVP MathiasVP added WIP This is a work-in-progress, do not merge yet! and removed WIP This is a work-in-progress, do not merge yet! labels Apr 21, 2020
@MathiasVP
Copy link
Contributor Author

@MathiasVP MathiasVP removed the WIP This is a work-in-progress, do not merge yet! label Apr 22, 2020
@MathiasVP MathiasVP marked this pull request as ready for review April 22, 2020 06:52
@MathiasVP MathiasVP requested a review from a team as a code owner April 22, 2020 06:52
Copy link
Contributor

@jbj jbj left a comment

Choose a reason for hiding this comment

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

This seems like an improvement overall, so I'll merge.

@jbj jbj merged commit f696594 into github:master Apr 23, 2020
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