squid:S2259 - question, maybe false positive

[kova]

Hi!

I could not figure out why the warning comes in case 1 and not in another one? Any ideas?
I’m using SonarLint for Eclipse, version 3.5.0.201804241315

Thanks for feedback!
Best Regards

[saberduck (Tibor Blenessy)]

Hello,

this is quite interesting case revealing weakness in one of the heuristics of our symbolic execution engine in SonarJava. The explanation is bit longer.

First, default behavior of the engine is that if we are not sure, we don’t raise issue for S2259, to avoid noise. So if we have no knowledge about the argument of the function and something is invoked on it, we will not raise an issue. That’s why following simple function will not raise issue, even though if you pass null as an argument, it will throw NPE.

String toLower1(String s) {
    return s.toLowerCase();  // No issue here
}

You can tune this behavior by providing some knowledge with annotations on argument, but that’s unrelated here.

One heuristics we apply is, that if we see in the execution of the function that there is comparison with null, we will consider this as an indicator that this value can actually have null value. So following will trigger the rule.

String toLower2(String s) {
    if (s == null) {
      System.out.println("s actually can be null, huh?");
    }
    return s.toLowerCase();  // S2259 issue raised
  }

In your example one more trick comes to play, which is short-circuit behavior of && operator. So when s == null is on the left side of && it means, that engine will start to consider the possibility of s having null value. However if null check is on the right side of && it will never be evaluated, because engine sees two options:

• i != 1 and we know nothing about s, so this is same situation as function toLower1 above
• i == 1 && s != null , s is not null thus all is good!

I hope that explains the behavior you are observing. I created following issue to track this weakness SONARJAVA-2796 , however this is bit tricky to fix, so I am not sure when we will try to tackle it.

Thank you for reporting it.