Skip to content

C#: Improve cs/dereference-* queries and add to the Code Quality suite. #19589

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ ql/csharp/ql/src/API Abuse/CallToGCCollect.ql
ql/csharp/ql/src/API Abuse/FormatInvalid.ql
ql/csharp/ql/src/API Abuse/NoDisposeCallOnLocalIDisposable.ql
ql/csharp/ql/src/Bad Practices/Control-Flow/ConstantCondition.ql
ql/csharp/ql/src/CSI/NullAlways.ql
ql/csharp/ql/src/CSI/NullMaybe.ql
ql/csharp/ql/src/Dead Code/DeadStoreOfLocal.ql
ql/csharp/ql/src/Language Abuse/MissedReadonlyOpportunity.ql
ql/csharp/ql/src/Likely Bugs/Collections/ContainerLengthCmpOffByOne.ql
Expand Down
9 changes: 7 additions & 2 deletions csharp/ql/lib/semmle/code/csharp/dataflow/Nullness.qll
Original file line number Diff line number Diff line change
Expand Up @@ -544,8 +544,13 @@ class Dereference extends G::DereferenceableExpr {
p.hasExtensionMethodModifier() and
not emc.isConditional()
|
p.fromSource() // assume all non-source extension methods perform a dereference
implies
// Assume all non-source extension methods on
// (1) nullable types are null-safe
// (2) non-nullable types are doing a dereference.
p.fromLibrary() and
not p.getAnnotatedType().isNullableRefType()
or
p.fromSource() and
exists(
Ssa::ImplicitParameterDefinition def,
AssignableDefinitions::ImplicitParameterDefinition pdef
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,7 @@ class SystemDiagnosticsDebugClass extends SystemDiagnosticsClass {
/** Gets an `Assert(bool, ...)` method. */
Method getAssertMethod() {
result.getDeclaringType() = this and
result.hasName("Assert") and
result.getParameter(0).getType() instanceof BoolType and
result.getReturnType() instanceof VoidType
result.hasName("Assert")
}
}

Expand Down
1 change: 1 addition & 0 deletions csharp/ql/src/CSI/NullAlways.ql
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
* correctness
* exceptions
* external/cwe/cwe-476
* quality
*/

import csharp
Expand Down
1 change: 1 addition & 0 deletions csharp/ql/src/CSI/NullMaybe.ql
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
* correctness
* exceptions
* external/cwe/cwe-476
* quality
*/

import csharp
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
category: minorAnalysis
---
* The queries `cs/dereferenced-value-is-always-null` and `cs/dereferenced-value-may-be-null` have been improved to reduce false positives. The queries no longer assume that expressions are dereferenced when passed as the receiver (`this` parameter) to extension methods where that parameter is a nullable type.
14 changes: 7 additions & 7 deletions csharp/ql/test/query-tests/Nullness/A.cs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ class A
public void Lock()
{
object synchronizedAlways = null;
lock (synchronizedAlways) // BAD (always)
lock (synchronizedAlways) // $ Alert[cs/dereferenced-value-is-always-null]
{
synchronizedAlways.GetHashCode(); // GOOD
}
Expand All @@ -14,7 +14,7 @@ public void Lock()
public void ArrayAssignTest()
{
int[] arrayNull = null;
arrayNull[0] = 10; // BAD (always)
arrayNull[0] = 10; // $ Alert[cs/dereferenced-value-is-always-null]

int[] arrayOk;
arrayOk = new int[10];
Expand All @@ -28,10 +28,10 @@ public void Access()
object methodAccess = null;
object methodCall = null;

Console.WriteLine(arrayAccess[1]); // BAD (always)
Console.WriteLine(fieldAccess.Length); // BAD (always)
Func<String> tmp = methodAccess.ToString; // BAD (always)
Console.WriteLine(methodCall.ToString()); // BAD (always)
Console.WriteLine(arrayAccess[1]); // $ Alert[cs/dereferenced-value-is-always-null]
Console.WriteLine(fieldAccess.Length); // $ Alert[cs/dereferenced-value-is-always-null]
Func<String> tmp = methodAccess.ToString; // $ Alert[cs/dereferenced-value-is-always-null]
Console.WriteLine(methodCall.ToString()); // $ Alert[cs/dereferenced-value-is-always-null]

Console.WriteLine(arrayAccess[1]); // GOOD
Console.WriteLine(fieldAccess.Length); // GOOD
Expand All @@ -47,7 +47,7 @@ public void OutOrRef()

object varRef = null;
TestMethod2(ref varRef);
varRef.ToString(); // BAD (always)
varRef.ToString(); // $ Alert[cs/dereferenced-value-is-always-null]

varRef = null;
TestMethod3(ref varRef);
Expand Down
10 changes: 5 additions & 5 deletions csharp/ql/test/query-tests/Nullness/Assert.cs
Original file line number Diff line number Diff line change
Expand Up @@ -12,23 +12,23 @@ void Fn(bool b)

s = b ? null : "";
Assert.IsNull(s);
Console.WriteLine(s.Length); // BAD (always)
Console.WriteLine(s.Length); // $ Alert[cs/dereferenced-value-is-always-null]

s = b ? null : "";
Assert.IsNotNull(s);
Console.WriteLine(s.Length); // GOOD

s = b ? null : "";
Assert.IsTrue(s == null);
Console.WriteLine(s.Length); // BAD (always)
Console.WriteLine(s.Length); // $ Alert[cs/dereferenced-value-is-always-null]

s = b ? null : "";
Assert.IsTrue(s != null);
Console.WriteLine(s.Length); // GOOD

s = b ? null : "";
Assert.IsFalse(s != null);
Console.WriteLine(s.Length); // BAD (always)
Console.WriteLine(s.Length); // $ Alert[cs/dereferenced-value-is-always-null]

s = b ? null : "";
Assert.IsFalse(s == null);
Expand All @@ -44,10 +44,10 @@ void Fn(bool b)

s = b ? null : "";
Assert.IsTrue(s == null && b);
Console.WriteLine(s.Length); // BAD (always)
Console.WriteLine(s.Length); // $ Alert[cs/dereferenced-value-is-always-null]

s = b ? null : "";
Assert.IsFalse(s != null || !b);
Console.WriteLine(s.Length); // BAD (always)
Console.WriteLine(s.Length); // $ Alert[cs/dereferenced-value-is-always-null]
}
}
4 changes: 2 additions & 2 deletions csharp/ql/test/query-tests/Nullness/B.cs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ public void OperatorCall()
B neqCallAlways = null;

if (eqCallAlways == null)
eqCallAlways.ToString(); // BAD (always)
eqCallAlways.ToString(); // $ Alert[cs/dereferenced-value-is-always-null]

if (b2 != null)
b2.ToString(); // GOOD
Expand All @@ -21,7 +21,7 @@ public void OperatorCall()

if (neqCallAlways != null) { }
else
neqCallAlways.ToString(); // BAD (always)
neqCallAlways.ToString(); // $ Alert[cs/dereferenced-value-is-always-null]
}

public static bool operator ==(B b1, B b2)
Expand Down
58 changes: 29 additions & 29 deletions csharp/ql/test/query-tests/Nullness/C.cs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ public void NotTest()

if (!(o != null))
{
o.GetHashCode(); // BAD (always)
o.GetHashCode(); // $ Alert[cs/dereferenced-value-is-always-null]
}
}

Expand All @@ -39,7 +39,7 @@ public void AssertTest()
{
var s = Maybe() ? null : "";
Debug.Assert(s == null);
s.ToString(); // BAD (always)
s.ToString(); // $ Alert[cs/dereferenced-value-is-always-null]

s = Maybe() ? null : "";
Debug.Assert(s != null);
Expand All @@ -50,22 +50,22 @@ public void AssertNullTest()
{
var o1 = new object();
AssertNull(o1);
o1.ToString(); // BAD (always) (false negative)
o1.ToString(); // $ MISSING: Alert[cs/dereferenced-value-is-always-null]

var o2 = Maybe() ? null : "";
Assert.IsNull(o2);
o2.ToString(); // BAD (always)
o2.ToString(); // $ Alert[cs/dereferenced-value-is-always-null]
}

public void AssertNotNullTest()
{
var o1 = Maybe() ? null : new object();
var o1 = Maybe() ? null : new object(); // $ Source[cs/dereferenced-value-may-be-null]
AssertNonNull(o1);
o1.ToString(); // GOOD (false positive)
o1.ToString(); // $ SPURIOUS (false positive): Alert[cs/dereferenced-value-may-be-null]

var o2 = Maybe() ? null : new object();
var o2 = Maybe() ? null : new object(); // $ Source[cs/dereferenced-value-may-be-null]
AssertNonNull(o1);
o2.ToString(); // BAD (maybe)
o2.ToString(); // $ Alert[cs/dereferenced-value-may-be-null]

var o3 = Maybe() ? null : new object();
Assert.IsNotNull(o3);
Expand All @@ -91,16 +91,16 @@ public void InstanceOf()

public void Lock()
{
var o = Maybe() ? null : new object();
lock (o) // BAD (maybe)
var o = Maybe() ? null : new object(); // $ Source[cs/dereferenced-value-may-be-null]
lock (o) // $ Alert[cs/dereferenced-value-may-be-null]
o.ToString(); // GOOD
}

public void Foreach(IEnumerable<int> list)
{
if (Maybe())
list = null;
foreach (var x in list) // BAD (maybe)
list = null; // $ Source[cs/dereferenced-value-may-be-null]
foreach (var x in list) // $ Alert[cs/dereferenced-value-may-be-null]
{
x.ToString(); // GOOD
list.ToString(); // GOOD
Expand Down Expand Up @@ -159,23 +159,23 @@ public void DoWhile()
s = null;
do
{
s.ToString(); // BAD (always)
s.ToString(); // $ Alert[cs/dereferenced-value-is-always-null]
s = null;
}
while (s != null);

s = null;
do
{
s.ToString(); // BAD (always)
s.ToString(); // $ Alert[cs/dereferenced-value-is-always-null]
}
while (s != null);

s = "";
do
{
s.ToString(); // BAD (maybe)
s = null;
s.ToString(); // $ Alert[cs/dereferenced-value-may-be-null]
s = null; // $ Source[cs/dereferenced-value-may-be-null]
}
while (true);
}
Expand All @@ -193,15 +193,15 @@ public void While()
s = null;
while (b)
{
s.ToString(); // BAD (always)
s.ToString(); // $ Alert[cs/dereferenced-value-is-always-null]
s = null;
}

s = "";
while (true)
{
s.ToString(); // BAD (maybe)
s = null;
s.ToString(); // $ Alert[cs/dereferenced-value-may-be-null]
s = null; // $ Source[cs/dereferenced-value-may-be-null]
}
}

Expand All @@ -215,12 +215,12 @@ public void If()
}

if (s == null)
s.ToString(); // BAD (always)
s.ToString(); // $ Alert[cs/dereferenced-value-is-always-null]

s = "";
if (s != null && s.Length % 2 == 0)
s = null;
s.ToString(); // BAD (maybe)
s = null; // $ Source[cs/dereferenced-value-may-be-null]
s.ToString(); // $ Alert[cs/dereferenced-value-may-be-null]
}

public void For()
Expand All @@ -230,23 +230,23 @@ public void For()
{
s.ToString(); // GOOD
}
s.ToString(); // BAD (always)
s.ToString(); // $ Alert[cs/dereferenced-value-is-always-null]

for (s = null; s == null; s = null)
{
s.ToString(); // BAD (always)
s.ToString(); // $ Alert[cs/dereferenced-value-is-always-null]
}

for (s = ""; ; s = null)
for (s = ""; ; s = null) // $ Source[cs/dereferenced-value-may-be-null]
{
s.ToString(); // BAD (maybe)
s.ToString(); // $ Alert[cs/dereferenced-value-may-be-null]
}
}

public void ArrayAssignTest()
{
int[] a = null;
a[0] = 10; // BAD (always)
a[0] = 10; // $ Alert[cs/dereferenced-value-is-always-null]

a = new int[10];
a[0] = 42; // GOOD
Expand All @@ -257,8 +257,8 @@ public void Access()
int[] ia = null;
string[] sa = null;

ia[1] = 0; // BAD (always)
var temp = sa.Length; // BAD (always)
ia[1] = 0; // $ Alert[cs/dereferenced-value-is-always-null]
var temp = sa.Length; // $ Alert[cs/dereferenced-value-is-always-null]

ia[1] = 0; // BAD (always), but not first
temp = sa.Length; // BAD (always), but not first
Expand Down
Loading