C++: Fix some FPs in cpp/invalid-pointer-deref

[MathiasVP]

This fixes the FPs I added in #12953. The query is designed to search for patterns such as:

int* p = malloc(n);
int q = p + n;
...
use(*q);

but by marking any PointerArithmeticInstructions as a sink (or rather: anything that upper bounds a PointerArithmeticInstruction) we get too much flow (as illustrated in #12953). So this PR restricts the sinks to only the PointerArithmeticInstruction such that:

• The operation appears on the right-hand side of a store (which is what the query always intended to do). After this store, we do a simple global dataflow to find the place where the value is dereferenced (as the query already does), or
• the operation is directly loaded from. This would constitute an immediate out-of-bounds dereference and we don't need to search for a StoreInstruction.

[MathiasVP]

Force-pushed a rebase to resolve the testcase conflicts after merging #12960.

There are some lost results. I'll investigate if they are the FPs (and are represented well by our tests).

[jketema]

This looks sensible, but I have a very hard time understanding why this solves the referenced FPs. If I, for example, look at:

void test15(unsigned index) {
  unsigned size = index + 13;
  if(size < index) {
    return;
  }
  int* newname = new int[size];
  newname[index] = 0; // GOOD [FALSE POSITIVE]
}

There is exactly one PointerAdd in the IR and that is used in a Store. So it seems this should not be a case that is ruled out by this PR, yet the FP disappears.

[jketema]

Suggested change

	// it's fine to treat pointer arithmetic as conversions as conversions
	// it's fine to treat pointer arithmetic as conversions

[MathiasVP]

There is exactly one PointerAdd in the IR and that is used in a Store. So it seems this should not be a case that is ruled out by this PR, yet the FP disappears.

But that PointerAdd is not flowing into the value of a store, instead, it's flowing into the store address 🙂.

[jketema]

But that PointerAdd is not flowing into the value of a store, instead, it's flowing into the store address 🙂.

My understanding of what the query is supposed to do is that were are actually only interested in the addresses from with we load and store, so this only adds to my confusion. Why would we look at all at values being stored?

[MathiasVP]

After sleeping on this I realized that you're right, @jketema. The query really shouldn't need to deal with this. The motivation for this PR was to remove FPs like:

void test15(unsigned index) {
  unsigned size = index + 13;
  if(size >= index) {
    int* newname = new int[size];
    newname[index] = 0; // GOOD [FALSE POSITIVE]
  }
}

like we discussed above, this FP is removed because newname[index] is neither the right-hand side of a StoreInstruction, nor is it directly loaded from. However, it turns out that the root cause of this issue is that the size >= index guard makes the range-analysis library think that size >= index, even though size >= index + 13 is a much more precise bound.

I'll pull this PR into a draft state while we discuss how to solve this problem.