Incorrect analysis of squid:S2259

[burge...@gmail.com]

Hi Group,

in our project, we have noticed incorrect results for rule squid:S2259 . We are currently using sonar qube version 6.7.3 with sonar java 5.3 (build 13828) . 

I can share with you some pseudo code which triggers this rule - even though the null check is handled in the previous method.

if (isGreaterThanZero(abc)) {

   return abc.multiply(abcd);

}

public static boolean isGreaterThanZero(BigDecimal value) {

   return value != null && BigDecimal.ZERO.compareTo(value) < 0;

}

The sonar will flag the variable "abc" as the potential null reference, even though the method isGreaterThanZero is checking the input value for null and returning false in that case.

Is this something which could be improved in future versions of sonar java plugin, or some specific limitation of analyzers?

[Michael Gumowski]

Hello Jozef,

Thanks for the feedback. Unfortunately, I'm not able to reproduce what you are observing with latest released version of SonarJava (5.4). However, we didn't worked on the symbolic execution engine (driving rule S2259) during the 5.4 sprint, so I would have expected to be able to reproduce it from your example.

I used the following self-contained code for my test:

package org.foo;

import java.math.BigDecimal;

class A {

  static BigDecimal foo(BigDecimal a, BigDecimal b) {

    if (isGreaterThanZero(a)) {

      return a.multiply(b); // FP S2259 here ?

    }

    return null;

  }

  public static boolean isGreaterThanZero(BigDecimal value) {

    return value != null && BigDecimal.ZERO.compareTo(value) < 0;

  }

}

Could you maybe provide a small self-contained code snippet reproducing the issue systematically? Because I'm probably lacking some particularity.

It would most probably help me to understand where is the difference and what's lead the symbolic execution engine to the wrong conclusions. Currently, from my example, the engine deduce the following method yields for method "isGreaterThanZero":

Concretely, it means that the only way to enters the "IF" statement is by following yield #2,  which implies arg0 (abc in your case) to be necessary not null. So I hardly see how we could deduce that abc is null inside the IF.

A few other questions which may help:

• Can you check if you are providing correctly the bytecode to the analysis?
• Is the isGreaterThanZero method part of the same file as the file reporting the FPs?
• Are you using some nullness annotations in your code ? (@Nullable, @Nonnull?)

Regards,

Michael

--
You received this message because you are subscribed to the Google Groups "SonarQube" group.
To unsubscribe from this group and stop receiving emails from it, send an email to sonarqube+...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/sonarqube/dfa33791-15c7-4c44-a1a6-904301eed8d0%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

--

Michael Gumowski | SonarSource

Software Developer, Language Team
https://www.sonarsource.com

[burge...@gmail.com]

You are right - with the example itself I was not able to reproduce it either . I have changed the example to more closely follow our actual code

public class A {
   public BigDecimal foo() {
     BigDecimal a = null;
     if (isGreaterThanZero(a)) {
        return a.multiply(BigDecimal.ONE); // FP S2259 here
     }
     return null;
   }
}

public class B {
   public static boolean isGreaterThanZero(BigDecimal value) {

      return value != null && BigDecimal.ZERO.compareTo(value) < 0;
   }
}

It works only if the 2 classes are separate. Our basic usage is to have one utils class with some static helper methods. We use those public static methods across our code to determine specific conditions. In this case, the variable will never be null. What is interesting is that if you also move that utils static method into the same class and make it private the code will work without the FP. If it will be changed to public the sonar lint plugin will find a FP. I guess this has something to do with overriding of the public method. Since when I try this example and move the static method into the class A and make it just public final it will not find that FP.

Dňa streda, 6. júna 2018 15:51:00 UTC+2 Michael Gumowski napísal(-a):

[Michael Gumowski]

Hey,

Thanks a lot for the precisions. Okay, so now I see the issue... And unfortunately you won't be able to do a lot about it, except probably marking the issues as false positive, for the time being.

To give you a bit more details, a few of the SonarJava rules are based on our Symbolic Execution (SE) engine (for instance S2259, S4165 or S2095). This SE engine is building method summaries (similarly to what I shown in my last message) only for methods which are non-overridable (private, or final, or static), and within the file being analyzed. For any other method, we assume we learn nothing and let the engine continue with known constraints on variable (null, non-null, true, false, etc.). We talk here of Cross-Procedural Context-Sensitive Path-Sensitive Data Flow Analysis. 

Now, when a method is called from another part of the source file, the SE engine jumps to the method declaration, study the possible outcomes, and deduces constraints on parameters and return values. It then goes back to the caller, and continues with what it learns from the invocation. Implicitly, it means that the definition of the method should be available in the syntax tree to produces such summaries with an adequate precision (and lower as much as possible the FP rate).

We have been working intensively last year on extending the SE engine to be able to handle Cross-file (x-file) data flow analysis, and therefore compute method summaries when we don't have access to the code itself, but only to the bytecode. It however raises numerous questions (when do we stop following invocations? how deep are we exploring? what constraint should we keep or ignore? should we explore libraries? how to report in SQ/SL the path when code is not available? etc.) and the results we were getting from this are currently not satisfactory to be enabled in production (too many FPs for too few new relevant issues).

In your case, however, it would have solved the issue and make the FP disappear, because we would have computed correctly the method behavior, even if present in another file. It would have discarded the null-case, and discard the issue.

Today, SonarJava uses this x-file approach only for a very limited number of hard-coded methods, from well known libraries, on which we know this x-file analysis works. You can look at the full list here: java-frontend/src/main/java/org/sonar/java/se/xproc/BehaviorCache.java#L48 

This feature is unfortunately not mature enough to be pushed further for the time being, and we need time to address all the questions such approach is asking.

I can not tell you when we will go back to this subject, but I created the following (way too large) ticket to cover the case you are facing: SONARJAVA-2778

But be sure that we will come back to the topic at some point, but I can not give you a "when".

Regards,

Michael

--
Important: this SonarQube Google Group will close on June 11th, 2018, in order to move to a new forum to power even more community discussions. See details in this post: https://groups.google.com/d/msg/sonarqube/BbSZz-JnhVM/DavhMueEAAAJ
---

You received this message because you are subscribed to the Google Groups "SonarQube" group.
To unsubscribe from this group and stop receiving emails from it, send an email to sonarqube+...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/sonarqube/3244bb27-3574-4821-9766-356e3cc96aa5%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.