C++: Fix the two null termination queries and re-enable them.

[geoffw0]

This is my proposed fix (update: major contributions from @MathiasVP) for performance of the two null termination queries that regressed the CPP-Differences suite last week (caused by #6794 and temporarily fixed by #6893). It is not behaviour preserving, as I didn't have much luck making an improvement that was. I'm very much open to suggestions of other approaches and discussions about tradeoffs.

This PR also re-enables the two queries as they should now perform acceptably.

The issue is in the NullTermination.qll library, that is only used by these two queries. It does not appear to be related to the recent QL changes themselves, rather just the fact that these queries were enabled. The issue does not affect all projects, but for example in php the time to compute mayAddNullTerminator exploded (specifically the "Assignment to another stack variable" part).

Performance:

• locally I get a speedup from 658s -> 294s 295s for cpp/improper-null-termination and 882s -> 529s 538s for cpp/user-controlled-null-termination-tainted on php (the remaining time I believe is largely cache warmup of SSA, def-use etc).
• the DCA results show a 28% speedup on php comparable to the original slowdown that is 32% of the slow run. TODO: re-run with the new strategy.
• locally I get a speedup from 1213s -> 1130s 939s for cpp/improper-null-termination on wireshark; less of a speedup, the issue in mayAddNullTerminator is resolved but there may be a similar problem in variableMustBeNullTerminated that remains.

Results:

• the change makes the "Assignment to another stack variable" case coarser, thus mayAddNullTerminator is wider so we will in principle see fewer query results.
• in local runs, on tests, and on DCA I saw no query differences.
• TODO: it's probably worth running an LGTM diff query, there will likely be something to see.
• identical with the new strategy.

@MathiasVP - I've not made this a draft, but I feel we should discuss it and possibly iterate before merging.

[MathiasVP]

It looks like a join-ordering issue where b.getASuccessor+() = bb0 is computed before the endpoints are properly restricted:

Tuple counts for NullTermination::mayAddNullTerminator#ff/2@i5#e422dw after 2m26s:
  43960      ~0%        {1} r1 = SCAN NullTermination::mayAddNullTerminator#ff#prev_delta OUTPUT In.1
  29502      ~155%      {1} r2 = JOIN r1 WITH Access::Access::getTarget_dispred#ff ON FIRST 1 OUTPUT Rhs.1
  13236      ~256%      {2} r3 = JOIN r2 WITH Function::Function::getParameter_dispred#fff_201#join_rhs ON FIRST 1 OUTPUT Rhs.1, Rhs.2
  13236      ~256%      {2} r4 = r3 AND NOT NullTermination::functionArgumentMustBeNullTerminated#bb(Lhs.0, Lhs.1)
  78728      ~167%      {2} r5 = JOIN r4 WITH Call::Call::getTarget_dispred#ff_10#join_rhs ON FIRST 1 OUTPUT Rhs.1 'e', Lhs.1
  72224      ~163%      {2} r6 = JOIN r5 WITH Call::Call::getAnArgumentSubExpr#ffb ON FIRST 2 OUTPUT Rhs.2 'va', Lhs.0 'e'
  60087      ~152%      {2} r7 = JOIN r6 WITH Access::VariableAccess#f ON FIRST 1 OUTPUT Lhs.0 'va', Lhs.1 'e'
  
  905853     ~0%        {3} r8 = SCAN NullTermination::mayAddNullTerminatorHelper#fff#prev_delta OUTPUT In.1, In.0 'e', In.2
  905853     ~1%        {3} r9 = JOIN r8 WITH Access::VariableAccess#f ON FIRST 1 OUTPUT Lhs.1 'e', Rhs.0, Lhs.2
  
  905853     ~1%        {4} r10 = JOIN r9 WITH BasicBlocks::Cached::basic_block_member#fff@staged_ext ON FIRST 1 OUTPUT Rhs.1, Lhs.1 'va', Lhs.0 'e', Lhs.2
  3127370279 ~3%        {4} r11 = JOIN r10 WITH #BasicBlocks::BasicBlock::getASuccessor_dispredPlus#ff ON FIRST 1 OUTPUT Lhs.3, Rhs.1, Lhs.1 'va', Lhs.2 'e'
  812954     ~1%        {5} r12 = JOIN r11 WITH BasicBlocks::Cached::basic_block_member#fff@staged_ext ON FIRST 2 OUTPUT Lhs.0, Lhs.1, Rhs.2, Lhs.2 'va', Lhs.3 'e'

I think you can get a simpler (and behaviour-preserving) fix by replacing:

mayAddNullTerminatorHelper(e, va, e0) and
bb.getNode(pos) = e and
bb0.getNode(pos0) = e0

with

mayAddNullTerminatorHelper(pragma[only_bind_into](e), va, pragma[only_bind_into](e0)) and
pragma[only_bind_into](bb).getNode(pos) = e and
pragma[only_bind_into](bb0).getNode(pos0) = e0

that should force the compiler to only compute bb.getASuccessor+() after bb and bb0 have been restricted.

[geoffw0]

I've pushed a commit updating to your suggestion, then a commit applying similar changes to variableMustBeNullTerminated, then a commit generalizing the solution into a prototype nodeBefore predicate. I'd like your thoughts on the last bit. It's definitely neater, but I'm not sure if it's suitable for more general use (if it is we should probably move it into one of the node classes; if it isn't we should just make it private; I'm also happy to undo that change if we lack confidence in it).

[rdmarsh2]

The naive version using ControlFlowNode.getASuccessor() is a common pitfall for new QL writers, so I'd be in favor of adding nodeBefore as a public predicate on ControlFlowNode, although I'd like a better name for it.

[MathiasVP]

The naive version using ControlFlowNode.getASuccessor() is a common pitfall for new QL writers, so I'd be in favor of adding nodeBefore as a public predicate on ControlFlowNode, although I'd like a better name for it.

Indeed, I think it would be super nice to have a transitive version of getASuccessor that is both easy to use (like ControlFlowNode.getASuccessor) and efficient (like nodeBefore).

I'd like your thoughts on the last bit. It's definitely neater, but I'm not sure if it's suitable for more general use (if it is we should probably move it into one of the node classes; if it isn't we should just make it private; I'm also happy to undo that change if we lack confidence in it).

For now, I think it's best to make nodeBefore private, and then create an issue saying something like "we should have a public predicate like nodeBefore, and replace the uses of nodeBefore (and related predicates in other libraries and queries) with that public predicate".

[geoffw0]

I've added some tests, fixed a bug revealed by the tests, renamed the predicate (though we can and should use a shorter name if we make it a member predicate of ControlFlowNode) and made it private. A few thoughts:

• @jbj pointed out that many uses of the old pattern / new predicate would probably ideally be doing something else instead - using dominance, dataflow, def-use, use-use, GVN or some other higher level library. Dominance is the closest to what we have but in the case of NullTermination.qll I believe it is not as suitable (we only care that there is a path, not about all paths).
• there are quite a lot of uses of ControlFlowNode.getASuccessor+ / getASuccessor* in our code (not to be confused with those same predicates on BasicBlock).
• I've created a ticket for the future work, please by all means discuss on there: https://github.com/github/codeql-c-team/issues/694

[MathiasVP]

LGTM! The PR check failure is not related to this PR.

[geoffw0]

The PR check failure is not related to this PR.

Would it help if I retry the check, or merge latest main?

[MathiasVP]

Would it help if I retry the check, or merge latest main?

Yes. This has been fixed on the latest main. So you should be fine if you retry.

[geoffw0]

I don't seem to be able to get DCA working right now. I'll try again / try something else to confirm performance tomorrow.

[MathiasVP]

Do tell me if I can help with anything!

[geoffw0]

DCA results are in:

• there will be a 4% overall performance cost to enabling the queries (compared to ~30% before the improvements). Some cost is to be expected when we're enabling new queries.
• PHP is fine now.
• It might still be worth investigating one or more of the now most affected projects: wireshark (large), vim, git or boost.
• there are a fair number of new results, as we'd expect for enabling two queries. In theory these should be the same results as at the time we reviewed C++: Improvements to cpp/improper-null-termination #6794 . In practice the results in SAMATE appear pretty good, while the real world ones are a very mixed bag - I'm not entirely happy with them. The queries definitely at best deserve a medium precision tag for the time being.

So the performance improvements are good, but I think this isn't quite ready to merge yet...

[geoffw0]

I've just removed the part of this PR that re-enables the queries, leaving us with just the performance fixes which I think are uncontroversial at this point. The intent being to get what we have here merged as I'm working on other things.

I've extended the ticket for to this work with the stuff I discussed doing above, so we should be able to come back to it.

[MathiasVP]

I've just removed the part of this PR that re-enables the queries, leaving us with just the performance fixes which I think are uncontroversial at this point. The intent being to get what we have here merged as I'm working on other things.

Sounds good.

[MathiasVP]

LGTM!