C++: Use fully converted expressions for cpp/use-after-free and cpp/double-free

[MathiasVP]

In tests such as:

void test_ref_delete(int *&p) {
  delete p;
  p = new int;
  use(p); // GOOD [FALSE POSITIVE]
}

we were getting a FP because we didn't block flow at the write to p. The reason dataflow wasn't blocking this write was because we didn't use the result of the reference dereference as the source, but instead we were using the value of type int*& as the source (because asExpr doesn't return ReferenceDereference expressions since they're conversions).

The fix is to actually pick the fully converted expression (i.e., the value of type int*) as the source for dataflow. SSA then correctly sees that the variable is reassigned, and the flow is blocked.

Commit-by-commit review recommended:

• The first commit changes the sources in cpp/use-after-free and cpp/double-free to use the fully converted expression. This commit accepts one regression caused by asConvertedExpr never returning a ParenthesisExpr.
• The second commit fixes the above regression. This introduces yet another regression because LeapYear.qll uses asConvertedExpr instead of asExpr. The library really shouldn't deal with conversions since it looks like the rest of the library only handles unconverted expressions
• The final commit fixes LeapYear.qll by the changing use of asConvertedExpr to asExpr.

[alexet]

LGTM

[alexet]

Is there a reason why we don't make this change at the IR level to have the instruction also return ParenthesisExpr rather than add a special case here? The other cases feel more like adjusting to make dataflow easier rather than more correct but this just feels like a missing case for at the IR level.

There's not really any reason to change it here I am just wanting to understand the decision.

[MathiasVP]

I think there are two reasons that combine to fully explain this:

1. Given an instruction i there's always at most one result for instr.getConvertedResultExpression().
2. Reading https://github.com/github/codeql/blob/main/cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/internal/TranslatedExpr.qll#L3236 it sounds like it's a conscious decision to not generate a CopyValue instruction for parenthesis expressions.

So since instr.getConvertedResultExpression() can only return one expression, and there's no instruction generated for parenthesis expressions, I don't think there's an easy way to fit this change into the IR level.