Skip to content

Commit d5a4dce

Browse files
calumgrantcalumgrant
committed
C#: Fix bug in dataflow library.
1 parent d76a980 commit d5a4dce

File tree

15 files changed

+356
-69
lines changed

15 files changed

+356
-69
lines changed

‎csharp/ql/src/semmle/code/csharp/controlflow/internal/NonReturning.qll‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,8 @@ private class ThrowingCall extends NonReturningCall {
4444
exists(CIL::Method m, CIL::Type ex |
4545
this.getTarget().matchesHandle(m) and
4646
alwaysThrowsException(m, ex) and
47-
c.getExceptionClass().matchesHandle(ex)
47+
c.getExceptionClass().matchesHandle(ex) and
48+
not m.isVirtual()
4849
)
4950
}
5051

‎csharp/ql/src/semmle/code/csharp/dataflow/DataFlow.qll‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -646,7 +646,7 @@ module DataFlow {
646646
sourceDecl.matchesHandle(result.(Callable))
647647
or
648648
// CIL callable without C# implementation in the database
649-
not sourceDecl.matchesHandle(any(Callable k)) and
649+
not sourceDecl.matchesHandle(any(Callable k | k.hasBody())) and
650650
result = sourceDecl
651651
else
652652
// C# callable without C# implementation in the database
@@ -1188,7 +1188,7 @@ module DataFlow {
11881188
* nodes that may potentially be reached in flow from some source to some
11891189
* sink.
11901190
*/
1191-
private module Pruning {
1191+
module Pruning {
11921192
/**
11931193
* Holds if `node` is reachable from a source in the configuration `config`,
11941194
* ignoring call contexts.

‎csharp/ql/src/semmle/code/csharp/dataflow/internal/CallableReturns.qll‎

Lines changed: 33 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -7,35 +7,53 @@ private import cil
77
private import semmle.code.csharp.dataflow.Nullness
88
private import semmle.code.cil.CallableReturns as CR
99

10+
private predicate finalCallable(Callable c) {
11+
not c.(Virtualizable).isVirtual() and
12+
not exists(DeclarationWithGetSetAccessors p | c = p.getAnAccessor() and p.isVirtual())
13+
}
14+
1015
/** Holds if callable `c` always returns null. */
1116
predicate alwaysNullCallable(Callable c) {
12-
exists(CIL::Method m | m.matchesHandle(c) | CR::alwaysNullMethod(m))
13-
or
14-
forex(Expr e | c.canReturn(e) | e instanceof AlwaysNullExpr)
17+
finalCallable(c) and
18+
(
19+
exists(CIL::Method m | m.matchesHandle(c) | CR::alwaysNullMethod(m))
20+
or
21+
forex(Expr e | c.canReturn(e) | e instanceof AlwaysNullExpr)
22+
)
1523
}
1624

1725
/** Holds if callable `c` always returns a non-null value. */
1826
predicate alwaysNotNullCallable(Callable c) {
19-
exists(CIL::Method m | m.matchesHandle(c) | CR::alwaysNotNullMethod(m))
20-
or
21-
forex(Expr e | c.canReturn(e) | e instanceof NonNullExpr)
27+
finalCallable(c) and
28+
(
29+
exists(CIL::Method m | m.matchesHandle(c) | CR::alwaysNotNullMethod(m))
30+
or
31+
forex(Expr e | c.canReturn(e) | e instanceof NonNullExpr)
32+
)
2233
}
2334

2435
/** Holds if callable 'c' always throws an exception. */
2536
predicate alwaysThrowsCallable(Callable c) {
26-
forex(ControlFlow::Node pre | pre = c.getExitPoint().getAPredecessor() |
27-
pre.getElement() instanceof ThrowElement
37+
finalCallable(c) and
38+
(
39+
forex(ControlFlow::Node pre | pre = c.getExitPoint().getAPredecessor() |
40+
pre.getElement() instanceof ThrowElement
41+
)
42+
or
43+
exists(CIL::Method m | m.matchesHandle(c) | CR::alwaysThrowsMethod(m))
2844
)
29-
or
30-
exists(CIL::Method m | m.matchesHandle(c) | CR::alwaysThrowsMethod(m))
3145
}
3246

3347
/** Holds if callable `c` always throws exception `ex`. */
3448
predicate alwaysThrowsException(Callable c, Class ex) {
35-
forex(ControlFlow::Node pre | pre = c.getExitPoint().getAPredecessor() |
36-
pre.getElement().(ThrowElement).getThrownExceptionType() = ex
49+
finalCallable(c) and
50+
(
51+
forex(ControlFlow::Node pre | pre = c.getExitPoint().getAPredecessor() |
52+
pre.getElement().(ThrowElement).getThrownExceptionType() = ex
53+
)
54+
or
55+
exists(CIL::Method m, CIL::Type t | m.matchesHandle(c) |
56+
CR::alwaysThrowsException(m, t) and t.matchesHandle(ex)
57+
)
3758
)
38-
or
39-
exists(CIL::Method m, CIL::Type t | m.matchesHandle(c) | CR::alwaysThrowsException(m, t) and t.matchesHandle(ex))
4059
}
41-

‎csharp/ql/test/library-tests/cil/dataflow/CallableReturns.expected‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,11 @@
1+
stubs
12
alwaysNull
3+
| System.Object Dataflow.NullMethods.ReturnsNull2() | 0: ldarg.0, 1: call Dataflow.NullMethods.ReturnsNull, 2: ret |
4+
| System.Object Dataflow.NullMethods.ReturnsNull() | 0: ldnull, 1: ret |
5+
| System.Object Dataflow.NullMethods.ReturnsNullIndirect() | 0: ldarg.0, 1: call Dataflow.NullMethods.ReturnsNull, 2: ret |
6+
| System.Object Dataflow.NullMethods.VirtualReturnsNull() | 0: ldnull, 1: ret |
7+
| System.Object Dataflow.NullMethods.get_NullProperty() | 0: ldnull, 1: ret |
8+
| System.Object Dataflow.NullMethods.get_VirtualNullProperty() | 0: ldnull, 1: ret |
29
| System.Object System.Collections.EmptyReadOnlyDictionaryInternal.get_Item(System.Object) | 0: ldarg.1, 1: brtrue.s 6:, 2: ldstr "key", 3: call System.SR.get_ArgumentNull_Key, 4: newobj System.ArgumentNullException..ctor, 5: throw, 6: ldnull, 7: ret |
310
alwaysNonNull
411
| System.ArgumentException System.ThrowHelper.GetAddingDuplicateWithKeyArgumentException(System.Object) |
@@ -11,6 +18,12 @@ alwaysNonNull
1118
| System.ArgumentOutOfRangeException System.ThrowHelper.GetArgumentOutOfRangeException(System.ExceptionArgument,System.Int32,System.ExceptionResource) |
1219
| System.Exception System.ThrowHelper.GetArraySegmentCtorValidationFailedException(System.Array,System.Int32,System.Int32) |
1320
| System.InvalidOperationException System.ThrowHelper.GetInvalidOperationException(System.ExceptionResource) |
21+
| System.Object Dataflow.NonNullMethods.ReturnsNonNull2() |
22+
| System.Object Dataflow.NonNullMethods.ReturnsNonNull() |
23+
| System.Object Dataflow.NonNullMethods.ReturnsNonNullIndirect() |
24+
| System.Object Dataflow.NonNullMethods.get_VirtualNonNull() |
25+
| System.Object Dataflow.NonNullMethods.get_VirtualNonNullProperty() |
26+
| System.String Dataflow.NonNullMethods.get_NonNullProperty2() |
1427
| System.Text.Encoder System.Text.ASCIIEncoding.GetEncoder() |
1528
| System.Text.Encoder System.Text.Encoding.GetEncoder() |
1629
| System.Text.Encoder System.Text.EncodingNLS.GetEncoder() |
@@ -20,6 +33,10 @@ alwaysNonNull
2033
| System.Text.Encoder System.Text.UTF32Encoding.GetEncoder() |
2134
| System.Text.Encoder System.Text.UnicodeEncoding.GetEncoder() |
2235
alwaysThrows
36+
| System.Object Dataflow.ThrowingMethods.AlwaysThrows() | System.InvalidOperationException | 0: newobj System.InvalidOperationException..ctor, 1: throw |
37+
| System.Object Dataflow.ThrowingMethods.VirtualThrows() | System.Exception | 0: newobj System.Exception..ctor, 1: throw |
38+
| System.Object Dataflow.ThrowingMethods.get_ThrowProperty() | System.Exception | 0: newobj System.Exception..ctor, 1: throw |
39+
| System.Object Dataflow.ThrowingMethods.get_VirtualThrowProperty() | System.Exception | 0: newobj System.Exception..ctor, 1: throw |
2340
| System.Object System.ValueTuple.get_Item(System.Int32) | System.IndexOutOfRangeException | 0: newobj System.IndexOutOfRangeException..ctor, 1: throw |
2441
| System.Void System.ThrowHelper.ThrowAddingDuplicateWithKeyArgumentException(System.Object) | System.ArgumentException | 0: ldarg.0, 1: call System.ThrowHelper.GetAddingDuplicateWithKeyArgumentException, 2: throw |
2542
| System.Void System.ThrowHelper.ThrowAggregateException(System.Collections.Generic.List<System.Exception>) | System.AggregateException | 0: ldarg.0, 1: newobj System.AggregateException..ctor, 2: throw |

‎csharp/ql/test/library-tests/cil/dataflow/CallableReturns.ql‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,13 @@ predicate relevantMethod(CIL::Method m) {
77
m.getName() = "get_Item"
88
or
99
m.getDeclaringType().getName() = "ThrowHelper"
10+
or
11+
m.getLocation().(CIL::Assembly).getName() = "DataFlow"
12+
}
13+
14+
// Check that the assembly hasn't been marked as a stub.
15+
query predicate stubs(string str) {
16+
exists(CIL::Assembly asm | CIL::assemblyIsStub(asm) | str = asm.toString())
1017
}
1118

1219
query predicate alwaysNull(string s, string d) {

‎csharp/ql/test/library-tests/cil/dataflow/ControlFlow.expected‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,2 @@
1-
| dataflow.cs:41:9:41:18 | call to method DeadCode |
2-
| dataflow.cs:49:9:49:18 | call to method DeadCode |
1+
| dataflow.cs:57:9:57:18 | call to method DeadCode |
2+
| dataflow.cs:65:9:65:18 | call to method DeadCode |

‎csharp/ql/test/library-tests/cil/dataflow/DataFlow.cs_‎

Lines changed: 176 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,176 @@
1+
using System;
2+
3+
namespace Dataflow
4+
{
5+
public class NullMethods
6+
{
7+
public object ReturnsNull() => null;
8+
9+
public object ReturnsNull2()
10+
{
11+
var x = ReturnsNull();
12+
return x;
13+
}
14+
15+
// Does not necessarily return null because of virtual method call.
16+
public object NotReturnsNull() => VirtualReturnsNull();
17+
18+
public object ReturnsNullIndirect() => ReturnsNull();
19+
20+
public virtual object VirtualReturnsNull() => null;
21+
22+
public object NullProperty { get => null; }
23+
24+
public virtual object VirtualNullProperty { get => null; }
25+
}
26+
27+
public class NonNullMethods
28+
{
29+
public object ReturnsNonNull() => new object();
30+
31+
public object ReturnsNonNull2()
32+
{
33+
var x = ReturnsNonNull();
34+
return x;
35+
}
36+
37+
public object ReturnsNonNullIndirect() => ReturnsNonNull();
38+
39+
public object NonNullProperty { get => 1; }
40+
41+
public string NonNullProperty2 { get => "not null"; }
42+
43+
public virtual object VirtualNonNull { get => "not null"; }
44+
45+
public bool cond = false;
46+
47+
public string MaybeNull()
48+
{
49+
if (cond)
50+
return null;
51+
else
52+
return "not null";
53+
}
54+
55+
public string MaybeNull2()
56+
{
57+
return cond ? null : "not null";
58+
}
59+
60+
public virtual object VirtualNonNullProperty { get => "non null"; }
61+
}
62+
63+
public class ThrowingMethods
64+
{
65+
public static object AlwaysThrows() => throw new InvalidOperationException();
66+
67+
public object AlwaysThrowsIndirect() => AlwaysThrows();
68+
69+
public virtual object VirtualThrows() => throw new Exception();
70+
71+
public object ThrowProperty { get => throw new Exception(); }
72+
public virtual object VirtualThrowProperty { get => throw new Exception(); }
73+
74+
}
75+
76+
public class DataFlow
77+
{
78+
public object Taint1(object x) => x;
79+
80+
public object Taint2(object x) => Taint5(x);
81+
82+
public string Taint3(string s)
83+
{
84+
var x = s;
85+
Console.WriteLine(s);
86+
return x;
87+
}
88+
89+
public object Taint5(object x) => Taint6(x);
90+
91+
private object Taint6(object x) => x;
92+
}
93+
94+
public class TaintFlow
95+
{
96+
public string Taint1(string a, string b) => a + b;
97+
98+
public int Taint2(int a, int b) => a + b;
99+
100+
public int Taint3(int a) => -a;
101+
102+
public string TaintIndirect(string a, string b) => Taint1(a, b);
103+
}
104+
105+
public class Properties
106+
{
107+
public int TrivialProperty1 { get; set; }
108+
109+
int field;
110+
111+
public int TrivialProperty2
112+
{
113+
get => field;
114+
set { field = value; }
115+
}
116+
}
117+
118+
public class ThisAssemblyIsNotAStub
119+
{
120+
public void F()
121+
{
122+
// Ensure that the assembly isn't tagged as a stub
123+
// Need to bump the average instruction count.
124+
Console.WriteLine("This is not a stub assembly");
125+
Console.WriteLine("This is not a stub assembly");
126+
Console.WriteLine("This is not a stub assembly");
127+
Console.WriteLine("This is not a stub assembly");
128+
Console.WriteLine("This is not a stub assembly");
129+
Console.WriteLine("This is not a stub assembly");
130+
Console.WriteLine("This is not a stub assembly");
131+
Console.WriteLine("This is not a stub assembly");
132+
Console.WriteLine("This is not a stub assembly");
133+
Console.WriteLine("This is not a stub assembly");
134+
Console.WriteLine("This is not a stub assembly");
135+
Console.WriteLine("This is not a stub assembly");
136+
Console.WriteLine("This is not a stub assembly");
137+
Console.WriteLine("This is not a stub assembly");
138+
Console.WriteLine("This is not a stub assembly");
139+
Console.WriteLine("This is not a stub assembly");
140+
Console.WriteLine("This is not a stub assembly");
141+
Console.WriteLine("This is not a stub assembly");
142+
Console.WriteLine("This is not a stub assembly");
143+
Console.WriteLine("This is not a stub assembly");
144+
Console.WriteLine("This is not a stub assembly");
145+
Console.WriteLine("This is not a stub assembly");
146+
Console.WriteLine("This is not a stub assembly");
147+
Console.WriteLine("This is not a stub assembly");
148+
Console.WriteLine("This is not a stub assembly");
149+
Console.WriteLine("This is not a stub assembly");
150+
Console.WriteLine("This is not a stub assembly");
151+
Console.WriteLine("This is not a stub assembly");
152+
Console.WriteLine("This is not a stub assembly");
153+
Console.WriteLine("This is not a stub assembly");
154+
Console.WriteLine("This is not a stub assembly");
155+
Console.WriteLine("This is not a stub assembly");
156+
Console.WriteLine("This is not a stub assembly");
157+
Console.WriteLine("This is not a stub assembly");
158+
Console.WriteLine("This is not a stub assembly");
159+
Console.WriteLine("This is not a stub assembly");
160+
Console.WriteLine("This is not a stub assembly");
161+
Console.WriteLine("This is not a stub assembly");
162+
Console.WriteLine("This is not a stub assembly");
163+
Console.WriteLine("This is not a stub assembly");
164+
Console.WriteLine("This is not a stub assembly");
165+
Console.WriteLine("This is not a stub assembly");
166+
Console.WriteLine("This is not a stub assembly");
167+
Console.WriteLine("This is not a stub assembly");
168+
Console.WriteLine("This is not a stub assembly");
169+
Console.WriteLine("This is not a stub assembly");
170+
Console.WriteLine("This is not a stub assembly");
171+
Console.WriteLine("This is not a stub assembly");
172+
Console.WriteLine("This is not a stub assembly");
173+
Console.WriteLine("This is not a stub assembly");
174+
}
175+
}
176+
}

‎csharp/ql/test/library-tests/cil/dataflow/DataFlow.dll‎

6.5 KB
Binary file not shown.

‎csharp/ql/test/library-tests/cil/dataflow/DataFlow.expected‎

Lines changed: 17 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,17 @@
1-
| dataflow.cs:16:18:16:26 | "tainted" | dataflow.cs:16:18:16:37 | call to method ToString |
2-
| dataflow.cs:17:27:17:28 | 12 | dataflow.cs:17:18:17:29 | call to method Abs |
3-
| dataflow.cs:18:27:18:27 | 2 | dataflow.cs:18:18:18:31 | call to method Max |
4-
| dataflow.cs:18:30:18:30 | 3 | dataflow.cs:18:18:18:31 | call to method Max |
5-
| dataflow.cs:20:45:20:53 | "tainted" | dataflow.cs:20:18:20:54 | call to method GetFullPath |
6-
| dataflow.cs:27:44:27:46 | 1 | dataflow.cs:27:18:27:52 | call to method IEEERemainder |
7-
| dataflow.cs:27:49:27:51 | 2 | dataflow.cs:27:18:27:52 | call to method IEEERemainder |
8-
| dataflow.cs:64:30:64:33 | null | dataflow.cs:61:23:61:50 | ... ? ... : ... |
9-
| dataflow.cs:64:30:64:33 | null | dataflow.cs:70:20:70:33 | call to method IndirectNull |
10-
| dataflow.cs:71:23:71:26 | null | dataflow.cs:61:23:61:50 | ... ? ... : ... |
1+
| dataflow.cs:18:18:18:26 | "tainted" | dataflow.cs:18:18:18:37 | call to method ToString |
2+
| dataflow.cs:19:27:19:28 | 12 | dataflow.cs:19:18:19:29 | call to method Abs |
3+
| dataflow.cs:20:27:20:27 | 2 | dataflow.cs:20:18:20:31 | call to method Max |
4+
| dataflow.cs:20:30:20:30 | 3 | dataflow.cs:20:18:20:31 | call to method Max |
5+
| dataflow.cs:22:45:22:53 | "tainted" | dataflow.cs:22:18:22:54 | call to method GetFullPath |
6+
| dataflow.cs:29:44:29:46 | 1 | dataflow.cs:29:18:29:52 | call to method IEEERemainder |
7+
| dataflow.cs:29:49:29:51 | 2 | dataflow.cs:29:18:29:52 | call to method IEEERemainder |
8+
| dataflow.cs:40:34:40:37 | "d1" | dataflow.cs:40:18:40:38 | call to method Taint1 |
9+
| dataflow.cs:41:34:41:37 | "d2" | dataflow.cs:41:18:41:38 | call to method Taint2 |
10+
| dataflow.cs:42:34:42:37 | "d3" | dataflow.cs:42:18:42:38 | call to method Taint3 |
11+
| dataflow.cs:46:28:46:32 | "t1a" | dataflow.cs:46:18:46:40 | call to method Taint1 |
12+
| dataflow.cs:46:35:46:39 | "t1b" | dataflow.cs:46:18:46:40 | call to method Taint1 |
13+
| dataflow.cs:49:35:49:38 | "t6" | dataflow.cs:49:18:49:45 | call to method TaintIndirect |
14+
| dataflow.cs:49:41:49:44 | "t6" | dataflow.cs:49:18:49:45 | call to method TaintIndirect |
15+
| dataflow.cs:95:30:95:33 | null | dataflow.cs:89:24:89:51 | ... ? ... : ... |
16+
| dataflow.cs:95:30:95:33 | null | dataflow.cs:101:20:101:33 | call to method IndirectNull |
17+
| dataflow.cs:102:23:102:26 | null | dataflow.cs:89:24:89:51 | ... ? ... : ... |

‎csharp/ql/test/library-tests/cil/dataflow/DataFlow.ql‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import csharp
22
import semmle.code.csharp.dataflow.DataFlow::DataFlow
3-
3+
// import DataFlow::PathGraph
4+
45
class FlowConfig extends Configuration {
56
FlowConfig() { this = "FlowConfig" }
67

0 commit comments

Comments
 (0)