[SonarLint][S2259][C#] False Positive - SonarLint is trying to solve the halting problem

Rules and Languages Report False-positive / False-negative...
,
1
  • SonarLint 5.1.0.39724, installed as extensions to Visual Studio 2019, Scanning C#-code.
      public void DemoProblem()
      {
         ConnectionStringSettings chosenConnection = AskUserToPickConnection();

         if (null == chosenConnection) ShutDownHard(0);

         ConnectUsingSettings(chosenConnection.ConnectionString); // reports S2259 'chosenConnection' is null on at least one execution path.
      }

ShutDownHard does an Environment.Exit() with the given exitCode, so the problem line is never reached if chosenConnection is null.

SonarLint can’t really know this without analyzing ReallyShutDown, which in turn is equivalent to
the halting problem.

What is extra vexing is that the S2259 disappears if I remove the null check.
I.e. it doesn’t warn if the code is actually vulnerable.

1 Like
2

Looked around and found these topics about the same rule, but different pieces of code.

As well as this blog post about False Positives, where it is apparent the problem is very well understood.

Aggregating here for future reference. Both for test cases if/when the rule is up for adjustment, and for other users to see what has happened before, and the official policy on false positives.

1 Like
3

Hello @JJungner and welcome to our community!

I can confirm this as a false positive, however for the time being we don’t support cross method analysis. Because of that our analyzer does not know what ShutDownHard(0) does.

Unfortunately the only thing for now you can do is to resolve the issue as a false positive.

All the best,
Čaba

4

Thank you for your reply.

I read the blog post referred to earlier, so I am aware of the official policy.

However, I’ve also been thinking a bit about it since.

The code contains a statement on the form

      if (<variable> == null) <do something>

This is an obvious null check. Yet the rule triggers.

And when I remove the null check, the rule doesn’t trigger. I.e. the rule somehow assumes that since there is no null check, the variable can’t be null.

So it’s worse than a false positive. It’s a flipped result.

The logical conclusion from seeing the null check there, should be that the code handles nulls in that particular variable.

Not the other way around.

Especially if you don’t bother parsing the <do something>-part

(In my not so humble opinion.)

5

@JJungner I understand that at the first look it might look like a flipped result but let me explain you how it works trough an example and you will see it is not.

First example:

if(variable == null)
  Console.WriteLine("Variable is null");
variable.ToString();  // Noncompliant

As you can see in this example in case the variable is null we just do some logging. After the if block the ToString call will cause some trouble in the form of a NullReferenceException.

Second example:

if(variable == null)
  variable = Sanitize();
variable.ToString();

In this example in case the variable is null, it will be assigned a new value. Now after this point we are not sure anymore if the variable has a null value or not, so no issue will be raised for the variable.ToString() invocation.

For your code snippet the case is that we don’t support cross method analysis and our engine does not understand what will ShutDownHard(0) do, hence the false positive.

And to understand why the issue is only triggered in case there is a null check…
Our engine does no know what AskUserToPickConnection method returns. It might be null but it might be the case that it does not return null in any case. In order to avoid noise and a lot of false positives, we assume it does not have the null value. Once the null check is done we now that the value can be null for at least one execution path, so the rule raises an issue.

2 Likes
6

May be staying the obvious, but:

 public void DemoProblem()
      {
         ConnectionStringSettings chosenConnection = AskUserToPickConnection();

         if (chosenConnection is null) { ShutDownHard(0); }
         else { ConnectUsingSettings(chosenConnection.ConnectionString); }
      }

will not longer raise the issue. It also helps understanding the code: without having to know what ShutDownHard() does, it is now clear, that the second part only is executed if the chosenConnection is not null.

2 Likes
7

Thanks, @Corniel . I did change the code similarly to your suggestion.

I was initially upset by SonarLint trying to solve the halting problem.
Then I was upset because they assumed that a ‘check for null’ meant ‘not handled’.
Then I went back and changed the code.

I still think ‘check for null’ = ‘not handled’ is beyond stupid, but I have moved on.

Will mark your code as the answer.

8

This topic was automatically closed 7 days after the last reply. New replies are no longer allowed.