From 8abf76b618077f8eab9f372e52ad0212f710c89e Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Tue, 26 Feb 2019 14:48:16 +0100 Subject: [PATCH 1/3] C#: Reduce size of `getAThrownException()` In the precense of multiple core libraries, `getAThrownException()` would return multiple copies of the same exception, say `System.OverflowException`, one for each core library. With this change we try to identify which core library a given control flow element was compiled against, and only return the corresponding version. --- .../csharp/controlflow/ControlFlowElement.qll | 5 +++ .../controlflow/internal/Completion.qll | 42 ++++++++++++++++++- 2 files changed, 46 insertions(+), 1 deletion(-) diff --git a/csharp/ql/src/semmle/code/csharp/controlflow/ControlFlowElement.qll b/csharp/ql/src/semmle/code/csharp/controlflow/ControlFlowElement.qll index e5d65e3b549d..a85f3c2cb4c2 100644 --- a/csharp/ql/src/semmle/code/csharp/controlflow/ControlFlowElement.qll +++ b/csharp/ql/src/semmle/code/csharp/controlflow/ControlFlowElement.qll @@ -19,6 +19,11 @@ class ControlFlowElement extends ExprOrStmtParent, @control_flow_element { /** Gets the enclosing callable of this element, if any. */ Callable getEnclosingCallable() { none() } + /** Gets the assembly that this element was compiled into. */ + Assembly getAssembly() { + result = this.getEnclosingCallable().getDeclaringType().getALocation() + } + /** * Gets a control flow node for this element. That is, a node in the * control flow graph that corresponds to this element. diff --git a/csharp/ql/src/semmle/code/csharp/controlflow/internal/Completion.qll b/csharp/ql/src/semmle/code/csharp/controlflow/internal/Completion.qll index 56edfd9c4f2b..b6d8c8860112 100644 --- a/csharp/ql/src/semmle/code/csharp/controlflow/internal/Completion.qll +++ b/csharp/ql/src/semmle/code/csharp/controlflow/internal/Completion.qll @@ -185,6 +185,35 @@ private class Overflowable extends UnaryOperation { } } +private class SystemType extends ValueOrRefType { + SystemType() { this.getNamespace() instanceof SystemNamespace } +} + +private class SystemTypeMention extends TypeMention { + SystemTypeMention() { this.getType() instanceof SystemType } + + Assembly getSystemAssembly() { result = this.getType().getALocation() } + + Assembly getAssembly() { result = this.getTarget().(ControlFlowElement).getAssembly() } +} + +/** + * Holds if assembly `a` was definitely compiled with core library `core`. + * + * The analysis is conservative, as it requires mentioning of a (non-special) + * core type inside assembly `a`. + */ +pragma[noinline] +private predicate assemblyCompiledWithCoreLib(Assembly a, Assembly core) { + exists(SystemTypeMention stm | a = stm.getAssembly() | + core = stm.getSystemAssembly() and + // Special built-in types like `object` and `int` are collapsed into one entity + // in the presence of multiple core libraries, so such entities cannot be used + // to determine the actual core library used at compilation + strictcount(stm.getSystemAssembly()) = 1 + ) +} + /** A control flow element that is inside a `try` block. */ private class TriedControlFlowElement extends ControlFlowElement { TriedControlFlowElement() { this = any(TryStmt try).getATriedElement() } @@ -192,7 +221,7 @@ private class TriedControlFlowElement extends ControlFlowElement { /** * Gets an exception class that is potentially thrown by this element, if any. */ - Class getAThrownException() { + private Class getAThrownException0() { this instanceof Overflowable and result instanceof SystemOverflowExceptionClass or @@ -249,6 +278,17 @@ private class TriedControlFlowElement extends ControlFlowElement { this instanceof StringLiteral and result instanceof SystemOutOfMemoryExceptionClass } + + private Assembly getCoreLib() { assemblyCompiledWithCoreLib(this.getAssembly(), result) } + + Class getAThrownException() { + result = this.getAThrownException0() and + ( + not exists(this.getCoreLib()) + or + this.getCoreLib() = result.getALocation() + ) + } } pragma[noinline] From b2ede5e2a10b2b16a6a2017b57b7d30f40af22fb Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Tue, 5 Mar 2019 14:57:46 +0100 Subject: [PATCH 2/3] Revise logic for reducing size of `getAThrownException()` When determining which core library a "tried control flow element" is compiled against, first look at exceptions caught by the surrounding `try` block, then look at assembly attributes, and finally choose (randomly) the core library with the highest lexicographic order. --- .../controlflow/internal/Completion.qll | 56 +++++++++++-------- .../semmle/code/csharp/frameworks/System.qll | 5 ++ 2 files changed, 37 insertions(+), 24 deletions(-) diff --git a/csharp/ql/src/semmle/code/csharp/controlflow/internal/Completion.qll b/csharp/ql/src/semmle/code/csharp/controlflow/internal/Completion.qll index b6d8c8860112..902e91db935e 100644 --- a/csharp/ql/src/semmle/code/csharp/controlflow/internal/Completion.qll +++ b/csharp/ql/src/semmle/code/csharp/controlflow/internal/Completion.qll @@ -185,38 +185,23 @@ private class Overflowable extends UnaryOperation { } } -private class SystemType extends ValueOrRefType { - SystemType() { this.getNamespace() instanceof SystemNamespace } -} - -private class SystemTypeMention extends TypeMention { - SystemTypeMention() { this.getType() instanceof SystemType } - - Assembly getSystemAssembly() { result = this.getType().getALocation() } - - Assembly getAssembly() { result = this.getTarget().(ControlFlowElement).getAssembly() } +private class CoreLib extends Assembly { + CoreLib() { this = any(SystemExceptionClass c).getALocation() } } /** * Holds if assembly `a` was definitely compiled with core library `core`. - * - * The analysis is conservative, as it requires mentioning of a (non-special) - * core type inside assembly `a`. */ pragma[noinline] -private predicate assemblyCompiledWithCoreLib(Assembly a, Assembly core) { - exists(SystemTypeMention stm | a = stm.getAssembly() | - core = stm.getSystemAssembly() and - // Special built-in types like `object` and `int` are collapsed into one entity - // in the presence of multiple core libraries, so such entities cannot be used - // to determine the actual core library used at compilation - strictcount(stm.getSystemAssembly()) = 1 - ) +private predicate assemblyCompiledWithCoreLib(Assembly a, CoreLib core) { + a.getAnAttribute().getType().getABaseType*().getALocation() = core } /** A control flow element that is inside a `try` block. */ private class TriedControlFlowElement extends ControlFlowElement { - TriedControlFlowElement() { this = any(TryStmt try).getATriedElement() } + TryStmt try; + + TriedControlFlowElement() { this = try.getATriedElement() } /** * Gets an exception class that is potentially thrown by this element, if any. @@ -279,16 +264,39 @@ private class TriedControlFlowElement extends ControlFlowElement { result instanceof SystemOutOfMemoryExceptionClass } - private Assembly getCoreLib() { assemblyCompiledWithCoreLib(this.getAssembly(), result) } + private CoreLib getCoreLibFromACatchClause() { + exists(SpecificCatchClause scc | scc = try.getACatchClause() | + result = scc.getCaughtExceptionType().getABaseType*().getALocation() + ) + } + + private CoreLib getCoreLib() { + result = this.getCoreLibFromACatchClause() + or + not exists(this.getCoreLibFromACatchClause()) and + assemblyCompiledWithCoreLib(this.getAssembly(), result) + } - Class getAThrownException() { + pragma[noinline] + private Class getAThrownExceptionFromPlausibleCoreLib(string name) { result = this.getAThrownException0() and + name = result.getQualifiedName() and ( not exists(this.getCoreLib()) or this.getCoreLib() = result.getALocation() ) } + + Class getAThrownException() { + exists(string name | + result = this.getAThrownExceptionFromPlausibleCoreLib(name) | + result = min(Class c | + c = this.getAThrownExceptionFromPlausibleCoreLib(name) | + c order by c.getLocation().(Assembly).getFullName() + ) + ) + } } pragma[noinline] diff --git a/csharp/ql/src/semmle/code/csharp/frameworks/System.qll b/csharp/ql/src/semmle/code/csharp/frameworks/System.qll index 24dd7f971a9d..76fabd949cdb 100644 --- a/csharp/ql/src/semmle/code/csharp/frameworks/System.qll +++ b/csharp/ql/src/semmle/code/csharp/frameworks/System.qll @@ -56,6 +56,11 @@ class SystemArrayClass extends SystemClass { SystemArrayClass() { this.hasName("Array") } } +/** `System.Attribute` class. */ +class SystemAttributeClass extends SystemClass { + SystemAttributeClass() { this.hasName("Attribute") } +} + /** The `System.Boolean` structure. */ class SystemBooleanStruct extends BoolType { /** Gets the `Parse(string)` method. */ From 0afb85cb915b45ef6250f3f72ceebd634da2773e Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Tue, 5 Mar 2019 19:27:36 +0100 Subject: [PATCH 3/3] C#: Address review comments --- .../csharp/controlflow/internal/Completion.qll | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/csharp/ql/src/semmle/code/csharp/controlflow/internal/Completion.qll b/csharp/ql/src/semmle/code/csharp/controlflow/internal/Completion.qll index 902e91db935e..631b89633c3d 100644 --- a/csharp/ql/src/semmle/code/csharp/controlflow/internal/Completion.qll +++ b/csharp/ql/src/semmle/code/csharp/controlflow/internal/Completion.qll @@ -194,7 +194,7 @@ private class CoreLib extends Assembly { */ pragma[noinline] private predicate assemblyCompiledWithCoreLib(Assembly a, CoreLib core) { - a.getAnAttribute().getType().getABaseType*().getALocation() = core + a.getAnAttribute().getType().getBaseClass*().(SystemAttributeClass).getALocation() = core } /** A control flow element that is inside a `try` block. */ @@ -266,7 +266,7 @@ private class TriedControlFlowElement extends ControlFlowElement { private CoreLib getCoreLibFromACatchClause() { exists(SpecificCatchClause scc | scc = try.getACatchClause() | - result = scc.getCaughtExceptionType().getABaseType*().getALocation() + result = scc.getCaughtExceptionType().getBaseClass*().(SystemExceptionClass).getALocation() ) } @@ -289,12 +289,12 @@ private class TriedControlFlowElement extends ControlFlowElement { } Class getAThrownException() { - exists(string name | - result = this.getAThrownExceptionFromPlausibleCoreLib(name) | + exists(string name | result = this.getAThrownExceptionFromPlausibleCoreLib(name) | result = min(Class c | - c = this.getAThrownExceptionFromPlausibleCoreLib(name) | - c order by c.getLocation().(Assembly).getFullName() - ) + c = this.getAThrownExceptionFromPlausibleCoreLib(name) + | + c order by c.getLocation().(Assembly).getFullName() + ) ) } }