C++: Simplify buffer.qll repair

[MathiasVP]

#10833 prepared the buffer.qll library for use-use flow. In the end, the PR ended up being a lot more complicated than I had anticipated.

After thinking a bit about this, I reached the conclusion that this is all because of the added use of asIndirectExpr to convert an expression to a DataFlow::Node in the buffer library. The complexity comes from the fact that, if n is the dataflow node tracking *p, then there's flow from that node to the indirect node tracking *(++p). So by using node.asIndirectExpr in the buffer library we would have dataflow through operations that incremented a pointer.

This required a bunch of complexity: It required the need for localFlowIncrStep (to track how much the pointer was incremented), and because this predicate had an integer column to count how much the pointer was incremented, the library could suddenly diverge on loops like:

while(true) { ++p; }

because we'd have dataflow from the indirect node of p to the indirect node of ++p, and localFlowIncrStep would loop forever incrementing the integer column. To fix this, I disabled flow through back-edges in localFlowNotIncrStep.

All in all, this was kinda messy. @jketema also reported that he was seeing multiple possible buffer sizes returned by the getBufferSize predicate, which is certainly not intended.

I think this PR is a much safer and simpler way to approach this. By not using asIndirectExpr we avoid this extra flow, and we don't need to block flow through back-edges since we don't have the localFlowIncrStep predicate anymore. The end result is a much simpler implementation 🎉. It might also solve multiple sizes being reported. @jketema let's discuss this on Monday.

The PR is split into two commits. The first commit contains all of the fixes explained above, and the second commit is purely a performance optimization (see the commit message for tuple counts. Note that the number of tuples in the commit message changes because the predicate is now part of the recursion. The number of tuples in the main predicate at the end is unchanged, though).

[jketema]

Because only the delta is looked at in the unique, there are unfortunately still cases where we're getting results where in the old situation we were not getting any:

void test15(bool b)
{
	char in_buffer1[8000];
	char in_buffer2[9000];
	char out_buffer[9000];
	
	char *in_buffer = b ? in_buffer1 : in_buffer2;

	bcopy(in_buffer, out_buffer, sizeof(out_buffer)); // GOOD: whether we're reading too much depends on `b`, whose value we do not know.
}

I think unique should be about complete buffer sizes and not just a delta (as was done in the original version).

[MathiasVP]

Thanks for the testcase, @jketema! Indeed, we don't want to report anything in that one. After I noticed this testcase:

union myUnion {
  char small[10];
  char large[100];
};

void myUnionTest()
{
  myUnion mu;
  // ...
  memset(&(mu.small), 0, 200); // BAD

I realized that what we're after isn't actually unique since both mu.small and mu.large flows to the memset (because mu is a union). Instead, what we want is to calculate to calculate the largest possible buffer size. This means that we do get a result in the above case (since 200 is larger than the largest possible buffer that flows to memset), but we don't get a result in your case above since the largest possible buffer that flows to in_buffer has size 900 (which matches the size of the write).

30f1547 removes the unique aggregate in the delta case, and instead uses max around the complete buffer size.

[jketema]

I would have expected that the union case was already handled by:

  exists(Union bufferType |
    // buffer is the address of a union member; in this case, we
    // take the size of the union itself rather the union member, since
    // it's usually OK to access that amount (e.g. clearing with memset).
    why = bufferExpr.(AddressOfExpr).getAddressable() and
    bufferType.getAMemberVariable() = why and
    result = bufferType.getSize()
  )

Why isn't this sufficient?