From cdfe2390165cb59bba3a8d22c030d326db9039ba Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Thu, 27 Jan 2022 14:50:04 +0100 Subject: [PATCH] C#: Guard against `AssociatedSymbol` not being an `IEventSymbol` Apply same logic as for property/indexer accessors to account for cases where the associated event cannot be determined. I have not been able to reproduce such cases locally, though we have seen reports of it happening. --- .../Entities/Accessor.cs | 45 ++++++++----------- .../Entities/EventAccessor.cs | 41 ++++++++--------- .../Entities/Method.cs | 4 +- 3 files changed, 41 insertions(+), 49 deletions(-) diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Accessor.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Accessor.cs index 20a9e84a92c..183e42f6ecb 100644 --- a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Accessor.cs +++ b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Accessor.cs @@ -6,11 +6,15 @@ namespace Semmle.Extraction.CSharp.Entities { internal class Accessor : Method { - protected Accessor(Context cx, IMethodSymbol init) - : base(cx, init) { } + private readonly IPropertySymbol property; + protected Accessor(Context cx, IMethodSymbol init, IPropertySymbol property) + : base(cx, init) + { + this.property = property; + } /// - /// Gets the property symbol associated accessor `symbol`, or `null` + /// Gets the property symbol associated with accessor `symbol`, or `null` /// if there is no associated symbol. /// public static IPropertySymbol? GetPropertySymbol(IMethodSymbol symbol) @@ -26,39 +30,26 @@ namespace Semmle.Extraction.CSharp.Entities return props.SingleOrDefault(); } - /// - /// Gets the property symbol associated with this accessor. - /// - private IPropertySymbol? PropertySymbol => GetPropertySymbol(Symbol); - - public new Accessor OriginalDefinition => Create(Context, Symbol.OriginalDefinition); - public override void Populate(TextWriter trapFile) { PopulateMethod(trapFile); PopulateModifiers(trapFile); ContainingType!.PopulateGenerics(); - var prop = PropertySymbol; - if (prop is null) - { - var type = Symbol.AssociatedSymbol?.GetType().ToString() ?? "null"; - Context.ModelError(Symbol, $"Unhandled accessor associated symbol of type {type}"); - return; - } - - var parent = Property.Create(Context, prop); + var parent = Property.Create(Context, property); int kind; Accessor unboundAccessor; - if (SymbolEqualityComparer.Default.Equals(Symbol, prop.GetMethod)) + if (SymbolEqualityComparer.Default.Equals(Symbol, property.GetMethod)) { kind = 1; - unboundAccessor = Create(Context, prop.OriginalDefinition.GetMethod!); + var orig = property.OriginalDefinition; + unboundAccessor = Create(Context, orig.GetMethod!, orig); } - else if (SymbolEqualityComparer.Default.Equals(Symbol, prop.SetMethod)) + else if (SymbolEqualityComparer.Default.Equals(Symbol, property.SetMethod)) { kind = 2; - unboundAccessor = Create(Context, prop.OriginalDefinition.SetMethod!); + var orig = property.OriginalDefinition; + unboundAccessor = Create(Context, orig.SetMethod!, orig); } else { @@ -84,14 +75,14 @@ namespace Semmle.Extraction.CSharp.Entities } } - public static new Accessor Create(Context cx, IMethodSymbol symbol) => - AccessorFactory.Instance.CreateEntityFromSymbol(cx, symbol); + public static Accessor Create(Context cx, IMethodSymbol symbol, IPropertySymbol prop) => + AccessorFactory.Instance.CreateEntity(cx, symbol, (symbol, prop)); - private class AccessorFactory : CachedEntityFactory + private class AccessorFactory : CachedEntityFactory<(IMethodSymbol, IPropertySymbol), Accessor> { public static AccessorFactory Instance { get; } = new AccessorFactory(); - public override Accessor Create(Context cx, IMethodSymbol init) => new Accessor(cx, init); + public override Accessor Create(Context cx, (IMethodSymbol, IPropertySymbol) init) => new(cx, init.Item1, init.Item2); } } } diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/EventAccessor.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/EventAccessor.cs index 0884caf1396..f51cd6c894f 100644 --- a/csharp/extractor/Semmle.Extraction.CSharp/Entities/EventAccessor.cs +++ b/csharp/extractor/Semmle.Extraction.CSharp/Entities/EventAccessor.cs @@ -3,45 +3,46 @@ using System.IO; namespace Semmle.Extraction.CSharp.Entities { - internal class EventAccessor : Accessor + internal class EventAccessor : Method { - private EventAccessor(Context cx, IMethodSymbol init) - : base(cx, init) { } + private readonly IEventSymbol @event; + + private EventAccessor(Context cx, IMethodSymbol init, IEventSymbol @event) + : base(cx, init) + { + this.@event = @event; + } /// - /// Gets the event symbol associated with this accessor. + /// Gets the event symbol associated with accessor `symbol`, or `null` + /// if there is no associated symbol. /// - private IEventSymbol? EventSymbol => Symbol.AssociatedSymbol as IEventSymbol; + public static IEventSymbol? GetEventSymbol(IMethodSymbol symbol) => + symbol.AssociatedSymbol as IEventSymbol; public override void Populate(TextWriter trapFile) { PopulateMethod(trapFile); ContainingType!.PopulateGenerics(); - var @event = EventSymbol; - if (@event is null) - { - var type = Symbol.AssociatedSymbol?.GetType().ToString() ?? "null"; - Context.ModelError(Symbol, $"Unhandled event accessor associated symbol of type {type}"); - return; - } - var parent = Event.Create(Context, @event); int kind; EventAccessor unboundAccessor; if (SymbolEqualityComparer.Default.Equals(Symbol, @event.AddMethod)) { kind = 1; - unboundAccessor = Create(Context, @event.OriginalDefinition.AddMethod!); + var orig = @event.OriginalDefinition; + unboundAccessor = Create(Context, orig.AddMethod!, orig); } else if (SymbolEqualityComparer.Default.Equals(Symbol, @event.RemoveMethod)) { kind = 2; - unboundAccessor = Create(Context, @event.OriginalDefinition.RemoveMethod!); + var orig = @event.OriginalDefinition; + unboundAccessor = Create(Context, orig.RemoveMethod!, orig); } else { - Context.ModelError(Symbol, "Undhandled event accessor kind"); + Context.ModelError(Symbol, $"Undhandled event accessor kind {Symbol.ToDisplayString()}"); return; } @@ -53,14 +54,14 @@ namespace Semmle.Extraction.CSharp.Entities Overrides(trapFile); } - public static new EventAccessor Create(Context cx, IMethodSymbol symbol) => - EventAccessorFactory.Instance.CreateEntityFromSymbol(cx, symbol); + public static EventAccessor Create(Context cx, IMethodSymbol symbol, IEventSymbol @event) => + EventAccessorFactory.Instance.CreateEntity(cx, symbol, (symbol, @event)); - private class EventAccessorFactory : CachedEntityFactory + private class EventAccessorFactory : CachedEntityFactory<(IMethodSymbol, IEventSymbol), EventAccessor> { public static EventAccessorFactory Instance { get; } = new EventAccessorFactory(); - public override EventAccessor Create(Context cx, IMethodSymbol init) => new EventAccessor(cx, init); + public override EventAccessor Create(Context cx, (IMethodSymbol, IEventSymbol) init) => new EventAccessor(cx, init.Item1, init.Item2); } } } diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Method.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Method.cs index 20c0033c4de..8777e95d5d0 100644 --- a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Method.cs +++ b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Method.cs @@ -262,10 +262,10 @@ namespace Semmle.Extraction.CSharp.Entities return Destructor.Create(cx, methodDecl); case MethodKind.PropertyGet: case MethodKind.PropertySet: - return Accessor.GetPropertySymbol(methodDecl) is null ? OrdinaryMethod.Create(cx, methodDecl) : (Method)Accessor.Create(cx, methodDecl); + return Accessor.GetPropertySymbol(methodDecl) is IPropertySymbol prop ? Accessor.Create(cx, methodDecl, prop) : OrdinaryMethod.Create(cx, methodDecl); case MethodKind.EventAdd: case MethodKind.EventRemove: - return EventAccessor.Create(cx, methodDecl); + return EventAccessor.GetEventSymbol(methodDecl) is IEventSymbol @event ? EventAccessor.Create(cx, methodDecl, @event) : OrdinaryMethod.Create(cx, methodDecl); case MethodKind.UserDefinedOperator: case MethodKind.BuiltinOperator: return UserOperator.Create(cx, methodDecl);