C++: Fix branch related FPs in cpp/improper-null-termination.

[geoffw0]

Fix branch related FPs in cpp/improper-null-termination, for example:

if (cond) {ptr = not_null_terminated_string()} else {ptr = null_terminated_string()}
...
if (cond) {add_null_termination(ptr)}
...
use_null_terminated_string(ptr);

Previously this was a false positive result because, although the string is always null terminated before the point of use, this is only true because certain paths through the control flow are impossible (namely taking cond == true on the first line and cond == false on the third line).

The solution I've used to is narrow the query - considering the presence of something that null terminates the string on any path to be enough, rather than requiring a barrier on all (supposed) paths.

---

LGTM diff query: https://lgtm.com/query/4160689657278568975/ (there are still other types of FPs in the results, but they seem like an improvement)
Performance: about the same (locally)

[MathiasVP]

LGTM other than a tiny QLDoc request.

[MathiasVP]

Could we highlight in the QLDoc that this is control flow (and not dataflow, which might be the expected kind of flow when reading this sentence)?

[geoffw0]

Done.

(I'm not entirely sure why this isn't data flow, but it seems to be working well enough)

[MathiasVP]

Yeah, I think it would be valuable to compare the current set of results to a version that uses semmle.code.cpp.dataflow.DataFlow, semmle.code.cpp.ir.dataflow.DataFlow, or event semmle.code.cpp.ir.dataflow.TaintTracking.

[geoffw0]

I just had a try at both. AST dataflow produces a lot of false positives, as best I can tell because it doesn't really know when a pointed buffer might be overwritten. e.g.

char buffer1[1024];
char buffer2[1024];

strcpy(buffer2, "content");
strcpy(buffer1, buffer2); // <-- I get a false positive on buffer2 here as though the line above didn't exist.

The IR DataFlow might do better at that but fails at the source - in this and most other cases an uninitialized buffer. UninitializedNode is deprecated in the IR (and does not work) so I don't get 90% of the sources.

Unless we can fix one of those, the quality of results using data flow is much worse than the current StackVariableReachability version.

[MathiasVP]

LGTM! I don't think we should change any control-flow to dataflow in this PR, so let's just merge these changes now.