Merge pull request #593 from hvitved/csharp/nullness

C#: Rewrite nullness queries

Author: calumgrant

change-notes/1.20/analysis-csharp.md

@@ -9,8 +9,11 @@
 
 ## Changes to existing queries
 
-| *@name of query (Query ID)*| *Impact on results*    | *How/why the query has changed*                                  |
+| *@name of query (Query ID)*  | *Impact on results*    | *How/why the query has changed*   |
+|------------------------------|------------------------|-----------------------------------|
 | Off-by-one comparison against container length (cs/index-out-of-bounds) | Fewer false positives | Results have been removed when there are additional guards on the index. |
+| Dereferenced variable is always null (cs/dereferenced-value-is-always-null) | Improved results | The query has been rewritten from scratch, and the analysis is now based on static single assignment (SSA) forms. The query is now enabled by default in LGTM. |
+| Dereferenced variable may be null (cs/dereferenced-value-may-be-null) | Improved results | The query has been rewritten from scratch, and the analysis is now based on static single assignment (SSA) forms. The query is now enabled by default in LGTM. |
 
 ## Changes to code extraction
 

csharp/ql/src/CSI/NullAlways.qhelp

@@ -2,13 +2,39 @@
   "-//Semmle//qhelp//EN"
   "qhelp.dtd">
 <qhelp>
+
+
 <overview>
-<p>If a variable is dereferenced, and the variable has a null value on all possible execution paths 
-leading to the dereferencing, it is guaranteed to result in a <code>NullReferenceException</code>.
+<p>If a variable is dereferenced, for example as the qualifier in a method call, and the
+variable has a <code>null</code> value on all possible execution paths leading to the
+dereferencing, the dereferencing is guaranteed to result in a <code>NullReferenceException</code>.
 </p>
 
 </overview>
 <recommendation>
-<p>Examine the code to check for possible errors.</p>
+
+<p>Ensure that the variable does not have a <code>null</code> value when it is dereferenced.
+</p>
+
 </recommendation>
+<example>
+<p>
+In the following examples, the condition <code>s.Length > 0</code> is only
+executed if <code>s</code> is <code>null</code>.
+</p>
+
+<sample src="NullAlwaysBad.cs" />
+
+<p>
+In the revised example, the condition is guarded correctly by using <code>&amp;&amp;</code> instead of
+<code>||</code>.
+</p>
+
+<sample src="NullAlwaysGood.cs" />
+</example>
+<references>
+
+  <li>Microsoft, <a href="https://docs.microsoft.com/en-us/dotnet/api/system.nullreferenceexception">NullReferenceException Class</a>.</li>
+
+</references>
 </qhelp>

csharp/ql/src/CSI/NullAlways.ql

@@ -1,9 +1,9 @@
 /**
  * @name Dereferenced variable is always null
- * @description Finds uses of a variable that may cause a NullPointerException
+ * @description Dereferencing a variable whose value is 'null' causes a 'NullReferenceException'.
  * @kind problem
  * @problem.severity error
- * @precision medium
+ * @precision very-high
  * @id cs/dereferenced-value-is-always-null
  * @tags reliability
  *       correctness
@@ -14,6 +14,6 @@
 import csharp
 import semmle.code.csharp.dataflow.Nullness
 
-from VariableAccess access, LocalVariable var
-where access = unguardedNullDereference(var)
-select access, "Variable $@ is always null here.", var, var.getName()
+from Dereference d, Ssa::SourceVariable v
+where d.isFirstAlwaysNull(v)
+select d, "Variable $@ is always null here.", v, v.toString()

csharp/ql/src/CSI/NullAlwaysBad.cs

@@ -0,0 +1,13 @@
+using System;
+
+namespace NullAlways
+{
+    class Bad
+    {
+        void DoPrint(string s)
+        {
+            if (s != null || s.Length > 0)
+                Console.WriteLine(s);
+        }
+    }
+}

csharp/ql/src/CSI/NullAlwaysGood.cs

@@ -0,0 +1,13 @@
+using System;
+
+namespace NullAlways
+{
+    class Good
+    {
+        void DoPrint(string s)
+        {
+            if (s != null && s.Length > 0)
+                Console.WriteLine(s);
+        }
+    }
+}

csharp/ql/src/CSI/NullMaybe.qhelp

@@ -2,13 +2,41 @@
   "-//Semmle//qhelp//EN"
   "qhelp.dtd">
 <qhelp>
+
+
 <overview>
-<p>If a variable is dereferenced, and the variable may have a null value on some execution paths 
-leading to the dereferencing, the dereferencing may result in a <code>NullReferenceException</code>.
+<p>If a variable is dereferenced, for example as the qualifier in a method call, and the
+variable may have a <code>null</code> value on some execution paths leading to the
+dereferencing, the dereferencing may result in a <code>NullReferenceException</code>.
 </p>
 
 </overview>
 <recommendation>
-<p>Examine the code to check for possible errors.</p>
+
+<p>Ensure that the variable does not have a <code>null</code> value when it is dereferenced.
+</p>
+
 </recommendation>
+<example>
+<p>
+In the following example, the method <code>DoPrint()</code> dereferences its parameter
+<code>o</code> unconditionally, resulting in a <code>NullReferenceException</code> via
+the call <code>DoPrint(null)</code>.
+</p>
+
+<sample src="NullMaybeBad.cs" />
+
+<p>
+In the revised example, the method <code>DoPrint()</code> guards the dereferencing with
+a <code>null</code> check.
+</p>
+
+<sample src="NullMaybeGood.cs" />
+
+</example>
+<references>
+
+  <li>Microsoft, <a href="https://docs.microsoft.com/en-us/dotnet/api/system.nullreferenceexception">NullReferenceException Class</a>.</li>
+
+</references>
 </qhelp>

csharp/ql/src/CSI/NullMaybe.ql

@@ -1,9 +1,10 @@
 /**
  * @name Dereferenced variable may be null
- * @description Finds uses of a variable that may cause a NullPointerException
+ * @description Dereferencing a variable whose value may be 'null' may cause a
+ *              'NullReferenceException'.
  * @kind problem
  * @problem.severity warning
- * @precision medium
+ * @precision high
  * @id cs/dereferenced-value-may-be-null
  * @tags reliability
  *       correctness
@@ -14,8 +15,6 @@
 import csharp
 import semmle.code.csharp.dataflow.Nullness
 
-from VariableAccess access, LocalVariable var
-where access = unguardedMaybeNullDereference(var)
-// do not flag definite nulls here; these are already flagged by NullAlways.ql
-and not access = unguardedNullDereference(var)
-select access, "Variable $@ may be null here.", var, var.getName()
+from Dereference d, Ssa::SourceVariable v, string msg, Element reason
+where d.isFirstMaybeNull(v.getAnSsaDefinition(), msg, reason)
+select d, "Variable $@ may be null here " + msg + ".", v, v.toString(), reason, "this"

csharp/ql/src/CSI/NullMaybeBad.cs

@@ -0,0 +1,15 @@
+using System;
+
+class Bad
+{
+    void DoPrint(object o)
+    {
+        Console.WriteLine(o.ToString());
+    }
+
+    void M()
+    {
+        DoPrint("Hello");
+        DoPrint(null);
+    }
+}

csharp/ql/src/CSI/NullMaybeGood.cs

@@ -0,0 +1,16 @@
+using System;
+
+class Good
+{
+    void DoPrint(object o)
+    {
+        if (o != null)
+            Console.WriteLine(o.ToString());
+    }
+
+    void M()
+    {
+        DoPrint("Hello");
+        DoPrint(null);
+    }
+}

csharp/ql/src/semmle/code/csharp/Stmt.qll

@@ -966,12 +966,17 @@ class TryStmt extends Stmt, @try_stmt {
     exists(ControlFlowElement mid |
       mid = getATriedElement() and
       not mid instanceof TryStmt and
-      result = mid.getAChild() and
-      mid.getEnclosingCallable() = result.getEnclosingCallable()
+      result = getAChild(mid, mid.getEnclosingCallable())
     )
   }
 }
 
+pragma[noinline]
+private ControlFlowElement getAChild(ControlFlowElement cfe, Callable c) {
+  result = cfe.getAChild() and
+  c = result.getEnclosingCallable()
+}
+
 /**
  * A `catch` clause within a `try` statement.
  *