Rule java:S2259: False-positive NullPointerException bug logged when variable is null-checked by an imported method

Product: SonarCloud
Rule: java:S2259

Hello,

Per this post in another thread, here is a segment of reproducible code:

        String test = null;
        checkNotNull(test, "This imported method will throw an exception because string `test` is null, hence the next line will not evaluate.");
        char unused = test.charAt(0);

We have noted that the issue does not arise if the imported method’s definition is used directly; the issue is due to SonarCloud not properly handling the outcome using the imported method definition.

        String test2 = null;
        if (test2 == null)
        {
            throw new PermissibleArgumentException(String.valueOf("This will throw an exception because string `test` is null"));
        }
        char unused2 = test2.charAt(0);

The imported method is a helper/convenience function and does what it is intended to do.

Should SonarCloud be able to determine that there is no NullPointerException risk in the first reproducible code sample (without the addition of a comment to tell SonarCloud to avoid adding a bug for this line of code)?

Thank you!

Best,
Christina

Note that the checkNotNull method definition is in the 2nd code segment, also here explicitly:

    public static <T> T checkNotNull(@Nullable T reference, @Nullable String errorMessage)
    {
        if (reference == null)
        {
            throw new PermissibleArgumentException(String.valueOf(errorMessage));
        }
        return reference;
    }

Using Sonarqube 9.4

Another example:

    HttpEntity<T> httpRequest = new HttpEntity<>(request, HTTP_HEADERS);
    ResponseEntity<Foo> response = restTemplate.postForEntity(serverUrl + "/odata/folders", httpRequest, Foo.class);

    if (!response.hasBody() || response.getStatusCode() != HttpStatus.CREATED) {
      throw new ReportingException("bla");
    }
    return response.getBody().toEntity();

Sonarqube complains about the response.getBody() call might return null, but response.hasBody() already does the null check before.

Hello @christina

Answer greatly inspired by this other post, with up-to-date information.

Just a few words about the rule now. Rule S2259 is based on a Symbolic Execution (SE) engine. This engine is validating execution paths and checking for non-null values or null-check along the way, exploring all the possibilities. Unfortunately, its actual state also has some limitations, like the one you are hitting here.

As of today, the SE engine is able to explore non-overridable methods (static, for instance), when declared in the same file being analyzed. When exploring such methods, the engine then deduces behaviors regarding null-checking (among other things). The SonarJava SE engine is, however, by default, not configured to explore methods declared in other files (in your case, I suspect that checkNotNull is defined in another file).

In addition to methods declared in the same file, we also support “behavior” of well-known methods. As of today, we support the methods from the classes listed here. For example, the following code is not raising an issue:

if (Objects.nonNull(arg)) {
   arg.doSomething(); // No issue here, arg is considered as not null
}

If you are using your own helper method, it is unfortunately not possible to define “custom method behavior” or anything like that. For the time being, I would recommend to mark as False Positive the issue.

Best,
Quentin

The second example is not exactly related to the problem described in my previous post, but is also due to a limitation of the engine: it is not able to detect that a call on an object has consequences on the result of another call.

To go a bit further into the implementation details, getBody() is annotated with org.springframework.lang.Nullable, annotation that we consider as Strong Nullable (An element which nullability can not be statically determined, it should always be checked for null). It is therefore fair to report an issue for this code… A Weak Nullable annotation would make more sense here. It seems to be another consequence of the inconsistent usage of nullability annotations in Java ecoystem.

All in all, the outcome is the same: I would recommend to mark as False Positive the issue.

Hello Quentin,

Thank you for your help here. We face exactly the same problem in my team : we use a method from a third party library that checks for NULL just like the Christina’s example.

It’s a mess for us because we have a lot of false-positive because of that.
Can we make something on our side to reduce those false-positive ?
Maybe extend the “behavior” of well-known methods ?

Thank you !
Regards