diff --git a/change-notes/1.20/analysis-csharp.md b/change-notes/1.20/analysis-csharp.md index bc6a3cf35251..29383dade134 100644 --- a/change-notes/1.20/analysis-csharp.md +++ b/change-notes/1.20/analysis-csharp.md @@ -17,6 +17,7 @@ ## Changes to code extraction +* Fix extraction of `for` statements where the condition declares new variables using `is`. * Initializers of `stackalloc` arrays are now extracted. ## Changes to QL libraries diff --git a/change-notes/1.20/analysis-javascript.md b/change-notes/1.20/analysis-javascript.md index af8bb984ae8a..ddc519a440fb 100644 --- a/change-notes/1.20/analysis-javascript.md +++ b/change-notes/1.20/analysis-javascript.md @@ -2,7 +2,7 @@ ## General improvements -* Support for popular libraries has been improved. Consequently, queries may produce more results on code bases that use the following features: +* Support for popular libraries has been improved. Consequently, queries may produce better results on code bases that use the following features: - client-side code, for example [React](https://reactjs.org/) - cookies and webstorage, for example [js-cookie](https://github.com/js-cookie/js-cookie) - server-side code, for example [hapi](https://hapijs.com/) @@ -18,15 +18,18 @@ | Incomplete regular expression for hostnames (`js/incomplete-hostname-regexp`) | correctness, security, external/cwe/cwe-020 | Highlights hostname sanitizers that are likely to be incomplete, indicating a violation of [CWE-020](https://cwe.mitre.org/data/definitions/20.html). Results are shown on LGTM by default.| | Incomplete URL substring sanitization | correctness, security, external/cwe/cwe-020 | Highlights URL sanitizers that are likely to be incomplete, indicating a violation of [CWE-020](https://cwe.mitre.org/data/definitions/20.html). Results shown on LGTM by default. | | Incorrect suffix check (`js/incorrect-suffix-check`) | correctness, security, external/cwe/cwe-020 | Highlights error-prone suffix checks based on `indexOf`, indicating a potential violation of [CWE-20](https://cwe.mitre.org/data/definitions/20.html). Results are shown on LGTM by default. | +| Loop iteration skipped due to shifting (`js/loop-iteration-skipped-due-to-shifting`) | correctness | Highlights code that removes an element from an array while iterating over it, causing the loop to skip over some elements. Results are shown on LGTM by default. | | Useless comparison test (`js/useless-comparison-test`) | correctness | Highlights code that is unreachable due to a numeric comparison that is always true or always false. Results are shown on LGTM by default. | ## Changes to existing queries | **Query** | **Expected impact** | **Change** | |--------------------------------------------|------------------------------|------------------------------------------------------------------------------| -| Client-side cross-site scripting | More results | This rule now recognizes WinJS functions that are vulnerable to HTML injection. | +| Client-side cross-site scripting | More true-positive results, fewer false-positive results. | This rule now recognizes WinJS functions that are vulnerable to HTML injection, and no longer flags certain safe uses of jQuery. | | Insecure randomness | More results | This rule now flags insecure uses of `crypto.pseudoRandomBytes`. | +| Uncontrolled data used in network request | More results | This rule now recognizes host values that are vulnerable to injection. | | Unused parameter | Fewer false-positive results | This rule no longer flags parameters with leading underscore. | | Unused variable, import, function or class | Fewer false-positive results | This rule now flags fewer variables that are implictly used by JSX elements, and no longer flags variables with leading underscore. | +| Uncontrolled data used in path expression | Fewer false-positive results | This rule now recognizes the Express `root` option, which prevents path traversal. | ## Changes to QL libraries diff --git a/cpp/ql/src/semmle/code/cpp/Macro.qll b/cpp/ql/src/semmle/code/cpp/Macro.qll index 2c72dbe1cb95..9a1c29edbc6a 100644 --- a/cpp/ql/src/semmle/code/cpp/Macro.qll +++ b/cpp/ql/src/semmle/code/cpp/Macro.qll @@ -55,10 +55,18 @@ class Macro extends PreprocessorDirective, @ppd_define { } /** - * A macro access (macro expansion or other macro access). + * A macro access. For example: + * ``` + * #ifdef MACRO1 // this line contains a MacroAccess + * int x = MACRO2; // this line contains a MacroAccess + * #endif + * ``` + * + * See also `MacroInvocation`, which represents only macro accesses + * that are expanded (such as in the second line of the example above). */ class MacroAccess extends Locatable, @macroinvocation { - /** Gets the macro being invoked. */ + /** Gets the macro that is being accessed. */ Macro getMacro() { macroinvocations(underlyingElement(this),unresolveElement(result),_,_) } /** @@ -73,7 +81,7 @@ class MacroAccess extends Locatable, @macroinvocation { } /** - * Gets the location of this macro invocation. For a nested invocation, where + * Gets the location of this macro access. For a nested access, where * `exists(this.getParentInvocation())`, this yields a location either inside * a `#define` directive or inside an argument to another macro. */ @@ -126,14 +134,22 @@ class MacroAccess extends Locatable, @macroinvocation { override string toString() { result = this.getMacro().getHead() } - /** Gets the name of the invoked macro. */ + /** Gets the name of the accessed macro. */ string getMacroName() { result = getMacro().getName() } } /** - * A macro invocation (macro expansion). + * A macro invocation (macro access that is expanded). For example: + * ``` + * #ifdef MACRO1 + * int x = MACRO2; // this line contains a MacroInvocation + * #endif + * ``` + * + * See also `MacroAccess`, which also represents macro accesses where the macro + * is checked but not expanded (such as in the first line of the example above). */ class MacroInvocation extends MacroAccess { MacroInvocation() { @@ -174,7 +190,7 @@ class MacroInvocation extends MacroAccess { /** * Gets the top-level expression associated with this macro invocation, * if any. Note that this predicate will fail if the top-level expanded - * element is a statement rather than an expression. + * element is not an expression (for example if it is a statement). */ Expr getExpr() { result = getAnExpandedElement() and @@ -185,7 +201,7 @@ class MacroInvocation extends MacroAccess { /** * Gets the top-level statement associated with this macro invocation, if * any. Note that this predicate will fail if the top-level expanded - * element is an expression rather than a statement. + * element is not a statement (for example if it is an expression). */ Stmt getStmt() { result = getAnExpandedElement() and diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Accessor.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Accessor.cs index b70ab6ce0862..05339d66cdeb 100644 --- a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Accessor.cs +++ b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Accessor.cs @@ -58,7 +58,7 @@ public override void Populate() } else { - Context.ModelError(symbol, "Undhandled accessor kind"); + Context.ModelError(symbol, "Unhandled accessor kind"); return; } diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Constructor.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Constructor.cs index d96db4b46246..420364d13a19 100644 --- a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Constructor.cs +++ b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Constructor.cs @@ -118,7 +118,7 @@ public override IId Id if (symbol.IsStatic) tb.Append("static"); tb.Append(ContainingType); AddParametersToId(Context, tb, symbol); - tb.Append("; constructor"); + tb.Append(";constructor"); }); } } diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Event.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Event.cs index 86a22c704a7f..05630eb54a12 100644 --- a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Event.cs +++ b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Event.cs @@ -30,12 +30,13 @@ public override void Populate() Context.Emit(Tuples.events(this, symbol.GetName(), ContainingType, type.TypeRef, Create(Context, symbol.OriginalDefinition))); var adder = symbol.AddMethod; - if (adder != null) - EventAccessor.Create(Context, adder); - var remover = symbol.RemoveMethod; - if (remover != null) - EventAccessor.Create(Context, remover); + + if (!(adder is null)) + Method.Create(Context, adder); + + if (!(remover is null)) + Method.Create(Context, remover); ExtractModifiers(); BindComments(); diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Expression.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Expression.cs index c050d4d83e95..d51f438fbcdb 100644 --- a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Expression.cs +++ b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Expression.cs @@ -117,17 +117,19 @@ public static ExprKind UnaryOperatorKind(Context cx, ExprKind originalKind, Expr public void OperatorCall(ExpressionSyntax node) { var @operator = cx.GetSymbolInfo(node); - var method = @operator.Symbol as IMethodSymbol; - - if (GetCallType(cx, node) == CallType.Dynamic) + if (@operator.Symbol is IMethodSymbol method) { - UserOperator.OperatorSymbol(method.Name, out string operatorName); - cx.Emit(Tuples.dynamic_member_name(this, operatorName)); - return; - } - if (method != null) + var callType = GetCallType(cx, node); + if (callType == CallType.Dynamic) + { + UserOperator.OperatorSymbol(method.Name, out string operatorName); + cx.Emit(Tuples.dynamic_member_name(this, operatorName)); + return; + } + cx.Emit(Tuples.expr_call(this, Method.Create(cx, method))); + } } public enum CallType @@ -148,12 +150,9 @@ public static CallType GetCallType(Context cx, ExpressionSyntax node) { var @operator = cx.GetSymbolInfo(node); - if (@operator.Symbol != null) + if (@operator.Symbol is IMethodSymbol method) { - var method = @operator.Symbol as IMethodSymbol; - - var containingSymbol = method.ContainingSymbol as ITypeSymbol; - if (containingSymbol != null && containingSymbol.TypeKind == Microsoft.CodeAnalysis.TypeKind.Dynamic) + if (method.ContainingSymbol is ITypeSymbol containingSymbol && containingSymbol.TypeKind == Microsoft.CodeAnalysis.TypeKind.Dynamic) { return CallType.Dynamic; } diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Expressions/Query.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Expressions/Query.cs index d5c8ead52fd1..18a016d07096 100644 --- a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Expressions/Query.cs +++ b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Expressions/Query.cs @@ -33,58 +33,37 @@ public QueryCall(Context cx, IMethodSymbol method, SyntaxNode clause, IExpressio /// /// Represents a chain of method calls (the operand being recursive). /// - class ClauseCall + abstract class Clause { - public ClauseCall operand; - public IMethodSymbol method; - public readonly List arguments = new List(); - public SyntaxNode node; - public ISymbol declaration; - public SyntaxToken name; - public ISymbol intoDeclaration; + protected readonly IMethodSymbol method; + protected readonly List arguments = new List(); + protected readonly SyntaxNode node; - public ExpressionSyntax Expr => arguments.First(); - - public ClauseCall WithClause(IMethodSymbol newMethod, SyntaxNode newNode, SyntaxToken newName = default(SyntaxToken), ISymbol newDeclaration = null) + protected Clause(IMethodSymbol method, SyntaxNode node) { - return new ClauseCall - { - operand = this, - method = newMethod, - node = newNode, - name = newName, - declaration = newDeclaration - }; + this.method = method; + this.node = node; } - public ClauseCall AddArgument(ExpressionSyntax arg) - { - if (arg != null) - arguments.Add(arg); - return this; - } + public ExpressionSyntax Expr => arguments.First(); - public ClauseCall WithInto(ISymbol into) - { - intoDeclaration = into; - return this; - } + public CallClause WithCallClause(IMethodSymbol newMethod, SyntaxNode newNode) => + new CallClause(this, newMethod, newNode); - Expression DeclareRangeVariable(Context cx, IExpressionParentEntity parent, int child, bool getElement) - { - return DeclareRangeVariable(cx, parent, child, getElement, declaration); - } + public LetClause WithLetClause(IMethodSymbol newMethod, SyntaxNode newNode, ISymbol newDeclaration, SyntaxToken newName) => + new LetClause(this, newMethod, newNode, newDeclaration, newName); - void DeclareIntoVariable(Context cx, IExpressionParentEntity parent, int intoChild, bool getElement) + public Clause AddArgument(ExpressionSyntax arg) { - if (intoDeclaration != null) - DeclareRangeVariable(cx, parent, intoChild, getElement, intoDeclaration); + if (arg != null) + arguments.Add(arg); + return this; } - Expression DeclareRangeVariable(Context cx, IExpressionParentEntity parent, int child, bool getElement, ISymbol variableSymbol) + protected Expression DeclareRangeVariable(Context cx, IExpressionParentEntity parent, int child, bool getElement, ISymbol variableSymbol, SyntaxToken name) { var type = Type.Create(cx, cx.GetType(Expr)); - Semmle.Extraction.Entities.Location nameLoc; + Extraction.Entities.Location nameLoc; Type declType; if (getElement) @@ -115,7 +94,7 @@ Expression DeclareRangeVariable(Context cx, IExpressionParentEntity parent, int return decl; } - void PopulateArguments(Context cx, QueryCall callExpr, int child) + protected void PopulateArguments(Context cx, QueryCall callExpr, int child) { foreach (var e in arguments) { @@ -123,34 +102,79 @@ void PopulateArguments(Context cx, QueryCall callExpr, int child) } } - public Expression Populate(Context cx, IExpressionParentEntity parent, int child) + public abstract Expression Populate(Context cx, IExpressionParentEntity parent, int child); + } + + class RangeClause : Clause + { + readonly ISymbol declaration; + readonly SyntaxToken name; + + public RangeClause(IMethodSymbol method, SyntaxNode node, ISymbol declaration, SyntaxToken name) : base(method, node) { - if (declaration != null) // The first "from" clause, or a "let" clause - { - if (operand == null) - { - return DeclareRangeVariable(cx, parent, child, true); - } - else - { - if (method == null) - cx.ModelError(node, "Unable to determine target of query expression"); - - var callExpr = new QueryCall(cx, method, node, parent, child); - operand.Populate(cx, callExpr, 0); - DeclareRangeVariable(cx, callExpr, 1, false); - PopulateArguments(cx, callExpr, 2); - DeclareIntoVariable(cx, callExpr, 2 + arguments.Count, false); - return callExpr; - } - } - else - { - var callExpr = new QueryCall(cx, method, node, parent, child); - operand.Populate(cx, callExpr, 0); - PopulateArguments(cx, callExpr, 1); - return callExpr; - } + this.declaration = declaration; + this.name = name; + } + + public override Expression Populate(Context cx, IExpressionParentEntity parent, int child) => + DeclareRangeVariable(cx, parent, child, true, declaration, name); + } + + class LetClause : Clause + { + readonly Clause operand; + readonly ISymbol declaration; + readonly SyntaxToken name; + ISymbol intoDeclaration; + + public LetClause(Clause operand, IMethodSymbol method, SyntaxNode node, ISymbol declaration, SyntaxToken name) : base(method, node) + { + this.operand = operand; + this.declaration = declaration; + this.name = name; + } + + public Clause WithInto(ISymbol into) + { + intoDeclaration = into; + return this; + } + + void DeclareIntoVariable(Context cx, IExpressionParentEntity parent, int intoChild, bool getElement) + { + if (intoDeclaration != null) + DeclareRangeVariable(cx, parent, intoChild, getElement, intoDeclaration, name); + } + + public override Expression Populate(Context cx, IExpressionParentEntity parent, int child) + { + if (method == null) + cx.ModelError(node, "Unable to determine target of query expression"); + + var callExpr = new QueryCall(cx, method, node, parent, child); + operand.Populate(cx, callExpr, 0); + DeclareRangeVariable(cx, callExpr, 1, false, declaration, name); + PopulateArguments(cx, callExpr, 2); + DeclareIntoVariable(cx, callExpr, 2 + arguments.Count, false); + return callExpr; + } + } + + class CallClause : Clause + { + readonly Clause operand; + + public CallClause(Clause operand, IMethodSymbol method, SyntaxNode node) : base(method, node) + { + this.operand = operand; + } + + public override Expression Populate(Context cx, IExpressionParentEntity parent, int child) + { + var callExpr = new QueryCall(cx, method, node, parent, child); + operand.Populate(cx, callExpr, 0); + PopulateArguments(cx, callExpr, 1); + return callExpr; } } @@ -161,18 +185,12 @@ public Expression Populate(Context cx, IExpressionParentEntity parent, int child /// The extraction context. /// The query expression. /// A "syntax tree" of the query. - static ClauseCall ConstructQueryExpression(Context cx, QueryExpressionSyntax node) + static Clause ConstructQueryExpression(Context cx, QueryExpressionSyntax node) { var info = cx.Model(node).GetQueryClauseInfo(node.FromClause); var method = info.OperationInfo.Symbol as IMethodSymbol; - ClauseCall clauseExpr = new ClauseCall - { - declaration = cx.Model(node).GetDeclaredSymbol(node.FromClause), - name = node.FromClause.Identifier, - method = method, - node = node.FromClause - }.AddArgument(node.FromClause.Expression); + Clause clauseExpr = new RangeClause(method, node.FromClause, cx.Model(node).GetDeclaredSymbol(node.FromClause), node.FromClause.Identifier).AddArgument(node.FromClause.Expression); foreach (var qc in node.Body.Clauses) { @@ -188,7 +206,7 @@ static ClauseCall ConstructQueryExpression(Context cx, QueryExpressionSyntax nod { method = cx.Model(node).GetSymbolInfo(ordering).Symbol as IMethodSymbol; - clauseExpr = clauseExpr.WithClause(method, orderByClause).AddArgument(ordering.Expression); + clauseExpr = clauseExpr.WithCallClause(method, orderByClause).AddArgument(ordering.Expression); if (method == null) cx.ModelError(ordering, "Could not determine method call for orderby clause"); @@ -196,23 +214,23 @@ static ClauseCall ConstructQueryExpression(Context cx, QueryExpressionSyntax nod break; case SyntaxKind.WhereClause: var whereClause = (WhereClauseSyntax)qc; - clauseExpr = clauseExpr.WithClause(method, whereClause).AddArgument(whereClause.Condition); + clauseExpr = clauseExpr.WithCallClause(method, whereClause).AddArgument(whereClause.Condition); break; case SyntaxKind.FromClause: var fromClause = (FromClauseSyntax)qc; clauseExpr = clauseExpr. - WithClause(method, fromClause, fromClause.Identifier, cx.Model(node).GetDeclaredSymbol(fromClause)). + WithLetClause(method, fromClause, cx.Model(node).GetDeclaredSymbol(fromClause), fromClause.Identifier). AddArgument(fromClause.Expression); break; case SyntaxKind.LetClause: var letClause = (LetClauseSyntax)qc; - clauseExpr = clauseExpr.WithClause(method, letClause, letClause.Identifier, cx.Model(node).GetDeclaredSymbol(letClause)). + clauseExpr = clauseExpr.WithLetClause(method, letClause, cx.Model(node).GetDeclaredSymbol(letClause), letClause.Identifier). AddArgument(letClause.Expression); break; case SyntaxKind.JoinClause: var joinClause = (JoinClauseSyntax)qc; - clauseExpr = clauseExpr.WithClause(method, joinClause, joinClause.Identifier, cx.Model(node).GetDeclaredSymbol(joinClause)). + clauseExpr = clauseExpr.WithLetClause(method, joinClause, cx.Model(node).GetDeclaredSymbol(joinClause), joinClause.Identifier). AddArgument(joinClause.InExpression). AddArgument(joinClause.LeftExpression). AddArgument(joinClause.RightExpression); @@ -220,7 +238,7 @@ static ClauseCall ConstructQueryExpression(Context cx, QueryExpressionSyntax nod if (joinClause.Into != null) { var into = cx.Model(node).GetDeclaredSymbol(joinClause.Into); - clauseExpr.WithInto(into); + ((LetClause)clauseExpr).WithInto(into); } break; @@ -231,16 +249,13 @@ static ClauseCall ConstructQueryExpression(Context cx, QueryExpressionSyntax nod method = cx.Model(node).GetSymbolInfo(node.Body.SelectOrGroup).Symbol as IMethodSymbol; - var selectClause = node.Body.SelectOrGroup as SelectClauseSyntax; - var groupClause = node.Body.SelectOrGroup as GroupClauseSyntax; - - clauseExpr = new ClauseCall { operand = clauseExpr, method = method, node = node.Body.SelectOrGroup }; + clauseExpr = new CallClause(clauseExpr, method, node.Body.SelectOrGroup); - if (selectClause != null) + if (node.Body.SelectOrGroup is SelectClauseSyntax selectClause) { clauseExpr.AddArgument(selectClause.Expression); } - else if (groupClause != null) + else if (node.Body.SelectOrGroup is GroupClauseSyntax groupClause) { clauseExpr. AddArgument(groupClause.GroupExpression). diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Indexer.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Indexer.cs index 8fc7e20ccafc..a133bd978859 100644 --- a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Indexer.cs +++ b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Indexer.cs @@ -21,18 +21,14 @@ public override void Populate() var getter = symbol.GetMethod; var setter = symbol.SetMethod; - if (getter == null && setter == null) + if (getter is null && setter is null) Context.ModelError(symbol, "No indexer accessor defined"); - if (getter != null) - { - Getter = Accessor.Create(Context, getter); - } + if (!(getter is null)) + Method.Create(Context, getter); - if (setter != null) - { - Setter = Accessor.Create(Context, setter); - } + if (!(setter is null)) + Method.Create(Context, setter); for (var i = 0; i < symbol.Parameters.Length; ++i) { @@ -40,26 +36,6 @@ public override void Populate() Parameter.Create(Context, symbol.Parameters[i], this, original); } - if (getter != null) - { - Getter = Accessor.Create(Context, getter); - Context.Emit(Tuples.accessors(Getter, 1, getter.Name, this, Getter.OriginalDefinition)); - - Context.Emit(Tuples.accessor_location(Getter, Getter.Location)); - Getter.Overrides(); - Getter.ExtractModifiers(); - } - - if (setter != null) - { - Setter = Accessor.Create(Context, setter); - Context.Emit(Tuples.accessors(Setter, 2, setter.Name, this, Setter.OriginalDefinition)); - - Context.Emit(Tuples.accessor_location(Setter, Setter.Location)); - Setter.Overrides(); - Setter.ExtractModifiers(); - } - if (IsSourceDeclaration) { var expressionBody = ExpressionBody; diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Method.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Method.cs index a54a93e65ddb..5c9f82a7b543 100644 --- a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Method.cs +++ b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Method.cs @@ -256,7 +256,15 @@ public static Method Create(Context cx, IMethodSymbol methodDecl) { if (methodDecl == null) return null; - switch (methodDecl.MethodKind) + var methodKind = methodDecl.MethodKind; + + if(methodKind == MethodKind.ExplicitInterfaceImplementation) + { + // Retrieve the original method kind + methodKind = methodDecl.ExplicitInterfaceImplementations.Select(m => m.MethodKind).FirstOrDefault(); + } + + switch (methodKind) { case MethodKind.StaticConstructor: case MethodKind.Constructor: @@ -264,13 +272,12 @@ public static Method Create(Context cx, IMethodSymbol methodDecl) case MethodKind.ReducedExtension: case MethodKind.Ordinary: case MethodKind.DelegateInvoke: - case MethodKind.ExplicitInterfaceImplementation: return OrdinaryMethod.Create(cx, methodDecl); case MethodKind.Destructor: return Destructor.Create(cx, methodDecl); case MethodKind.PropertyGet: case MethodKind.PropertySet: - return Accessor.Create(cx, methodDecl); + return methodDecl.AssociatedSymbol is null ? OrdinaryMethod.Create(cx, methodDecl) : (Method)Accessor.Create(cx, methodDecl); case MethodKind.EventAdd: case MethodKind.EventRemove: return EventAccessor.Create(cx, methodDecl); diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Namespace.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Namespace.cs index f66ac8e6d605..30c155a6d8e9 100644 --- a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Namespace.cs +++ b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Namespace.cs @@ -2,7 +2,7 @@ namespace Semmle.Extraction.CSharp.Entities { - class Namespace : CachedEntity + sealed class Namespace : CachedEntity { Namespace(Context cx, INamespaceSymbol init) : base(cx, init) { } @@ -31,7 +31,7 @@ public override IId Id } } - public static Namespace Create(Context cx, INamespaceSymbol ns) => NamespaceFactory.Instance.CreateEntity(cx, ns); + public static Namespace Create(Context cx, INamespaceSymbol ns) => NamespaceFactory.Instance.CreateEntity2(cx, ns); class NamespaceFactory : ICachedEntityFactory { @@ -41,5 +41,14 @@ class NamespaceFactory : ICachedEntityFactory } public override TrapStackBehaviour TrapStackBehaviour => TrapStackBehaviour.NoLabel; + + public override int GetHashCode() => QualifiedName.GetHashCode(); + + string QualifiedName => symbol.ToDisplayString(); + + public override bool Equals(object obj) + { + return obj is Namespace ns && QualifiedName == ns.QualifiedName; + } } } diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Property.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Property.cs index 30cfadd3125b..a682df4744d0 100644 --- a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Property.cs +++ b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Property.cs @@ -24,9 +24,6 @@ public override IId Id } } - protected Accessor Getter { get; set; } - protected Accessor Setter { get; set; } - public override void Populate() { ExtractMetadataHandle(); @@ -38,12 +35,13 @@ public override void Populate() Context.Emit(Tuples.properties(this, symbol.GetName(), ContainingType, type.TypeRef, Create(Context, symbol.OriginalDefinition))); var getter = symbol.GetMethod; - if (getter != null) - Getter = Accessor.Create(Context, getter); - var setter = symbol.SetMethod; - if (setter != null) - Setter = Accessor.Create(Context, setter); + + if (!(getter is null)) + Method.Create(Context, getter); + + if (!(setter is null)) + Method.Create(Context, setter); var declSyntaxReferences = IsSourceDeclaration ? symbol.DeclaringSyntaxReferences. @@ -101,7 +99,9 @@ public override Microsoft.CodeAnalysis.Location FullLocation public static Property Create(Context cx, IPropertySymbol prop) { - return prop.IsIndexer ? Indexer.Create(cx, prop) : PropertyFactory.Instance.CreateEntity(cx, prop); + bool isIndexer = prop.IsIndexer || prop.ExplicitInterfaceImplementations.Any(e => e.IsIndexer); + + return isIndexer ? Indexer.Create(cx, prop) : PropertyFactory.Instance.CreateEntity(cx, prop); } public void VisitDeclaration(Context cx, PropertyDeclarationSyntax p) diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Statements/For.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Statements/For.cs index 2c918af83ef6..c0860d21d127 100644 --- a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Statements/For.cs +++ b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Statements/For.cs @@ -28,7 +28,10 @@ protected override void Populate() Expression.Create(cx, init, this, child--); } - Statement.Create(cx, Stmt.Statement, this, 1 + Stmt.Incrementors.Count); + if (Stmt.Condition != null) + { + Expression.Create(cx, Stmt.Condition, this, 0); + } child = 1; foreach (var inc in Stmt.Incrementors) @@ -36,10 +39,7 @@ protected override void Populate() Expression.Create(cx, inc, this, child++); } - if (Stmt.Condition != null) - { - Expression.Create(cx, Stmt.Condition, this, 0); - } + Statement.Create(cx, Stmt.Statement, this, 1 + Stmt.Incrementors.Count); } } } diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Types/NamedType.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Types/NamedType.cs index a5dba66c9342..45dcef696dd7 100644 --- a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Types/NamedType.cs +++ b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Types/NamedType.cs @@ -174,7 +174,7 @@ public NamedTypeRef(Context cx, INamedTypeSymbol symbol) : base(cx, symbol) referencedType = Type.Create(cx, symbol); } - public static NamedTypeRef Create(Context cx, INamedTypeSymbol type) => NamedTypeRefFactory.Instance.CreateEntity(cx, type); + public static NamedTypeRef Create(Context cx, INamedTypeSymbol type) => NamedTypeRefFactory.Instance.CreateEntity2(cx, type); class NamedTypeRefFactory : ICachedEntityFactory { diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Types/Type.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Types/Type.cs index 16d97b846660..465015019fdf 100644 --- a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Types/Type.cs +++ b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Types/Type.cs @@ -158,7 +158,7 @@ protected void ExtractType() var param = invokeMethod.Parameters[i]; var originalParam = invokeMethod.OriginalDefinition.Parameters[i]; var originalParamEntity = Equals(param, originalParam) ? null : - DelegateTypeParameter.Create(Context, originalParam, Create(Context, ((INamedTypeSymbol)symbol).ConstructedFrom)); + DelegateTypeParameter.Create(Context, originalParam, Create(Context, ((INamedTypeSymbol)symbol).OriginalDefinition)); DelegateTypeParameter.Create(Context, param, this, originalParamEntity); } diff --git a/csharp/extractor/Semmle.Extraction/Context.cs b/csharp/extractor/Semmle.Extraction/Context.cs index b2b3c42765f8..76ed4ddea63a 100644 --- a/csharp/extractor/Semmle.Extraction/Context.cs +++ b/csharp/extractor/Semmle.Extraction/Context.cs @@ -43,28 +43,90 @@ public SemanticModel Model(SyntaxNode node) int NewId() => TrapWriter.IdCounter++; /// - /// Gets the cached label for the given entity, or creates a new - /// (cached) label if it hasn't already been created. The label - /// is set on the supplied object. + /// Creates a new entity using the factory. /// - /// true iff the label already existed. - public bool GetOrAddCachedLabel(ICachedEntity entity) + /// The entity factory. + /// The initializer for the entity. + /// The new/existing entity. + public Entity CreateEntity(ICachedEntityFactory factory, Type init) where Entity : ICachedEntity { - var id = GetId(entity); - if (id == null) - throw new InternalError("Attempt to create a null entity for {0}", entity.GetType()); + return init == null ? CreateEntity2(factory, init) : CreateNonNullEntity(factory, init); + } - Label existingLabel; - if (labelCache.TryGetValue(id, out existingLabel)) + /// + /// Creates a new entity using the factory. + /// Uses a different cache to , + /// and can store null values. + /// + /// The entity factory. + /// The initializer for the entity. + /// The new/existing entity. + public Entity CreateEntity2(ICachedEntityFactory factory, Type init) where Entity : ICachedEntity + { + using (StackGuard) { - entity.Label = existingLabel; - return true; + var entity = factory.Create(this, init); + + if (entityLabelCache.TryGetValue(entity, out var label)) + { + entity.Label = label; + } + else + { + var id = entity.Id; +#if DEBUG_LABELS + CheckEntityHasUniqueLabel(id, entity); +#endif + label = new Label(NewId()); + entity.Label = label; + entityLabelCache[entity] = label; + DefineLabel(label, id); + if (entity.NeedsPopulation) + Populate(init as ISymbol, entity); + } + return entity; } + } - entity.Label = new Label(NewId()); - DefineLabel(entity.Label, id); - labelCache[id] = entity.Label; - return false; +#if DEBUG_LABELS + private void CheckEntityHasUniqueLabel(IId id, ICachedEntity entity) + { + if (idLabelCache.TryGetValue(id, out var originalEntity)) + { + Extractor.Message(new Message { message = "Label collision for " + id.ToString(), severity = Severity.Warning }); + } + else + { + idLabelCache[id] = entity; + } + } +#endif + + private Entity CreateNonNullEntity(ICachedEntityFactory factory, Type init) where Entity : ICachedEntity + { + if (objectEntityCache.TryGetValue(init, out var cached)) + return (Entity)cached; + + using (StackGuard) + { + var label = new Label(NewId()); + var entity = factory.Create(this, init); + entity.Label = label; + + objectEntityCache[init] = entity; + + var id = entity.Id; + DefineLabel(label, id); + +#if DEBUG_LABELS + CheckEntityHasUniqueLabel(id, entity); +#endif + + if (entity.NeedsPopulation) + Populate(init as ISymbol, entity); + + return entity; + } } /// @@ -90,25 +152,6 @@ public bool ExtractGenerics(ICachedEntity entity) } } - /// - /// Gets the ID belonging to cached entity . - /// - /// The ID itself is also cached, but unlike the label cache (which is used - /// to prevent reextraction/infinite loops), this is a pure performance - /// optimization. Moreover, the label cache is injective, which the ID cache - /// need not be. - /// - IId GetId(ICachedEntity entity) - { - IId id; - if (!idCache.TryGetValue(entity, out id)) - { - id = entity.Id; - idCache[entity] = id; - } - return id; - } - /// /// Creates a fresh label with ID "*", and set it on the /// supplied object. @@ -120,7 +163,11 @@ public void AddFreshLabel(IEntity entity) entity.Label = label; } - readonly Dictionary labelCache = new Dictionary(); +#if DEBUG_LABELS + readonly Dictionary idLabelCache = new Dictionary(); +#endif + readonly Dictionary objectEntityCache = new Dictionary(); + readonly Dictionary entityLabelCache = new Dictionary(); readonly HashSet