Regarding duplication: neither of those solutions work, as the source nodes and the sink nodes of the duplicated results might have no other solution than that the underlying expression is the same. There's no flow between the two alternatives. The only thing that is giving me something close to the old result is dropping in something like:

  predicate hasFilteredFlowPath(DataFlow::PathNode source, DataFlow::PathNode sink) {
    this.hasFlowPath(source, sink) and
    not exists(DataFlow::PathNode source2, DataFlow::PathNode sink2 |
      this.hasFlowPath(source2, sink2) and
      asSourceExpr(source.getNode()) = asSourceExpr(source2.getNode()) and
      asSinkExpr(sink.getNode()) = asSinkExpr(sink2.getNode())
    |
      not exists(source.getNode().asConvertedExpr()) and exists(source2.getNode().asConvertedExpr())
      or
      not exists(sink.getNode().asConvertedExpr()) and exists(sink2.getNode().asConvertedExpr())
    )
  }

this above is the defined configuration.

Also it turns out the DCA results are very deceptive here although DCA only reports 19 more results for vim, the result count (even with the above predicate dropped in) goes from slightly over 200 to slightly over 600(!).

Regarding duplication: neither of those solutions work, as the source nodes and the sink nodes of the duplicated results might have no other solution than that the underlying expression is the same. There's no flow between the two alternatives.

That's unfortunate. Where there are two alternatives with no flow between them, can we identify a common parent perhaps?

can we identify a common parent perhaps?

What do you mean by parent?

What do you mean by parent?

I was thinking on the AST, but any kind of parent would do. If the duplicates, where they exist, always have a common parent (that would not be common to a non-duplicate) we can pair them against each other and choose one of the duplicates to keep. I'm not sure what kinds of things we actually have at the duplicate sources and sinks, however. Might have to dig deeper.

I was thinking on the AST

Well, that's the problem right: it's the same expression twice in each case. Hence the code in #11435 (comment), where is just pick one at "random".

It looks like you're way ahead of me in the discussion thread with @MathiasVP , so I'll leave this to the two of you now. Let me know if you get stuck / would like my opinion on anything.

As far as I can currently tell, most new results I'm seeing in DCA are coming from the rewritten version not having a sanitizer mimicking the following one from default taint tracking

As far as I can currently tell, most new results I'm seeing in DCA are coming from the rewritten version not having a sanitizer mimicking the following one from default taint tracking

In general, I really don't like that sanitizer in DefaultTaintTracking (since a node may be marked as a sanitizer when the equality comparison is totally unrelated to the dataflow path). It's a good hammer for when a query has an unreasonable number of FPs, but my guess is that it's a lot more useful in query that does taint through integral expressions (where I'd expect a lot more equality comparisons).

Turns out the real difference in number of results was coming from hasUpperBoundsCheck. Having that is not ideal either, but introducing it in this rewrite gives as very similar before and after results in DCA.

I briefly discussed with @geoffw0 and I agree with him that we should aim here for a very similar set of results when comparing with the version we had before the rewrite. Improving the query is a separate task, which we can do at a later moment, and which should now be easier, as we no longer depend on DefaultTaintTracking.

MRVA results on 996 projects (one workflow timed out): https://gist.github.com/jketema/b2da9ff4d0939ca3a63e15bbcd05a556 There are 188 result changes in total (both lost and gained). This seems acceptable to me.

As briefly discussed last week. Could you have a look at this @rdmarsh2? Especially the hasFilteredFlowPath predicate.

Could we use RemoteFlowSource/LocalFlowSource rather than isUserInput to get the "right thing" for sources? For sinks, I'm surprised we can't just use the operand of ReadSideEffect. I think that would get rid of the need for the asSourceExpr/asSinkExpr wrappers and for hasFilteredFlowPath.

Could we use RemoteFlowSource/LocalFlowSource rather than isUserInput to get the "right thing" for sources?

If I replace isSink by:

  override predicate isSource(DataFlow::Node node) {
    node instanceof LocalFlowSource
    or
    node instanceof RemoteFlowSource
  }

I'm starting to lose results. In particular scanf related ones like:

    char fileName[20];
    scanf("%s", fileName);
    fopen(fileName, "wb+"); // BAD

For sinks, I'm surprised we can't just use the operand of ReadSideEffect.

That seems to work for the test cases we have. I frankly never tried this.

I'm starting to lose results. In particular scanf related ones like:

I hope we can do such a rewrite as a follow-up. We need to check that the FlowSource class covers all the cases that isUserInput covers, though.

We need to check that the FlowSource class covers all the cases that isUserInput covers, though.

Yeah, it currently doesn't, but let's not pull that into this PR.

With the sink changes suggested by @rdmarsh2 there are 65 additional results (all losses in this case) in MRVA. Given the number of results this query gives, that's probably acceptable. So, if the query looks fine in its current form, I'd like to move on.

I'll file an internal issue for the scanf/LocalFlowSource issue identified above.

Does this need a change note?

Does this need a change note?

I'd say no.

I'll file an internal issue for the scanf/LocalFlowSource issue identified above.

And fscanf, I think in CPP we consider file reads to be local flow sources as well. These sources will benefit other queries as well.