Overriding a java rule with a custom version


(Robert Smith) #1

Hi all

I have the scenario where we have an external library we use in our java code. There are two places where we are getting lots of false positives, and ideally I’d like to add some custom rules to override the default checks. The library has a lot of JNLI wrappers, which I think are confusing SQ.

  1. Functions in the library can return nulls, but SQ cannot identify this and flags it with "Remove this expression which always evaluates to “true”"
ResultObject result = library.Util.getResult(...);
if (result != null) { ... } // this is flagged as always true, but in reality it might not be
  1. Library functions check for null / cause the function to return, so null cannot be thrown, but it is reported as possible (A “NullPointerException” could be thrown; “X” is nullable here)
ResultObject result = library.Util.getResult(...);
// checks for null
if (library.Util.isResultValid(result)) {
  result.doSomething(); // this is flagged as NullPointerException could be thrown 


ResultObject result = library.Util.getResult(...);
// this could be any check that resolves to true
if (true) {
  library.Util.exitFailure(); // at this point, this is analogous to "return"
result.doSomething(0); // this is flagged as NullPointerException could be thrown, as SQ does not know the class stops there

I am really just looking for some pointers, is this possible to do, and where to start looking if so please.

Thanks, Rob

(Michael Gumowski) #2

Hello Rob,

Unfortunately, the rules you are talking about are all based on our Symbolic Execution Engine. Their implementation is therefore extremely complex, and pretty hard (if not impossible) to mimic with custom rules. I would definitely not try the “custom” approach. Unfortunately, you are here hitting one of the painful limitation of the engine, and I don’t see any simple other way out than marking these issues as FPs.

Now… Bear with me if you wanna try something a bit more interesting and challenging, because you need a bit of context. :slight_smile:

To summarize (a lot), the symbolic execution engine tries to symbolically execute the body of each methods, following constraints on parameters and variables it learns along the way, searching for possible issues (conditions always true/false, NPE, unclosed resources, etc.). The engine also deduce method behaviors of methods, which it uses when handling method invocations. These behaviors are generated (and cached) on the fly, when the called methods are present in the same file, by following invocations.

Now, when a called method is outside the file (meaning accessed by the engine only through the bytecode), this is a different story. In such case, by default the engine only look at signatures and annotations, in order to deduce eventual constraints on parameters and return values, but it does not try to deduce the real method behavior… It’s however doable, and it has been implemented in SonarJava. However the results we were observing during final stages of implementation were not satisfactory enough to enable the feature in production. This behavior is consequently partially disabled. For reference, we call the feature Cross-File analysis (X-File).

From how I see it, allowing the engine to explore your library (library.Util.getResult(), …) to deduce such method behaviors could solve the issues, by deducing the correct nullness constraints. It is also possible that it will reach the limit of the engine and generate new FPs. But it worth trying.

The engine is currently configured in order to not rely on x-file analysis, except for a very limited set of hardcoded methods from known libraries, for which we know the results are correct.

If you want to dig into the SonarJava code and experiment a bit, I see two things that you may want to try, preferably in that order:

  1. Experiment with SonarJava itself:
    We have been discussing a bit internally about adding an experimental feature allowing users to specify extra methods to be analyzed by the engine, relying on bitecode. But we never pushed the idea into implementation. This is currently covered by the following ticket, which still need some refining: SONARJAVA-2778
    For the time being, the list of allowed methods is hardcoded. See SonarSource/sonar-java/java-frontend/src/main/java/se/xproc/BehaviorCache.java#L48 for the full list. You may try to add the methods from your utility class(es) into this list, and see if the engine is able to deduce the adequate behaviors. Don’t hesitate to ask if you need some help on expected syntax, but the code should be self-explanatory. If this approach is working for you, we may consider working on allow customization of this white list for users. But we desperately need some feedback.

  2. Enabling x-file during analysis:
    [:warning: This is an experimental and undocumented feature, it may impact considerably analysis time, memory consumption, and potentially generate numerous FPs. Analysis failures on unrecognized bytecode instructions could also be a possible outcome. It is also most probable that we are going to drop the whole feature at some point in the future. :warning:]
    Now that you have been warned… :smiley: You may try to enable the full x-file analysis for your project. To do so, set the property sonar.java.xfile to true in your configuration file or pom. The engine will then dig into each non-overrideable method it find (static, final, etc.) and compute method behaviors to be used during symbolic execution. It will then explore your binaries, but it can also generates tons of FPs if at some point of the process, a computed behavior is utterly wrong. Hopefully, it will reduce the noise of the rules, but it’s not guaranteed.

Finally. I know I’m stating the obvious, but please do not try any of these points on your production instance.

I would also appreciate any feedback if you decide to try any of these options.

Hope this helps,