Java: Add a global extension point for taint steps.

[aschackmull]

This should make it easier to add custom taint steps that apply to all queries.

[yh-semmle]

This interface differs slightly from the equivalent one in Javascript. Would it make sense to make this consistent across languages? (cc @xiemaisi)

Perhaps also add a similar warning re performance to the QLDoc?

[xiemaisi]

I agree that it would be nice to make the API consistent. @aschackmull, could you explain the advantages of your proposed API? (The name of the class doesn't matter since we can always provide aliases, but the member predicate name and signature more difficult to assimilate.)

[aschackmull]

For javascript there's the class

abstract class AdditionalTaintStep extends DataFlow::Node {
    abstract predicate step(DataFlow::Node pred, DataFlow::Node succ);
  }

I find this interface confusing as it is using a 3-column relation to specify a 2-column step relation. As far as I can see the this column is mostly ignored and appears to be arbitrarily restricted to be equal to one of the two other columns in the various overrides. I also think the name of the class in the javascript QL is slightly off (as it says it's a step, but really it's a single node), but that ties into the this column being mostly ignored, so a name describing the this column isn't tied to much.

As for the performance warning, I don't think it applies in the same way here as for javascript, since the default taint steps that we include are added as a separate predicate instead of being part of the dispatch, so that means we could cache it separately (which we btw. don't do currently - I haven't checked whether it would be worth it)

[xiemaisi]

Thanks. Unfortunately that explanation does not convince me, in particular I would expect step to look like a binary predicate, not a predicate-with-a-result as in your API. The fact that in JavaScript this must be bound to something is indeed unfortunate, but the alternatives in present-day QL seemed worse.

I do, however, acknowledge that this is a matter of taste, so if most people feel that this API is better I won't stand in the way of its adoption.

[aschackmull]

I could of course also use a single ordinary column instead of the result column:

abstract predicate step(DataFlow::Node node);

[aschackmull]

But I'm not sure that's better. It remains that it unfortunately isn't possible to write an abstract predicate in QL that both looks like a binary predicate and also is a binary predicate.

[aschackmull]

@jbj Suggests a third alternative, add an ipa unit-type as a base of the class, such that the this column is a trivial one-valued column. Then we can have a looks-like-binary predicate that also effectively is sort-of binary.

[xiemaisi]

That is a very interesting idea. I think I would like that approach (even though it would still require an incompatible change in the JavaScript libraries, and hence would take time).

What do others think? (@esben-semmle, @asger-semmle, ...)

[asger-semmle]

The unit type sounds good to me. I've been tempted to do the same thing in a few other places.

[aschackmull]

Updated to use a unit type.