C++: Handle call-contexts mismatches in cpp/invalid-pointer-deref

[MathiasVP]

The cpp/invalid-pointer-deref query finds problematic dereferences in two steps:

1. We first identify flow form an allocation to the construction of a pointer that's out-of-bounds, and then
2. we find flow from the out-of-bounds pointer to a dereference.

This is split up into two configurations, and whenever we do such a split in a query we risk FPs caused by losing all call-context information when we transition between the two configurations when faced with code that roughly looks like this:

int id(int x) {
  return x; // assume the transition between the two configurations happens here.
}

void f1() {
  int x = source();
  int y = id(x);
}

void f2() {
  int x = 0;
  int y = id(x);
  sink(x);
}

Normally we'd fix this problem by using a flow state to collapse the two configurations into one, but this isn't very easy because step 1 in this query is done using the product-flow library (and not just a normal dataflow configurations).

So this PR fixes the above problem by constructing a final configuration that represents the problematic flow path.

[MathiasVP]

@jketema I think this PR is good to go. I need to verify that the lost results are all from fixing call contexts, but I'm pretty confident that this is the case. Looking forward to trying this out on MRVA 🤞.

[jketema]

Currently, this looks incorrect to me. See comments below.

[jketema]

Is this actually doing anything since we're using invalidPointerToDerefSource as a source anyway?

[MathiasVP]

We're starting the flow at the allocation just like before. Note that the first column of invalidPointerToDerefSource (which is the one we use to mark the source in FinalConfig):

predicate isSource(DataFlow::Node source, FlowState state) {
  state = TInitial() and
  exists(DataFlow::Node derefSource |
    invalidPointerToDerefSource(source, _, derefSource, _) and
    InvalidPointerToDerefFlow::flow(derefSource, _)
  )
}

is the dataflow node that represents the allocation:

predicate invalidPointerToDerefSource(
  DataFlow::Node source1, PointerArithmeticInstruction pai, DataFlow::Node derefSource, int delta
) {
  exists(
    AllocToInvalidPointerFlow::PathNode1 pSource1, AllocToInvalidPointerFlow::PathNode1 pSink1,
    AllocToInvalidPointerFlow::PathNode2 pSink2, DataFlow::Node sink1, DataFlow::Node sink2,
    int delta0
  |
    pragma[only_bind_out](pSource1.getNode()) = source1 and
    // ...
    AllocToInvalidPointerFlow::flowPath(pSource1, _, pragma[only_bind_into](pSink1),
      pragma[only_bind_into](pSink2)) and
    // ...
  )
}

[jketema]

I see now. Why is all this complexity needed, and why can we just use the source from AllocToInvalidPointerConfig?

[MathiasVP]

It needs to be a source that actually flows to a sink. If we just write AllocToInvalidPointerConfig::isSourcePair(source1, _, _, _, _) we'll mark all allocations as sources irregardless of all the filtering that the rest of the query has done.

[MathiasVP]

That is why this final flow configuration is basically "for free" (as can be seen in the DCA run). There are not a lot of sources/sinks now since this configuration is really jus there to make sure the call contexts match up

[MathiasVP]

with more and more functionality piling into invalidPointerToDerefSource.

This PR doesn't do anything to invalidPointerToDerefSource other than renaming a variable and moving another variable from exists to a parameter.

This is a query structuring issue.

In that case, I'll think about how we can change the structure of the query, then.

[jketema]

This PR doesn't do anything to invalidPointerToDerefSource other than renaming a variable and moving another variable from exists to a parameter.

Allowing the predicate to be used in one more non-obvious way. Maybe we should start by actually properly naming the predicate (because it being a source of one of the dataflow configurations is probably the least relevant of all its uses at the moment), and also giving its parameters more descriptive names.

More general, every name that ends in a number should probably get a more descriptive name that does not involve number.

[MathiasVP]

Those are definitely some easy changes to make. I've created an internal issue for these things, and I suggest we delay this PR (as well as #13725) until this has been completed.

[jketema]

I wonder if we could actually properly simplify the query based on the work you did here. In particular:

• Could we get rid of TMergedPathNode by adding another state and step to FinalConfig that goes to the final load/store instruction?
• Could we get rid of InvalidPointerToDerefConfig by merging its functionality into FinalConfig?

[MathiasVP]

I was able to get rid of TMergedPathNode in 5e06043. I'm still trying to think about how we can get rid of InvalidPointerToDerefConfig by merging its functionality into FinalConfig.

[jketema]

My main comment seemed to have gone missing. Here you go.

[jketema]

This doesn't look correct to me, as the paths we display now no longer start from an allocation.

[MathiasVP]

I think this comment is a side-effect of the misunderstanding I've tried to clear up here: source is indeed the node that represents the allocation, and when we write:

invalidPointerToDerefSource(source, _, derefSource, _) and
InvalidPointerToDerefFlow::flow(derefSource, _)

we're saying: "the source is the node that represents the allocation, and the derefSource must be a node that flows to a dereference (as identified by InvalidPointerToDerefFlow).

Does that make sense?