`MEM53-CPP`: False positive due to flow through `realloc`

[MathiasVP]

Affected rules

• MEM53-CPP

Description

In https://github.com/github/codeql/pull/14637 we added taint-flow through the indirection of the pointer passed to realloc to the indirection of the result. That is, flow through the following example:

int* p = ...;
*p = tainted_value;
int* q = (int*)realloc(p, 1024);
sink(*p);

this relies on the new taint-tracking library to distinguish between the result of realloc(...), and the result of what realloc(...) points to. Since the old AST-based taint-tracking library cannot do this this results in a FP in the testcases for MEM53-CPP (that we accepted on the next branch here: https://github.com/github/codeql-coding-standards/pull/419)

The query already tries to rule out realloc cases by excluding them in the definition of the taint-tracking configuration's isSource, but to get this query back to not reporting a FP here a barrier on realloc would have to be inserted.

As @jketema points out the affected test is actually really sketchy since there’s no guarantee that memory allocated with new can safely be realloc'ed. So maybe this scenario should be thought about more carefully by someone on your team.

[lcartey]

Thanks for the report! I recently merged this change from next into main, and modified the query to apply realloc as a barrier, which makes the behaviour consistent with how the query worked before https://github.com/github/codeql/pull/14637.

I believe the current test case for realloc should use malloc for the allocation initially, before using placement new. Then the test case wouldn't trigger undefined behaviour.

In addition, we should consider whether we want to cover any of the realloc cases under this rule. I think a common cases would be an array of objects that is being expanded. Do we then expect to see a constructor call for each new instance added in this case, and flag otherwise?