From 6b38a81e5072e5464ad7b3582b6de4a80eb6e81f Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Thu, 15 Feb 2024 15:34:06 +0100 Subject: [PATCH 1/8] C#: Add some test cases for primary constructor inititalizers and a failing dataflow test. --- .../dataflow/constructors/ConstructorFlow.expected | 1 + .../dataflow/constructors/Constructors.cs | 12 ++++++++++++ .../test/library-tests/expressions/PrintAst.expected | 12 ++++++++++++ .../ql/test/library-tests/expressions/expressions.cs | 4 ++++ 4 files changed, 29 insertions(+) diff --git a/csharp/ql/test/library-tests/dataflow/constructors/ConstructorFlow.expected b/csharp/ql/test/library-tests/dataflow/constructors/ConstructorFlow.expected index a12db3ca3a9..10ba6381c6a 100644 --- a/csharp/ql/test/library-tests/dataflow/constructors/ConstructorFlow.expected +++ b/csharp/ql/test/library-tests/dataflow/constructors/ConstructorFlow.expected @@ -1,4 +1,5 @@ testFailures +| Constructors.cs:113:25:113:43 | // ... | Missing result:hasValueFlow=6 | edges | Constructors.cs:5:24:5:25 | [post] this access : C_no_ctor [field s1] : Object | Constructors.cs:9:27:9:41 | object creation of type C_no_ctor : C_no_ctor [field s1] : Object | provenance | | | Constructors.cs:5:29:5:45 | call to method Source : Object | Constructors.cs:5:24:5:25 | [post] this access : C_no_ctor [field s1] : Object | provenance | | diff --git a/csharp/ql/test/library-tests/dataflow/constructors/Constructors.cs b/csharp/ql/test/library-tests/dataflow/constructors/Constructors.cs index 41724b78ddf..f8a75953d08 100644 --- a/csharp/ql/test/library-tests/dataflow/constructors/Constructors.cs +++ b/csharp/ql/test/library-tests/dataflow/constructors/Constructors.cs @@ -101,6 +101,18 @@ public class Constructors Sink(c2.Obj22); // $ hasValueFlow=5 } + public class C3(object o31param) + { + public object Obj31 => o31param; + } + + public void M5() + { + var o31 = Source(6); + var c3 = new C3(o31); + Sink(c3.Obj31); // $ hasValueFlow=6 + } + public static void Sink(object o) { } public static T Source(object source) => throw null; diff --git a/csharp/ql/test/library-tests/expressions/PrintAst.expected b/csharp/ql/test/library-tests/expressions/PrintAst.expected index 60006e8d939..c1d146f8eef 100644 --- a/csharp/ql/test/library-tests/expressions/PrintAst.expected +++ b/csharp/ql/test/library-tests/expressions/PrintAst.expected @@ -2406,3 +2406,15 @@ expressions.cs: # 512| 0: [IntLiteral] 10 # 515| 5: [Field] myInlineArrayElements # 515| -1: [TypeMention] int +# 518| 22: [Class] ClassC1 +# 518| 4: [InstanceConstructor,PrimaryConstructor] ClassC1 +#-----| 2: (Parameters) +# 518| 0: [Parameter] oc1 +# 518| -1: [TypeMention] object +# 520| 23: [Class] ClassC2 +#-----| 3: (Base types) +# 520| 0: [TypeMention] ClassC1 +# 520| 4: [InstanceConstructor,PrimaryConstructor] ClassC2 +#-----| 2: (Parameters) +# 520| 0: [Parameter] oc2 +# 520| -1: [TypeMention] object diff --git a/csharp/ql/test/library-tests/expressions/expressions.cs b/csharp/ql/test/library-tests/expressions/expressions.cs index 5e5826d7d23..b4079d5e9a3 100644 --- a/csharp/ql/test/library-tests/expressions/expressions.cs +++ b/csharp/ql/test/library-tests/expressions/expressions.cs @@ -514,4 +514,8 @@ namespace Expressions { private int myInlineArrayElements; } + + class ClassC1(object oc1) { } + + class ClassC2(object oc2) : ClassC1(oc2) { } } From c613851c2d2ac2d5142622257fdabff231abde18 Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Thu, 15 Feb 2024 13:31:34 +0100 Subject: [PATCH 2/8] C#: Invert logic in ExtractInitializer. --- .../Entities/Constructor.cs | 126 +++++++++--------- 1 file changed, 62 insertions(+), 64 deletions(-) diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Constructor.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Constructor.cs index c7b2b9abc6a..1583c46750d 100644 --- a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Constructor.cs +++ b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Constructor.cs @@ -39,80 +39,78 @@ namespace Semmle.Extraction.CSharp.Entities var syntax = Syntax; var initializer = syntax?.Initializer; - if (initializer is null) + if (initializer is not null) { - if (Symbol.MethodKind is MethodKind.Constructor) + ITypeSymbol initializerType; + var symbolInfo = Context.GetSymbolInfo(initializer); + + switch (initializer.Kind()) { - var baseType = Symbol.ContainingType.BaseType; - if (baseType is null) - { - if (Symbol.ContainingType.SpecialType != SpecialType.System_Object) - { - Context.ModelError(Symbol, "Unable to resolve base type in implicit constructor initializer"); - } + case SyntaxKind.BaseConstructorInitializer: + initializerType = Symbol.ContainingType.BaseType!; + break; + case SyntaxKind.ThisConstructorInitializer: + initializerType = Symbol.ContainingType; + break; + default: + Context.ModelError(initializer, "Unknown initializer"); return; - } - - var baseConstructor = baseType.InstanceConstructors.FirstOrDefault(c => c.Arity is 0); - - if (baseConstructor is null) - { - Context.ModelError(Symbol, "Unable to resolve implicit constructor initializer call"); - return; - } - - var baseConstructorTarget = Create(Context, baseConstructor); - var info = new ExpressionInfo(Context, - AnnotatedTypeSymbol.CreateNotAnnotated(baseType), - Location, - Kinds.ExprKind.CONSTRUCTOR_INIT, - this, - -1, - isCompilerGenerated: true, - null); - - trapFile.expr_call(new Expression(info), baseConstructorTarget); } - return; - } - ITypeSymbol initializerType; - var symbolInfo = Context.GetSymbolInfo(initializer); + var initInfo = new ExpressionInfo(Context, + AnnotatedTypeSymbol.CreateNotAnnotated(initializerType), + Context.CreateLocation(initializer.ThisOrBaseKeyword.GetLocation()), + Kinds.ExprKind.CONSTRUCTOR_INIT, + this, + -1, + false, + null); - switch (initializer.Kind()) - { - case SyntaxKind.BaseConstructorInitializer: - initializerType = Symbol.ContainingType.BaseType!; - break; - case SyntaxKind.ThisConstructorInitializer: - initializerType = Symbol.ContainingType; - break; - default: - Context.ModelError(initializer, "Unknown initializer"); + var init = new Expression(initInfo); + + var target = Constructor.Create(Context, (IMethodSymbol?)symbolInfo.Symbol); + if (target is null) + { + Context.ModelError(Symbol, "Unable to resolve call"); return; + } + + trapFile.expr_call(init, target); + + init.PopulateArguments(trapFile, initializer.ArgumentList, 0); } - - var initInfo = new ExpressionInfo(Context, - AnnotatedTypeSymbol.CreateNotAnnotated(initializerType), - Context.CreateLocation(initializer.ThisOrBaseKeyword.GetLocation()), - Kinds.ExprKind.CONSTRUCTOR_INIT, - this, - -1, - false, - null); - - var init = new Expression(initInfo); - - var target = Constructor.Create(Context, (IMethodSymbol?)symbolInfo.Symbol); - if (target is null) + else if (Symbol.MethodKind is MethodKind.Constructor) { - Context.ModelError(Symbol, "Unable to resolve call"); - return; + var baseType = Symbol.ContainingType.BaseType; + if (baseType is null) + { + if (Symbol.ContainingType.SpecialType != SpecialType.System_Object) + { + Context.ModelError(Symbol, "Unable to resolve base type in implicit constructor initializer"); + } + return; + } + + var baseConstructor = baseType.InstanceConstructors.FirstOrDefault(c => c.Arity is 0); + + if (baseConstructor is null) + { + Context.ModelError(Symbol, "Unable to resolve implicit constructor initializer call"); + return; + } + + var baseConstructorTarget = Create(Context, baseConstructor); + var info = new ExpressionInfo(Context, + AnnotatedTypeSymbol.CreateNotAnnotated(baseType), + Location, + Kinds.ExprKind.CONSTRUCTOR_INIT, + this, + -1, + isCompilerGenerated: true, + null); + + trapFile.expr_call(new Expression(info), baseConstructorTarget); } - - trapFile.expr_call(init, target); - - init.PopulateArguments(trapFile, initializer.ArgumentList, 0); } private ConstructorDeclarationSyntax? Syntax From 7a802055199db5add3389d2bd785323ac2ccaa64 Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Thu, 15 Feb 2024 13:56:54 +0100 Subject: [PATCH 3/8] C#: Extract explicit and implicit primary constructor initializers. --- .../Entities/Constructor.cs | 110 +++++++++++------- .../Entities/Method.cs | 3 +- .../Entities/Types/Type.cs | 2 +- 3 files changed, 74 insertions(+), 41 deletions(-) diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Constructor.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Constructor.cs index 1583c46750d..58c689b8e70 100644 --- a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Constructor.cs +++ b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Constructor.cs @@ -1,3 +1,5 @@ +using System; +using System.Collections.Generic; using System.Diagnostics.CodeAnalysis; using System.IO; using System.Linq; @@ -10,8 +12,16 @@ namespace Semmle.Extraction.CSharp.Entities { internal class Constructor : Method { + private readonly Lazy> DeclaringReferenceSyntax; + private Constructor(Context cx, IMethodSymbol init) - : base(cx, init) { } + : base(cx, init) + { + DeclaringReferenceSyntax = new(() => + Symbol.DeclaringSyntaxReferences + .Select(r => r.GetSyntax()) + .ToList()); + } public override void Populate(TextWriter trapFile) { @@ -33,16 +43,17 @@ namespace Semmle.Extraction.CSharp.Entities protected override void ExtractInitializers(TextWriter trapFile) { // Do not extract initializers for constructed types. - if (!IsSourceDeclaration) + // Only extract initializers for constructors with a body and primary constructors. + if (Block is null && ExpressionBody is null && !IsPrimary || + !IsSourceDeclaration) + { return; + } - var syntax = Syntax; - var initializer = syntax?.Initializer; - - if (initializer is not null) + if (OrdinaryConstructorSyntax?.Initializer is ConstructorInitializerSyntax initializer) { ITypeSymbol initializerType; - var symbolInfo = Context.GetSymbolInfo(initializer); + var initializerInfo = Context.GetSymbolInfo(initializer); switch (initializer.Kind()) { @@ -57,27 +68,14 @@ namespace Semmle.Extraction.CSharp.Entities return; } - var initInfo = new ExpressionInfo(Context, - AnnotatedTypeSymbol.CreateNotAnnotated(initializerType), - Context.CreateLocation(initializer.ThisOrBaseKeyword.GetLocation()), - Kinds.ExprKind.CONSTRUCTOR_INIT, - this, - -1, - false, - null); + ExtractSourceInitializer(trapFile, initializerType, (IMethodSymbol?)initializerInfo.Symbol, initializer.ArgumentList, initializer.ThisOrBaseKeyword.GetLocation()); + } + else if (PrimaryBase is PrimaryConstructorBaseTypeSyntax primaryInitializer) + { + var primaryInfo = Context.GetSymbolInfo(primaryInitializer); + var primarySymbol = primaryInfo.Symbol; - var init = new Expression(initInfo); - - var target = Constructor.Create(Context, (IMethodSymbol?)symbolInfo.Symbol); - if (target is null) - { - Context.ModelError(Symbol, "Unable to resolve call"); - return; - } - - trapFile.expr_call(init, target); - - init.PopulateArguments(trapFile, initializer.ArgumentList, 0); + ExtractSourceInitializer(trapFile, primarySymbol?.ContainingType, (IMethodSymbol?)primarySymbol, primaryInitializer.ArgumentList, primaryInitializer.GetLocation()); } else if (Symbol.MethodKind is MethodKind.Constructor) { @@ -113,17 +111,50 @@ namespace Semmle.Extraction.CSharp.Entities } } - private ConstructorDeclarationSyntax? Syntax + private void ExtractSourceInitializer(TextWriter trapFile, ITypeSymbol? type, IMethodSymbol? symbol, ArgumentListSyntax arguments, Location location) { - get + var initInfo = new ExpressionInfo(Context, + AnnotatedTypeSymbol.CreateNotAnnotated(type), + Context.CreateLocation(location), + Kinds.ExprKind.CONSTRUCTOR_INIT, + this, + -1, + false, + null); + + var init = new Expression(initInfo); + + var target = Constructor.Create(Context, symbol); + if (target is null) { - return Symbol.DeclaringSyntaxReferences - .Select(r => r.GetSyntax()) - .OfType() - .FirstOrDefault(); + Context.ModelError(Symbol, "Unable to resolve call"); + return; } + + trapFile.expr_call(init, target); + + init.PopulateArguments(trapFile, arguments, 0); } + private ConstructorDeclarationSyntax? OrdinaryConstructorSyntax => + DeclaringReferenceSyntax.Value + .OfType() + .FirstOrDefault(); + + private TypeDeclarationSyntax? PrimaryConstructorSyntax => + DeclaringReferenceSyntax.Value + .OfType() + .FirstOrDefault(t => t is ClassDeclarationSyntax or StructDeclarationSyntax or RecordDeclarationSyntax); + + private PrimaryConstructorBaseTypeSyntax? PrimaryBase => + PrimaryConstructorSyntax? + .BaseList? + .Types + .OfType() + .FirstOrDefault(); + + private bool IsPrimary => PrimaryConstructorSyntax is not null; + [return: NotNullIfNotNull("constructor")] public static new Constructor? Create(Context cx, IMethodSymbol? constructor) { @@ -158,19 +189,20 @@ namespace Semmle.Extraction.CSharp.Entities trapFile.Write(";constructor"); } - private ConstructorDeclarationSyntax? GetSyntax() => - Symbol.DeclaringSyntaxReferences.Select(r => r.GetSyntax()).OfType().FirstOrDefault(); - public override Microsoft.CodeAnalysis.Location? FullLocation => ReportingLocation; public override Microsoft.CodeAnalysis.Location? ReportingLocation { get { - var syn = GetSyntax(); - if (syn is not null) + if (OrdinaryConstructorSyntax is not null) { - return syn.Identifier.GetLocation(); + return OrdinaryConstructorSyntax.Identifier.GetLocation(); + } + + if (PrimaryConstructorSyntax is not null) + { + return PrimaryConstructorSyntax.Identifier.GetLocation(); } if (Symbol.IsImplicitlyDeclared) diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Method.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Method.cs index feffcbb3a54..e7915717c90 100644 --- a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Method.cs +++ b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Method.cs @@ -54,12 +54,13 @@ namespace Semmle.Extraction.CSharp.Entities var block = Block; var expr = ExpressionBody; + Context.PopulateLater(() => ExtractInitializers(trapFile)); + if (block is not null || expr is not null) { Context.PopulateLater( () => { - ExtractInitializers(trapFile); if (block is not null) Statements.Block.Create(Context, block, this, 0); else diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Types/Type.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Types/Type.cs index 291dda12942..e3919d40800 100644 --- a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Types/Type.cs +++ b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Types/Type.cs @@ -57,7 +57,7 @@ namespace Semmle.Extraction.CSharp.Entities { return Kinds.TypeKind.TUPLE; } - return Symbol.IsInlineArray() + return Symbol.IsInlineArray() ? Kinds.TypeKind.INLINE_ARRAY : Kinds.TypeKind.STRUCT; } From dcde6597bc13c7be65e1b32189a07ccbcdaa6ebb Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Thu, 15 Feb 2024 15:41:12 +0100 Subject: [PATCH 4/8] C#: Updated expected test output. --- .../constructors/ConstructorFlow.expected | 22 ++++++++++++++++++- .../ConstructorInitializers.expected | 2 ++ .../expressions/PrintAst.expected | 2 ++ 3 files changed, 25 insertions(+), 1 deletion(-) diff --git a/csharp/ql/test/library-tests/dataflow/constructors/ConstructorFlow.expected b/csharp/ql/test/library-tests/dataflow/constructors/ConstructorFlow.expected index 10ba6381c6a..70faa61bd73 100644 --- a/csharp/ql/test/library-tests/dataflow/constructors/ConstructorFlow.expected +++ b/csharp/ql/test/library-tests/dataflow/constructors/ConstructorFlow.expected @@ -1,5 +1,4 @@ testFailures -| Constructors.cs:113:25:113:43 | // ... | Missing result:hasValueFlow=6 | edges | Constructors.cs:5:24:5:25 | [post] this access : C_no_ctor [field s1] : Object | Constructors.cs:9:27:9:41 | object creation of type C_no_ctor : C_no_ctor [field s1] : Object | provenance | | | Constructors.cs:5:29:5:45 | call to method Source : Object | Constructors.cs:5:24:5:25 | [post] this access : C_no_ctor [field s1] : Object | provenance | | @@ -15,7 +14,9 @@ edges | Constructors.cs:33:18:33:19 | this access : C_with_ctor [field s1] : Object | Constructors.cs:33:18:33:19 | access to field s1 | provenance | | | Constructors.cs:41:26:41:26 | o : Object | Constructors.cs:41:38:41:38 | access to parameter o : Object | provenance | | | Constructors.cs:41:38:41:38 | access to parameter o : Object | Constructors.cs:41:32:41:34 | [post] this access : C1 [field Obj] : Object | provenance | | +| Constructors.cs:44:28:44:35 | o21param : Object | Constructors.cs:46:23:46:27 | this access : C2 [parameter o21param] : Object | provenance | | | Constructors.cs:44:28:44:35 | o21param : Object | Constructors.cs:46:31:46:38 | access to parameter o21param : Object | provenance | | +| Constructors.cs:46:23:46:27 | this access : C2 [parameter o21param] : Object | Constructors.cs:46:31:46:38 | access to parameter o21param : Object | provenance | | | Constructors.cs:46:31:46:38 | access to parameter o21param : Object | Constructors.cs:46:23:46:27 | [post] this access : C2 [field Obj21] : Object | provenance | | | Constructors.cs:48:32:48:39 | this : C2 [parameter o22param] : Object | Constructors.cs:48:32:48:39 | access to parameter o22param : Object | provenance | | | Constructors.cs:50:32:50:36 | this : C2 [field Obj21] : Object | Constructors.cs:50:32:50:36 | this access : C2 [field Obj21] : Object | provenance | | @@ -56,6 +57,13 @@ edges | Constructors.cs:100:25:100:29 | access to local variable taint : Object | Constructors.cs:100:9:100:10 | [post] access to local variable c2 : C2 [parameter o22param] : Object | provenance | | | Constructors.cs:101:14:101:15 | access to local variable c2 : C2 [parameter o22param] : Object | Constructors.cs:48:32:48:39 | this : C2 [parameter o22param] : Object | provenance | | | Constructors.cs:101:14:101:15 | access to local variable c2 : C2 [parameter o22param] : Object | Constructors.cs:101:14:101:21 | access to property Obj22 | provenance | | +| Constructors.cs:106:32:106:39 | this : C3 [parameter o31param] : Object | Constructors.cs:106:32:106:39 | access to parameter o31param : Object | provenance | | +| Constructors.cs:111:19:111:35 | call to method Source : Object | Constructors.cs:112:25:112:27 | access to local variable o31 : Object | provenance | | +| Constructors.cs:112:18:112:28 | object creation of type C3 : C3 [parameter o31param] : Object | Constructors.cs:113:14:113:15 | access to local variable c3 : C3 [parameter o31param] : Object | provenance | | +| Constructors.cs:112:25:112:27 | access to local variable o31 : Object | Constructors.cs:104:28:104:35 | o31param : Object | provenance | | +| Constructors.cs:112:25:112:27 | access to local variable o31 : Object | Constructors.cs:112:18:112:28 | object creation of type C3 : C3 [parameter o31param] : Object | provenance | | +| Constructors.cs:113:14:113:15 | access to local variable c3 : C3 [parameter o31param] : Object | Constructors.cs:106:32:106:39 | this : C3 [parameter o31param] : Object | provenance | | +| Constructors.cs:113:14:113:15 | access to local variable c3 : C3 [parameter o31param] : Object | Constructors.cs:113:14:113:21 | access to property Obj31 | provenance | | nodes | Constructors.cs:5:24:5:25 | [post] this access : C_no_ctor [field s1] : Object | semmle.label | [post] this access : C_no_ctor [field s1] : Object | | Constructors.cs:5:29:5:45 | call to method Source : Object | semmle.label | call to method Source : Object | @@ -77,6 +85,7 @@ nodes | Constructors.cs:44:28:44:35 | o21param : Object | semmle.label | o21param : Object | | Constructors.cs:44:45:44:52 | o22param : Object | semmle.label | o22param : Object | | Constructors.cs:46:23:46:27 | [post] this access : C2 [field Obj21] : Object | semmle.label | [post] this access : C2 [field Obj21] : Object | +| Constructors.cs:46:23:46:27 | this access : C2 [parameter o21param] : Object | semmle.label | this access : C2 [parameter o21param] : Object | | Constructors.cs:46:31:46:38 | access to parameter o21param : Object | semmle.label | access to parameter o21param : Object | | Constructors.cs:48:32:48:39 | access to parameter o22param : Object | semmle.label | access to parameter o22param : Object | | Constructors.cs:48:32:48:39 | this : C2 [parameter o22param] : Object | semmle.label | this : C2 [parameter o22param] : Object | @@ -117,6 +126,14 @@ nodes | Constructors.cs:100:25:100:29 | access to local variable taint : Object | semmle.label | access to local variable taint : Object | | Constructors.cs:101:14:101:15 | access to local variable c2 : C2 [parameter o22param] : Object | semmle.label | access to local variable c2 : C2 [parameter o22param] : Object | | Constructors.cs:101:14:101:21 | access to property Obj22 | semmle.label | access to property Obj22 | +| Constructors.cs:104:28:104:35 | o31param : Object | semmle.label | o31param : Object | +| Constructors.cs:106:32:106:39 | access to parameter o31param : Object | semmle.label | access to parameter o31param : Object | +| Constructors.cs:106:32:106:39 | this : C3 [parameter o31param] : Object | semmle.label | this : C3 [parameter o31param] : Object | +| Constructors.cs:111:19:111:35 | call to method Source : Object | semmle.label | call to method Source : Object | +| Constructors.cs:112:18:112:28 | object creation of type C3 : C3 [parameter o31param] : Object | semmle.label | object creation of type C3 : C3 [parameter o31param] : Object | +| Constructors.cs:112:25:112:27 | access to local variable o31 : Object | semmle.label | access to local variable o31 : Object | +| Constructors.cs:113:14:113:15 | access to local variable c3 : C3 [parameter o31param] : Object | semmle.label | access to local variable c3 : C3 [parameter o31param] : Object | +| Constructors.cs:113:14:113:21 | access to property Obj31 | semmle.label | access to property Obj31 | subpaths | Constructors.cs:64:37:64:37 | access to parameter o : Object | Constructors.cs:57:54:57:55 | o2 : Object | Constructors.cs:59:13:59:19 | SSA def(o1) : Object | Constructors.cs:64:27:64:34 | SSA def(o22param) : Object | | Constructors.cs:71:25:71:25 | access to local variable o : Object | Constructors.cs:41:26:41:26 | o : Object | Constructors.cs:41:32:41:34 | [post] this access : C1 [field Obj] : Object | Constructors.cs:71:18:71:26 | object creation of type C1 : C1 [field Obj] : Object | @@ -128,6 +145,8 @@ subpaths | Constructors.cs:93:14:93:15 | access to local variable c2 : C2 [parameter o22param] : Object | Constructors.cs:48:32:48:39 | this : C2 [parameter o22param] : Object | Constructors.cs:48:32:48:39 | access to parameter o22param : Object | Constructors.cs:93:14:93:21 | access to property Obj22 | | Constructors.cs:100:25:100:29 | access to local variable taint : Object | Constructors.cs:62:41:62:41 | o : Object | Constructors.cs:64:27:64:34 | SSA def(o22param) : Object | Constructors.cs:100:9:100:10 | [post] access to local variable c2 : C2 [parameter o22param] : Object | | Constructors.cs:101:14:101:15 | access to local variable c2 : C2 [parameter o22param] : Object | Constructors.cs:48:32:48:39 | this : C2 [parameter o22param] : Object | Constructors.cs:48:32:48:39 | access to parameter o22param : Object | Constructors.cs:101:14:101:21 | access to property Obj22 | +| Constructors.cs:112:25:112:27 | access to local variable o31 : Object | Constructors.cs:104:28:104:35 | o31param : Object | Constructors.cs:104:28:104:35 | o31param : Object | Constructors.cs:112:18:112:28 | object creation of type C3 : C3 [parameter o31param] : Object | +| Constructors.cs:113:14:113:15 | access to local variable c3 : C3 [parameter o31param] : Object | Constructors.cs:106:32:106:39 | this : C3 [parameter o31param] : Object | Constructors.cs:106:32:106:39 | access to parameter o31param : Object | Constructors.cs:113:14:113:21 | access to property Obj31 | #select | Constructors.cs:15:18:15:19 | access to field s1 | Constructors.cs:5:29:5:45 | call to method Source : Object | Constructors.cs:15:18:15:19 | access to field s1 | $@ | Constructors.cs:5:29:5:45 | call to method Source : Object | call to method Source : Object | | Constructors.cs:33:18:33:19 | access to field s1 | Constructors.cs:21:29:21:45 | call to method Source : Object | Constructors.cs:33:18:33:19 | access to field s1 | $@ | Constructors.cs:21:29:21:45 | call to method Source : Object | call to method Source : Object | @@ -137,3 +156,4 @@ subpaths | Constructors.cs:82:14:82:21 | access to property Obj23 | Constructors.cs:77:19:77:35 | call to method Source : Object | Constructors.cs:82:14:82:21 | access to property Obj23 | $@ | Constructors.cs:77:19:77:35 | call to method Source : Object | call to method Source : Object | | Constructors.cs:93:14:93:21 | access to property Obj22 | Constructors.cs:91:21:91:37 | call to method Source : Object | Constructors.cs:93:14:93:21 | access to property Obj22 | $@ | Constructors.cs:91:21:91:37 | call to method Source : Object | call to method Source : Object | | Constructors.cs:101:14:101:21 | access to property Obj22 | Constructors.cs:99:21:99:37 | call to method Source : Object | Constructors.cs:101:14:101:21 | access to property Obj22 | $@ | Constructors.cs:99:21:99:37 | call to method Source : Object | call to method Source : Object | +| Constructors.cs:113:14:113:21 | access to property Obj31 | Constructors.cs:111:19:111:35 | call to method Source : Object | Constructors.cs:113:14:113:21 | access to property Obj31 | $@ | Constructors.cs:111:19:111:35 | call to method Source : Object | call to method Source : Object | diff --git a/csharp/ql/test/library-tests/expressions/ConstructorInitializers.expected b/csharp/ql/test/library-tests/expressions/ConstructorInitializers.expected index 9f65e360ec0..d6224193f87 100644 --- a/csharp/ql/test/library-tests/expressions/ConstructorInitializers.expected +++ b/csharp/ql/test/library-tests/expressions/ConstructorInitializers.expected @@ -6,3 +6,5 @@ | expressions.cs:271:16:271:24 | IntVector | expressions.cs:271:16:271:24 | call to constructor Object | file://:0:0:0:0 | Object | | expressions.cs:311:16:311:20 | Digit | expressions.cs:311:16:311:20 | call to constructor ValueType | file://:0:0:0:0 | ValueType | | expressions.cs:481:20:481:22 | Num | expressions.cs:481:20:481:22 | call to constructor Object | file://:0:0:0:0 | Object | +| expressions.cs:518:11:518:17 | ClassC1 | expressions.cs:518:11:518:17 | call to constructor Object | file://:0:0:0:0 | Object | +| expressions.cs:520:11:520:17 | ClassC2 | expressions.cs:520:33:520:44 | call to constructor ClassC1 | expressions.cs:518:11:518:17 | ClassC1 | diff --git a/csharp/ql/test/library-tests/expressions/PrintAst.expected b/csharp/ql/test/library-tests/expressions/PrintAst.expected index c1d146f8eef..e865a36d549 100644 --- a/csharp/ql/test/library-tests/expressions/PrintAst.expected +++ b/csharp/ql/test/library-tests/expressions/PrintAst.expected @@ -2418,3 +2418,5 @@ expressions.cs: #-----| 2: (Parameters) # 520| 0: [Parameter] oc2 # 520| -1: [TypeMention] object +# 520| 3: [ConstructorInitializer] call to constructor ClassC1 +# 520| 0: [ParameterAccess] access to parameter oc2 From 28d5c11b6fc31766e4e5c2aae08e7f03af21b126 Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Fri, 16 Feb 2024 15:30:38 +0100 Subject: [PATCH 5/8] C#: Synthesize an empty body for primary constructors. --- .../Entities/Constructor.cs | 6 +++++ .../Statements/SyntheticEmptyBlock.cs | 24 +++++++++++++++++++ csharp/ql/lib/semmle/code/csharp/Callable.qll | 4 +++- 3 files changed, 33 insertions(+), 1 deletion(-) create mode 100644 csharp/extractor/Semmle.Extraction.CSharp/Entities/Statements/SyntheticEmptyBlock.cs diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Constructor.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Constructor.cs index 58c689b8e70..e7096769573 100644 --- a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Constructor.cs +++ b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Constructor.cs @@ -32,6 +32,12 @@ namespace Semmle.Extraction.CSharp.Entities trapFile.constructors(this, Symbol.ContainingType.Name, ContainingType, (Constructor)OriginalDefinition); trapFile.constructor_location(this, Location); + if (IsPrimary) + { + // Create a synthetic empty body for primary constructors. + Statements.SyntheticEmptyBlock.Create(Context, this, 0, Location); + } + if (Symbol.IsImplicitlyDeclared) { var lineCounts = new LineCounts() { Total = 2, Code = 1, Comment = 0 }; diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Statements/SyntheticEmptyBlock.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Statements/SyntheticEmptyBlock.cs new file mode 100644 index 00000000000..0b292d208ff --- /dev/null +++ b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Statements/SyntheticEmptyBlock.cs @@ -0,0 +1,24 @@ +using System.IO; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Semmle.Extraction.Entities; +using Semmle.Extraction.Kinds; + +namespace Semmle.Extraction.CSharp.Entities.Statements +{ + internal class SyntheticEmptyBlock : Statement + { + private SyntheticEmptyBlock(Context cx, BlockSyntax block, IStatementParentEntity parent, int child, Location location) + : base(cx, block, StmtKind.BLOCK, parent, child, location) { } + + public static SyntheticEmptyBlock Create(Context cx, IStatementParentEntity parent, int child, Location location) + { + var block = SyntaxFactory.Block(); + var ret = new SyntheticEmptyBlock(cx, block, parent, child, location); + ret.TryPopulate(); + return ret; + } + + protected override void PopulateStatement(TextWriter trapFile) { } + } +} diff --git a/csharp/ql/lib/semmle/code/csharp/Callable.qll b/csharp/ql/lib/semmle/code/csharp/Callable.qll index be42d84d7de..89f29458cc9 100644 --- a/csharp/ql/lib/semmle/code/csharp/Callable.qll +++ b/csharp/ql/lib/semmle/code/csharp/Callable.qll @@ -416,7 +416,9 @@ class InstanceConstructor extends Constructor { */ class PrimaryConstructor extends Constructor { PrimaryConstructor() { - not this.hasBody() and + // In the extractor we use the constructor location as the location for the + // synthesized empty body of the constructor. + this.getLocation() = this.getBody().getLocation() and this.getDeclaringType().fromSource() and this.fromSource() } From d83687125cd8c50db9ba5c13cfb3b393080393ed Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Mon, 19 Feb 2024 10:47:59 +0100 Subject: [PATCH 6/8] C#: Add postupdate nodes for all instance parameter accesses - otherwise we get missing post update nodes to to reverseRead in the data flow consistency queries. --- .../code/csharp/dataflow/internal/DataFlowPrivate.qll | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowPrivate.qll b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowPrivate.qll index acdae06c8b0..af2c6354080 100644 --- a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowPrivate.qll +++ b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowPrivate.qll @@ -953,12 +953,8 @@ private module Cached { callCfn = any(Call c | isParamsArg(c, _, _)).getAControlFlowNode() } or TFlowInsensitiveFieldNode(FieldOrProperty f) { f.isFieldLike() } or - TInstanceParameterAccessNode(ControlFlow::Node cfn, boolean isPostUpdate) { - exists(ParameterAccess pa | cfn = getAPrimaryConstructorParameterCfn(pa) | - isPostUpdate = false - or - pa instanceof ParameterWrite and isPostUpdate = true - ) + TInstanceParameterAccessNode(ControlFlow::Node cfn, Boolean isPostUpdate) { + cfn = getAPrimaryConstructorParameterCfn(_) } or TPrimaryConstructorThisAccessNode(Parameter p, Boolean isPostUpdate) { p.getCallable() instanceof PrimaryConstructor From feda6bc01bcb0f2adc1cebcfa3b5ea71649b14b9 Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Fri, 16 Feb 2024 15:30:57 +0100 Subject: [PATCH 7/8] C#: Update expected test output. --- .../constructors/PrintAst.expected | 5 +++++ .../library-tests/csharp9/PrintAst.expected | 17 +++++++++++++++++ .../dataflow/tuples/DataFlowStep.expected | 3 +++ .../dataflow/tuples/PrintAst.expected | 1 + .../library-tests/expressions/PrintAst.expected | 2 ++ 5 files changed, 28 insertions(+) diff --git a/csharp/ql/test/library-tests/constructors/PrintAst.expected b/csharp/ql/test/library-tests/constructors/PrintAst.expected index bd949312d8f..c917d5c7fa1 100644 --- a/csharp/ql/test/library-tests/constructors/PrintAst.expected +++ b/csharp/ql/test/library-tests/constructors/PrintAst.expected @@ -25,6 +25,7 @@ constructors.cs: # 23| -1: [TypeMention] object # 23| 1: [Parameter] s # 23| -1: [TypeMention] string +# 23| 4: [BlockStmt] {...} # 25| 5: [InstanceConstructor] C1 #-----| 2: (Parameters) # 25| 0: [Parameter] o @@ -44,3 +45,7 @@ constructors.cs: # 28| -1: [TypeMention] string # 28| 2: [Parameter] i # 28| -1: [TypeMention] int +# 28| 3: [ConstructorInitializer] call to constructor C1 +# 28| 0: [ParameterAccess] access to parameter o +# 28| 1: [ParameterAccess] access to parameter s +# 28| 4: [BlockStmt] {...} diff --git a/csharp/ql/test/library-tests/csharp9/PrintAst.expected b/csharp/ql/test/library-tests/csharp9/PrintAst.expected index 7bc97c98d70..a46ffa3257b 100644 --- a/csharp/ql/test/library-tests/csharp9/PrintAst.expected +++ b/csharp/ql/test/library-tests/csharp9/PrintAst.expected @@ -884,6 +884,7 @@ Record.cs: # 27| -1: [TypeMention] string # 27| 1: [Parameter] LastName # 27| -1: [TypeMention] string +# 27| 4: [BlockStmt] {...} # 27| 16: [Property] FirstName # 27| 3: [Getter] get_FirstName # 27| 4: [Setter] set_FirstName @@ -913,6 +914,10 @@ Record.cs: # 29| -1: [TypeMention] string # 29| 2: [Parameter] Subject # 29| -1: [TypeMention] string +# 30| 3: [ConstructorInitializer] call to constructor Person1 +# 30| 0: [ParameterAccess] access to parameter FirstName +# 30| 1: [ParameterAccess] access to parameter LastName +# 29| 4: [BlockStmt] {...} # 29| 17: [Property] Subject # 29| 3: [Getter] get_Subject # 29| 4: [Setter] set_Subject @@ -937,6 +942,10 @@ Record.cs: # 32| -1: [TypeMention] string # 32| 2: [Parameter] Level # 32| -1: [TypeMention] int +# 33| 3: [ConstructorInitializer] call to constructor Person1 +# 33| 0: [ParameterAccess] access to parameter FirstName +# 33| 1: [ParameterAccess] access to parameter LastName +# 32| 4: [BlockStmt] {...} # 32| 17: [Property] Level # 32| 3: [Getter] get_Level # 32| 4: [Setter] set_Level @@ -957,6 +966,7 @@ Record.cs: #-----| 2: (Parameters) # 35| 0: [Parameter] Name # 35| -1: [TypeMention] string +# 35| 4: [BlockStmt] {...} # 35| 16: [Property] Name # 35| 3: [Getter] get_Name # 35| 4: [Setter] set_Name @@ -981,6 +991,9 @@ Record.cs: #-----| 2: (Parameters) # 41| 0: [Parameter] Name # 41| -1: [TypeMention] string +# 41| 3: [ConstructorInitializer] call to constructor Pet +# 41| 0: [ParameterAccess] access to parameter Name +# 41| 4: [BlockStmt] {...} # 41| 15: [Property] EqualityContract # 41| 3: [Getter] get_EqualityContract # 43| 16: [Method] WagTail @@ -1022,6 +1035,7 @@ Record.cs: #-----| 2: (Parameters) # 54| 0: [Parameter] A # 54| -1: [TypeMention] string +# 54| 4: [BlockStmt] {...} # 54| 16: [Property] A # 54| 3: [Getter] get_A # 54| 4: [Setter] set_A @@ -1044,6 +1058,9 @@ Record.cs: # 56| -1: [TypeMention] string # 56| 1: [Parameter] B # 56| -1: [TypeMention] string +# 56| 3: [ConstructorInitializer] call to constructor R1 +# 56| 0: [ParameterAccess] access to parameter A +# 56| 4: [BlockStmt] {...} # 56| 17: [Property] B # 56| 3: [Getter] get_B # 56| 4: [Setter] set_B diff --git a/csharp/ql/test/library-tests/dataflow/tuples/DataFlowStep.expected b/csharp/ql/test/library-tests/dataflow/tuples/DataFlowStep.expected index bf41a804fdb..7a3c03fb060 100644 --- a/csharp/ql/test/library-tests/dataflow/tuples/DataFlowStep.expected +++ b/csharp/ql/test/library-tests/dataflow/tuples/DataFlowStep.expected @@ -135,6 +135,9 @@ | Tuples.cs:87:27:87:27 | SSA def(q) | Tuples.cs:91:18:91:18 | access to local variable q | | Tuples.cs:87:30:87:30 | SSA def(r) | Tuples.cs:90:18:90:18 | access to local variable r | | Tuples.cs:91:18:91:18 | access to local variable q | Tuples.cs:91:18:91:18 | (...) ... | +| Tuples.cs:95:12:95:13 | this | Tuples.cs:95:22:95:22 | this | +| Tuples.cs:95:22:95:22 | [post] this | Tuples.cs:95:29:95:29 | this | +| Tuples.cs:95:22:95:22 | this | Tuples.cs:95:29:95:29 | this | | Tuples.cs:99:13:99:33 | SSA def(o) | Tuples.cs:100:24:100:24 | access to local variable o | | Tuples.cs:99:17:99:33 | call to method Source | Tuples.cs:99:13:99:33 | SSA def(o) | | Tuples.cs:99:32:99:32 | 9 | Tuples.cs:99:32:99:32 | (...) ... | diff --git a/csharp/ql/test/library-tests/dataflow/tuples/PrintAst.expected b/csharp/ql/test/library-tests/dataflow/tuples/PrintAst.expected index 7cbddb9d3d5..97fb24fb239 100644 --- a/csharp/ql/test/library-tests/dataflow/tuples/PrintAst.expected +++ b/csharp/ql/test/library-tests/dataflow/tuples/PrintAst.expected @@ -369,6 +369,7 @@ Tuples.cs: # 95| -1: [TypeMention] string # 95| 1: [Parameter] j # 95| -1: [TypeMention] int +# 95| 4: [BlockStmt] {...} # 95| 16: [Property] i # 95| 3: [Getter] get_i # 95| 4: [Setter] set_i diff --git a/csharp/ql/test/library-tests/expressions/PrintAst.expected b/csharp/ql/test/library-tests/expressions/PrintAst.expected index e865a36d549..d6e677602e3 100644 --- a/csharp/ql/test/library-tests/expressions/PrintAst.expected +++ b/csharp/ql/test/library-tests/expressions/PrintAst.expected @@ -2411,6 +2411,7 @@ expressions.cs: #-----| 2: (Parameters) # 518| 0: [Parameter] oc1 # 518| -1: [TypeMention] object +# 518| 4: [BlockStmt] {...} # 520| 23: [Class] ClassC2 #-----| 3: (Base types) # 520| 0: [TypeMention] ClassC1 @@ -2420,3 +2421,4 @@ expressions.cs: # 520| -1: [TypeMention] object # 520| 3: [ConstructorInitializer] call to constructor ClassC1 # 520| 0: [ParameterAccess] access to parameter oc2 +# 520| 4: [BlockStmt] {...} From f246272b5fd5ebb49023c0eaadba72e719ab6101 Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Tue, 20 Feb 2024 11:48:01 +0100 Subject: [PATCH 8/8] C#: Code quality improvements. --- .../Semmle.Extraction.CSharp/Entities/Constructor.cs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Constructor.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Constructor.cs index e7096769573..039ff7d9fdb 100644 --- a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Constructor.cs +++ b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Constructor.cs @@ -12,15 +12,15 @@ namespace Semmle.Extraction.CSharp.Entities { internal class Constructor : Method { - private readonly Lazy> DeclaringReferenceSyntax; + private readonly List declaringReferenceSyntax; private Constructor(Context cx, IMethodSymbol init) : base(cx, init) { - DeclaringReferenceSyntax = new(() => + declaringReferenceSyntax = Symbol.DeclaringSyntaxReferences .Select(r => r.GetSyntax()) - .ToList()); + .ToList(); } public override void Populate(TextWriter trapFile) @@ -143,12 +143,12 @@ namespace Semmle.Extraction.CSharp.Entities } private ConstructorDeclarationSyntax? OrdinaryConstructorSyntax => - DeclaringReferenceSyntax.Value + declaringReferenceSyntax .OfType() .FirstOrDefault(); private TypeDeclarationSyntax? PrimaryConstructorSyntax => - DeclaringReferenceSyntax.Value + declaringReferenceSyntax .OfType() .FirstOrDefault(t => t is ClassDeclarationSyntax or StructDeclarationSyntax or RecordDeclarationSyntax);