From 0e0ab1ea972ddb555a9b31ce8264eab117fbf56e Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Wed, 26 Sep 2018 09:15:20 +0200 Subject: [PATCH 1/3] C++: make `unresolve` a member of ElementBase Also remove the charpred of ElementBase. This gets rid of many redundant charpred checks. It means that incomplete classes from the db are now `Element`s, which is maybe noisy but should not be harmful. Together, these changes give a great reduction in DIL and should help the optimiser. It brings the DIL of `UncontrolledFormatString.ql` down from 43,908 lines to 35,400 lines. --- cpp/ql/src/semmle/code/cpp/Class.qll | 7 ++- cpp/ql/src/semmle/code/cpp/Element.qll | 54 +++++++++++-------- cpp/ql/src/semmle/code/cpp/Specifier.qll | 4 +- .../src/semmle/code/cpp/pointsto/PointsTo.qll | 16 +++++- 4 files changed, 56 insertions(+), 25 deletions(-) diff --git a/cpp/ql/src/semmle/code/cpp/Class.qll b/cpp/ql/src/semmle/code/cpp/Class.qll index 8d4414f03f38..7ebaf8c7df0f 100644 --- a/cpp/ql/src/semmle/code/cpp/Class.qll +++ b/cpp/ql/src/semmle/code/cpp/Class.qll @@ -12,7 +12,12 @@ private import semmle.code.cpp.internal.ResolveClass */ class Class extends UserType { Class() { - isClass(underlyingElement(this)) + isClass(this.underlying()) and + this = resolveClass(_) + } + + override @element unresolve() { + resolveClass(result) = this } /** Gets a child declaration of this class. */ diff --git a/cpp/ql/src/semmle/code/cpp/Element.qll b/cpp/ql/src/semmle/code/cpp/Element.qll index 7231d11aec9d..13f554960093 100644 --- a/cpp/ql/src/semmle/code/cpp/Element.qll +++ b/cpp/ql/src/semmle/code/cpp/Element.qll @@ -2,63 +2,75 @@ import semmle.code.cpp.Location private import semmle.code.cpp.Enclosing private import semmle.code.cpp.internal.ResolveClass -/** - * Get the `@element` that represents this `@element`. - * Normally this will simply be `e`, but sometimes it is not. - * For example, for an incomplete struct `e` the result may be a - * complete struct with the same name. - */ -private cached @element resolveElement(@element e) { - if isClass(e) - then result = resolveClass(e) - else result = e -} - /** * Get the `Element` that represents this `@element`. * Normally this will simply be a cast of `e`, but sometimes it is not. * For example, for an incomplete struct `e` the result may be a * complete struct with the same name. */ +pragma[inline] Element mkElement(@element e) { - result = resolveElement(e) + result.unresolve() = e } /** - * Get an `@element` that resolves to the `Element`. This should + * INTERNAL: Do not use. + * + * Gets an `@element` that resolves to the `Element`. This should * normally only be called from member predicates, where `e` is not * `this` and you need the result for an argument to a database * extensional. * See `underlyingElement` for when `e` is `this`. */ +pragma[inline] @element unresolveElement(Element e) { - resolveElement(result) = e + result = e.unresolve() } /** - * Get the `@element` that this `Element` extends. This should normally + * INTERNAL: Do not use. + * + * Gets the `@element` that this `Element` extends. This should normally * only be called from member predicates, where `e` is `this` and you * need the result for an argument to a database extensional. * See `unresolveElement` for when `e` is not `this`. */ +pragma[inline] @element underlyingElement(Element e) { result = e } /** - * A C/C++ element with no member predicates other than `toString`. Not for + * A C/C++ element with a minimal set of member predicates. Not for * general use. This class does not define a location, so classes wanting to * change their location without affecting other classes can extend * `ElementBase` instead of `Element` to create a new rootdef for `getURL`, * `getLocation`, or `hasLocationInfo`. */ class ElementBase extends @element { - ElementBase() { - this = resolveElement(_) - } - /** Gets a textual representation of this element. */ string toString() { none() } + + /** + * INTERNAL: Do not use. + * + * Gets the `@element` that this `Element` extends. This should normally only + * be called from member predicates on `this` where you need the result for + * an argument to a database extensional. + * See `unresolve` for when the qualifier is not `this`. + */ + final @element underlying() { result = this } + + /** + * INTERNAL: Do not use. + * + * Gets an `@element` that resolves to the `Element`. This should normally + * only be called from member predicates, where the qualifier is not `this` + * and you need the result for an argument to a database extensional. + * See `underlying` for when the qualifier is `this`. + */ + pragma[inline] + @element unresolve() { result = this } } /** diff --git a/cpp/ql/src/semmle/code/cpp/Specifier.qll b/cpp/ql/src/semmle/code/cpp/Specifier.qll index 373d8db57505..0bb9d0dd9b7d 100644 --- a/cpp/ql/src/semmle/code/cpp/Specifier.qll +++ b/cpp/ql/src/semmle/code/cpp/Specifier.qll @@ -294,13 +294,13 @@ class AttributeArgument extends Element, @attribute_arg { } override string toString() { - if exists (@attribute_arg_empty self | mkElement(self) = this) + if exists (@attribute_arg_empty self | self = underlyingElement(this)) then result = "empty argument" else exists (string prefix, string tail | (if exists(getName()) then prefix = getName() + "=" else prefix = "") and - (if exists (@attribute_arg_type self | mkElement(self) = this) + (if exists (@attribute_arg_type self | self = underlyingElement(this)) then tail = getValueType().getName() else tail = getValueText()) and result = prefix + tail) diff --git a/cpp/ql/src/semmle/code/cpp/pointsto/PointsTo.qll b/cpp/ql/src/semmle/code/cpp/pointsto/PointsTo.qll index bd4684cdfa42..7c70fd50d505 100644 --- a/cpp/ql/src/semmle/code/cpp/pointsto/PointsTo.qll +++ b/cpp/ql/src/semmle/code/cpp/pointsto/PointsTo.qll @@ -633,12 +633,26 @@ class PointsToExpr extends Expr pragma[noopt] Element pointsTo() { - this.interesting() and exists(int set, @element thisEntity, @element resultEntity | thisEntity = underlyingElement(this) and pointstosets(set, thisEntity) and setlocations(set, resultEntity) and resultEntity = unresolveElement(result)) + this.interesting() and + exists(int set, @element thisEntity, @element resultEntity | + thisEntity = this.underlying() and + pointstosets(set, thisEntity) and + setlocations(set, resultEntity) and + resultEntity = localUnresolveElement(result) + ) } float confidence() { result = 1.0 / count(this.pointsTo()) } } +/* + * This is used above in a `pragma[noopt]` context, which prevents its + * customary inlining. We materialise it explicitly here. + */ +private @element localUnresolveElement(Element e) { + result = unresolveElement(e) +} + /** * Holds if anything points to an element, that is, is equivalent to: * ``` From 6ccd208d4e0e74bb12759445f6a8525015e5d60b Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Wed, 26 Sep 2018 14:02:15 +0200 Subject: [PATCH 2/3] C++: Prevent incomplete classes from being Types Raw classes from the database that are incomplete and should be represented by their complete twin are now allowed to be `Element`s for performance reasons, but this commit prevents them from being `Type`s. It was causing confusion in test results and might also cause confusion in queries. --- cpp/ql/src/semmle/code/cpp/Class.qll | 3 +-- cpp/ql/src/semmle/code/cpp/Type.qll | 2 ++ cpp/ql/src/semmle/code/cpp/internal/ResolveClass.qll | 6 ++++++ 3 files changed, 9 insertions(+), 2 deletions(-) diff --git a/cpp/ql/src/semmle/code/cpp/Class.qll b/cpp/ql/src/semmle/code/cpp/Class.qll index 7ebaf8c7df0f..e2364744b145 100644 --- a/cpp/ql/src/semmle/code/cpp/Class.qll +++ b/cpp/ql/src/semmle/code/cpp/Class.qll @@ -12,8 +12,7 @@ private import semmle.code.cpp.internal.ResolveClass */ class Class extends UserType { Class() { - isClass(this.underlying()) and - this = resolveClass(_) + isClass(this.underlying()) } override @element unresolve() { diff --git a/cpp/ql/src/semmle/code/cpp/Type.qll b/cpp/ql/src/semmle/code/cpp/Type.qll index d34f2e4916f0..3a78e6c7cf94 100644 --- a/cpp/ql/src/semmle/code/cpp/Type.qll +++ b/cpp/ql/src/semmle/code/cpp/Type.qll @@ -7,6 +7,8 @@ private import semmle.code.cpp.internal.ResolveClass * A C/C++ type. */ class Type extends Locatable, @type { + Type() { isType(this.underlying()) } + /** * Gets the name of this type. */ diff --git a/cpp/ql/src/semmle/code/cpp/internal/ResolveClass.qll b/cpp/ql/src/semmle/code/cpp/internal/ResolveClass.qll index eaefc8196cc8..5f9c0006435e 100644 --- a/cpp/ql/src/semmle/code/cpp/internal/ResolveClass.qll +++ b/cpp/ql/src/semmle/code/cpp/internal/ResolveClass.qll @@ -54,4 +54,10 @@ cached private module Cached { (usertypes(t,_,1) or usertypes(t,_,2) or usertypes(t,_,3) or usertypes(t,_,6) or usertypes(t,_,10) or usertypes(t,_,11) or usertypes(t,_,12)) } + + cached predicate isType(@type t) { + not isClass(t) + or + t = resolveClass(_) + } } From 3b2512fa0d12949d40a80c394195ac34cd20ad5b Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Wed, 26 Sep 2018 14:39:44 +0200 Subject: [PATCH 3/3] C++: pragma[nomagic] in Overflow.qll These two predicates were supposed to be fast but became slow after the recent inlining of `unresolve`. --- cpp/ql/src/semmle/code/cpp/security/Overflow.qll | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cpp/ql/src/semmle/code/cpp/security/Overflow.qll b/cpp/ql/src/semmle/code/cpp/security/Overflow.qll index 5e3a6cc8b80f..3a5b72c1d389 100644 --- a/cpp/ql/src/semmle/code/cpp/security/Overflow.qll +++ b/cpp/ql/src/semmle/code/cpp/security/Overflow.qll @@ -13,6 +13,7 @@ predicate guardedAbs(Operation e, Expr use) { } /** is the size of this use guarded to be less than something? */ +pragma[nomagic] predicate guardedLesser(Operation e, Expr use) { exists(IfStmt c, RelationalOperation guard | use = guard.getLesserOperand().getAChild*() and @@ -33,6 +34,7 @@ predicate guardedLesser(Operation e, Expr use) { } /** is the size of this use guarded to be greater than something? */ +pragma[nomagic] predicate guardedGreater(Operation e, Expr use) { exists(IfStmt c, RelationalOperation guard | use = guard.getGreaterOperand().getAChild*() and