From e663abd5da850a81d05e81d488e3f51366700840 Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Wed, 6 Feb 2019 14:08:30 +0100 Subject: [PATCH] C#: Avoid using `ExceptionClass` in deliberate Cartesian products Using the class `ExceptionClass` in combination with a deliberate Cartesian product can lead to bad join orders, for example ``` EVALUATE NONRECURSIVE RELATION: Completion::TriedControlFlowElement::getAThrownException_dispred#ff(int this, int result) :- {1} r1 = JOIN Expr::Expr::getType_dispred#ff_10#join_rhs WITH @integral_type#f ON Expr::Expr::getType_dispred#ff_10#join_rhs.<0>=@integral_type#f.<0> OUTPUT FIELDS {Expr::Expr::getType_dispred#ff_10#join_rhs.<1>} {1} r2 = JOIN r1 WITH @un_op#f ON r1.<0>=@un_op#f.<0> OUTPUT FIELDS {r1.<0>} {1} r3 = JOIN r2 WITH Stmt::TryStmt::getATriedElement#ff_1#join_rhs ON r2.<0>=Stmt::TryStmt::getATriedElement#ff_1#join_rhs.<0> OUTPUT FIELDS {r2.<0>} {2} r4 = JOIN r3 WITH Stmt::ExceptionClass#f CARTESIAN PRODUCT OUTPUT FIELDS {Stmt::ExceptionClass#f.<0>,r3.<0>} {2} r5 = JOIN r4 WITH System::SystemOverflowExceptionClass#class#f ON r4.<0>=System::SystemOverflowExceptionClass#class#f.<0> OUTPUT FIELDS {r4.<1>,r4.<0>} ``` where the CP is made with `ExceptionClass` rather than `SystemOverflowExceptionClass` directly. --- .../API Abuse/DisposeNotCalledOnException.ql | 2 +- .../semmle/code/csharp/commons/Assertions.qll | 6 ++-- .../controlflow/internal/Completion.qll | 28 ++++++++++--------- .../ql/src/semmle/code/csharp/exprs/Expr.qll | 2 +- .../csharp/frameworks/test/VisualStudio.qll | 2 +- 5 files changed, 21 insertions(+), 19 deletions(-) diff --git a/csharp/ql/src/API Abuse/DisposeNotCalledOnException.ql b/csharp/ql/src/API Abuse/DisposeNotCalledOnException.ql index 4789179fc6d..adabc9edc14 100644 --- a/csharp/ql/src/API Abuse/DisposeNotCalledOnException.ql +++ b/csharp/ql/src/API Abuse/DisposeNotCalledOnException.ql @@ -23,7 +23,7 @@ import semmle.code.csharp.frameworks.System * Gets an exception type that may be thrown during the execution of method `m`. * Assumes any exception may be thrown by library types. */ -ExceptionClass getAThrownException(Method m) { +Class getAThrownException(Method m) { m.fromLibrary() and result = any(SystemExceptionClass sc) or diff --git a/csharp/ql/src/semmle/code/csharp/commons/Assertions.qll b/csharp/ql/src/semmle/code/csharp/commons/Assertions.qll index 16f5671676c..e982d8acbd9 100644 --- a/csharp/ql/src/semmle/code/csharp/commons/Assertions.qll +++ b/csharp/ql/src/semmle/code/csharp/commons/Assertions.qll @@ -15,7 +15,7 @@ abstract class AssertMethod extends Method { final Parameter getAssertedParameter() { result = this.getParameter(this.getAssertionIndex()) } /** Gets the exception being thrown if the assertion fails, if any. */ - abstract ExceptionClass getExceptionClass(); + abstract Class getExceptionClass(); } /** A positive assertion method. */ @@ -122,7 +122,7 @@ class SystemDiagnosticsDebugAssertTrueMethod extends AssertTrueMethod { override int getAssertionIndex() { result = 0 } - override ExceptionClass getExceptionClass() { + override Class getExceptionClass() { // A failing assertion generates a message box, see // https://docs.microsoft.com/en-us/dotnet/api/system.diagnostics.debug.assert none() @@ -182,7 +182,7 @@ class ForwarderAssertMethod extends AssertMethod { override int getAssertionIndex() { result = p.getPosition() } - override ExceptionClass getExceptionClass() { + override Class getExceptionClass() { result = this.getUnderlyingAssertMethod().getExceptionClass() } 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 ce41d25aeb5..769bca5ad2a 100644 --- a/csharp/ql/src/semmle/code/csharp/controlflow/internal/Completion.qll +++ b/csharp/ql/src/semmle/code/csharp/controlflow/internal/Completion.qll @@ -178,6 +178,13 @@ private predicate isMatchingConstant(Expr e, boolean value) { ) } +private class Overflowable extends UnaryOperation { + Overflowable() { + not this instanceof UnaryBitwiseOperation and + this.getType() instanceof IntegralType + } +} + /** A control flow element that is inside a `try` block. */ private class TriedControlFlowElement extends ControlFlowElement { TriedControlFlowElement() { this = any(TryStmt try).getATriedElement() } @@ -185,20 +192,15 @@ private class TriedControlFlowElement extends ControlFlowElement { /** * Gets an exception class that is potentially thrown by this element, if any. */ - ExceptionClass getAThrownException() { - this = any(UnaryOperation uo | - not uo instanceof UnaryBitwiseOperation and - uo.getType() instanceof IntegralType and - result instanceof SystemOverflowExceptionClass - ) + Class getAThrownException() { + this instanceof Overflowable and + result instanceof SystemOverflowExceptionClass or - this = any(CastExpr ce | - ce.getType() instanceof IntegralType and - result instanceof SystemOverflowExceptionClass - or - invalidCastCandidate(ce) and - result instanceof SystemInvalidCastExceptionClass - ) + this.(CastExpr).getType() instanceof IntegralType and + result instanceof SystemOverflowExceptionClass + or + invalidCastCandidate(this) and + result instanceof SystemInvalidCastExceptionClass or this instanceof Call and result instanceof SystemExceptionClass diff --git a/csharp/ql/src/semmle/code/csharp/exprs/Expr.qll b/csharp/ql/src/semmle/code/csharp/exprs/Expr.qll index 25a4bf5761f..0f7cc9f65d5 100644 --- a/csharp/ql/src/semmle/code/csharp/exprs/Expr.qll +++ b/csharp/ql/src/semmle/code/csharp/exprs/Expr.qll @@ -558,7 +558,7 @@ class ThrowElement extends ControlFlowElement, DotNet::Throw, @throw_element { override Expr getExpr() { result = this.getChild(0) } /** Gets the type of exception being thrown. */ - ExceptionClass getThrownExceptionType() { + Class getThrownExceptionType() { result = getExpr().getType() or // Corner case: `throw null` diff --git a/csharp/ql/src/semmle/code/csharp/frameworks/test/VisualStudio.qll b/csharp/ql/src/semmle/code/csharp/frameworks/test/VisualStudio.qll index 901bf3a6428..16f05cd7834 100644 --- a/csharp/ql/src/semmle/code/csharp/frameworks/test/VisualStudio.qll +++ b/csharp/ql/src/semmle/code/csharp/frameworks/test/VisualStudio.qll @@ -89,7 +89,7 @@ class VSTestAssertClass extends Class { } /** The `Microsoft.VisualStudio.TestTools.UnitTesting.AssertFailedException` class. */ -class AssertFailedExceptionClass extends ExceptionClass { +class AssertFailedExceptionClass extends Class { AssertFailedExceptionClass() { this.getNamespace() instanceof VSTestNamespace and this.hasName("AssertFailedException")