[BugFix] Break recursion in GetTypeInfo (#4415)

* Break recursion in GetTypeInfo * Algo fix and test added * Fixed c++ warning * Fix ASM instrumentations being enabled when they should not * Improved RegisterCallTarget traces * Add smoke tests for deeply nested hierarchies * Add net462 support to DeepNestedHierachy test * Minor fix and perf improvement --------- Co-authored-by: Andrew Lock <[email protected]>
DataDog · Jul 20, 2023 · c64aef8 · c64aef8
1 parent cc4f12b
commit c64aef8
Show file tree

Hide file tree

Showing 14 changed files with 933 additions and 68 deletions.
diff --git a/Datadog.Trace.sln b/Datadog.Trace.sln
@@ -526,6 +526,8 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Samples.Probes.Unreferenced
 EndProject
 Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Sandbox.LegacySecurityPolicy", "tracer\test\test-applications\regression\Sandbox.LegacySecurityPolicy\Sandbox.LegacySecurityPolicy.csproj", "{959E9599-8D99-43BC-8038-B91F76179C1C}"
 EndProject
+Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "DeepNestedHierarchy", "tracer\test\test-applications\regression\DeepNestedHierarchy\DeepNestedHierarchy.csproj", "{1B3E6BEE-F7AB-433E-A1D9-E8BE3782419B}"
+EndProject
 Global
 	GlobalSection(SolutionConfigurationPlatforms) = preSolution
 		Debug|Any CPU = Debug|Any CPU
@@ -1246,6 +1248,10 @@ Global
 		{959E9599-8D99-43BC-8038-B91F76179C1C}.Debug|Any CPU.Build.0 = Debug|Any CPU
 		{959E9599-8D99-43BC-8038-B91F76179C1C}.Release|Any CPU.ActiveCfg = Release|Any CPU
 		{959E9599-8D99-43BC-8038-B91F76179C1C}.Release|Any CPU.Build.0 = Release|Any CPU
+		{1B3E6BEE-F7AB-433E-A1D9-E8BE3782419B}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
+		{1B3E6BEE-F7AB-433E-A1D9-E8BE3782419B}.Debug|Any CPU.Build.0 = Debug|Any CPU
+		{1B3E6BEE-F7AB-433E-A1D9-E8BE3782419B}.Release|Any CPU.ActiveCfg = Release|Any CPU
+		{1B3E6BEE-F7AB-433E-A1D9-E8BE3782419B}.Release|Any CPU.Build.0 = Release|Any CPU
 	EndGlobalSection
 	GlobalSection(SolutionProperties) = preSolution
 		HideSolutionNode = FALSE
@@ -1449,6 +1455,7 @@ Global
 		{51AF3B29-D107-4E22-8A85-DF02A8A898E5} = {8683D82A-2BBE-4199-9C36-C59F48804F90}
 		{DE15D292-DC01-4AEA-B473-C2F2908341A7} = {0884B566-D22E-498C-BAA9-26D50ABCAE3A}
 		{959E9599-8D99-43BC-8038-B91F76179C1C} = {498A300E-D036-49B7-A43D-821D1CAF11A5}
+		{1B3E6BEE-F7AB-433E-A1D9-E8BE3782419B} = {498A300E-D036-49B7-A43D-821D1CAF11A5}
 	EndGlobalSection
 	GlobalSection(ExtensibilityGlobals) = postSolution
 		SolutionGuid = {160A1D00-1F5B-40F8-A155-621B4459D78F}

diff --git a/_launch_test.ps1 b/_launch_test.ps1
@@ -0,0 +1,7 @@
+$n = 0
+While($n -lt 100)
+{
+#nuke runmanagedunittests --verbosity normal -filter Waf
+dotnet test --no-restore --configuration release
+$n++
+}
diff --git a/tracer/src/Datadog.Trace/ClrProfiler/Instrumentation.cs b/tracer/src/Datadog.Trace/ClrProfiler/Instrumentation.cs
@@ -110,7 +110,7 @@ public static void Initialize()
                     Log.Debug("Enabling CallTarget integration definitions in native library.");
 
                     InstrumentationCategory enabledCategories = InstrumentationCategory.Tracing;
-                    if (Security.Instance.Enabled || Security.Instance.Settings.CanBeToggled)
+                    if (Security.Instance.Enabled)
                     {
                         Log.Debug("Enabling AppSec call target category");
                         enabledCategories |= InstrumentationCategory.AppSec;

diff --git a/tracer/src/Datadog.Tracer.Native/clr_helpers.cpp b/tracer/src/Datadog.Tracer.Native/clr_helpers.cpp
@@ -215,7 +215,7 @@ ModuleInfo GetModuleInfo(ICorProfilerInfo4* info, const ModuleID& module_id)
     return {module_id, shared::WSTRING(module_path), GetAssemblyInfo(info, assembly_id), module_flags};
 }
 
-TypeInfo GetTypeInfo(const ComPtr<IMetaDataImport2>& metadata_import, const mdToken& token)
+TypeInfo GetTypeInfo(const ComPtr<IMetaDataImport2>& metadata_import, const mdToken& token_)
 {
     mdToken parent_token = mdTokenNil;
     std::shared_ptr<TypeInfo> parentTypeInfo = nullptr;
@@ -231,83 +231,97 @@ TypeInfo GetTypeInfo(const ComPtr<IMetaDataImport2>& metadata_import, const mdTo
     bool type_isSealed = false;
 
     HRESULT hr = E_FAIL;
-    const auto token_type = TypeFromToken(token);
 
-    switch (token_type)
+    auto token = token_;
+    auto typeSpec = mdTypeSpecNil;
+    std::set<mdToken> processed;
+
+    while (token != mdTokenNil)
     {
-        case mdtTypeDef:
-            hr = metadata_import->GetTypeDefProps(token, type_name, kNameMaxSize, &type_name_len, &type_flags,
-                                                  &type_extends);
+        const auto token_type = TypeFromToken(token);
+        switch (token_type)
+        {
+            case mdtTypeDef:
+                hr = metadata_import->GetTypeDefProps(token, type_name, kNameMaxSize, &type_name_len, &type_flags,
+                                                      &type_extends);
 
-            metadata_import->GetNestedClassProps(token, &parent_type_token);
-            if (parent_type_token != mdTokenNil)
-            {
-                parentTypeInfo = std::make_shared<TypeInfo>(GetTypeInfo(metadata_import, parent_type_token));
-            }
+                metadata_import->GetNestedClassProps(token, &parent_type_token);
+                if (parent_type_token != mdTokenNil)
+                {
+                    parentTypeInfo = std::make_shared<TypeInfo>(GetTypeInfo(metadata_import, parent_type_token));
+                }
+
+                if (type_extends != mdTokenNil)
+                {
+                    extendsInfo = std::make_shared<TypeInfo>(GetTypeInfo(metadata_import, type_extends));
+                    type_valueType =
+                        extendsInfo->name == WStr("System.ValueType") || extendsInfo->name == WStr("System.Enum");
+                }
+
+                type_isAbstract = IsTdAbstract(type_flags);
+                type_isSealed = IsTdSealed(type_flags);
 
-            if (type_extends != mdTokenNil)
+                break;
+            case mdtTypeRef:
+                hr = metadata_import->GetTypeRefProps(token, &parent_token, type_name, kNameMaxSize, &type_name_len);
+                break;
+            case mdtTypeSpec:
             {
-                extendsInfo = std::make_shared<TypeInfo>(GetTypeInfo(metadata_import, type_extends));
-                type_valueType =
-                    extendsInfo->name == WStr("System.ValueType") || extendsInfo->name == WStr("System.Enum");
-            }
+                PCCOR_SIGNATURE signature{};
+                ULONG signature_length{};
 
-            type_isAbstract = IsTdAbstract(type_flags);
-            type_isSealed = IsTdSealed(type_flags);
+                hr = metadata_import->GetTypeSpecFromToken(token, &signature, &signature_length);
 
-            break;
-        case mdtTypeRef:
-            hr = metadata_import->GetTypeRefProps(token, &parent_token, type_name, kNameMaxSize, &type_name_len);
-            break;
-        case mdtTypeSpec:
-        {
-            PCCOR_SIGNATURE signature{};
-            ULONG signature_length{};
+                if (FAILED(hr) || signature_length < 3)
+                {
+                    return {};
+                }
 
-            hr = metadata_import->GetTypeSpecFromToken(token, &signature, &signature_length);
+                if (signature[0] & ELEMENT_TYPE_GENERICINST)
+                {
+                    if (std::find(processed.begin(), processed.end(), token) != processed.end())
+                    {
+                        return {}; // Break circular reference
+                    }
+                    processed.insert(token);
 
-            if (FAILED(hr) || signature_length < 3)
-            {
-                return {};
+                    mdToken type_token;
+                    CorSigUncompressToken(&signature[2], &type_token);
+                    typeSpec = token;
+                    token = type_token;
+                    continue;
+                }
             }
+            break;
+            case mdtModuleRef:
+                metadata_import->GetModuleRefProps(token, type_name, kNameMaxSize, &type_name_len);
+                break;
+            case mdtMemberRef:
+                return GetFunctionInfo(metadata_import, token).type;
+                break;
+            case mdtMethodDef:
+                return GetFunctionInfo(metadata_import, token).type;
+                break;
+        }
 
-            if (signature[0] & ELEMENT_TYPE_GENERICINST)
-            {
-                mdToken type_token;
-                CorSigUncompressToken(&signature[2], &type_token);
-                const auto baseType = GetTypeInfo(metadata_import, type_token);
-                return {baseType.id,        baseType.name,        token,
-                        token_type,         baseType.extend_from, baseType.valueType,
-                        baseType.isGeneric, baseType.isAbstract, baseType.isSealed,
-                        baseType.parent_type, baseType.scopeToken};
-            }
+        if (FAILED(hr) || type_name_len == 0)
+        {
+            return {};
         }
-        break;
-        case mdtModuleRef:
-            metadata_import->GetModuleRefProps(token, type_name, kNameMaxSize, &type_name_len);
-            break;
-        case mdtMemberRef:
-            return GetFunctionInfo(metadata_import, token).type;
-            break;
-        case mdtMethodDef:
-            return GetFunctionInfo(metadata_import, token).type;
-            break;
-    }
-    if (FAILED(hr) || type_name_len == 0)
-    {
-        return {};
-    }
 
-    const auto type_name_string = shared::WSTRING(type_name);
-    const auto generic_token_index = type_name_string.rfind(WStr("`"));
-    if (generic_token_index != std::string::npos)
-    {
-        const auto idxFromRight = type_name_string.length() - generic_token_index - 1;
-        type_isGeneric = idxFromRight == 1 || idxFromRight == 2;
-    }
+        const auto type_name_string = shared::WSTRING(type_name);
+        const auto generic_token_index = type_name_string.rfind(WStr("`"));
+        if (generic_token_index != std::string::npos)
+        {
+            const auto idxFromRight = type_name_string.length() - generic_token_index - 1;
+            type_isGeneric = idxFromRight == 1 || idxFromRight == 2;
+        }
 
-    return {token,       type_name_string, mdTypeSpecNil,  token_type,
-            extendsInfo, type_valueType,   type_isGeneric, type_isAbstract, type_isSealed, parentTypeInfo, parent_token};
+        return {token,         type_name_string, typeSpec,       typeSpec != mdTypeSpecNil ? mdtTypeSpec : token_type,
+                extendsInfo,   type_valueType,   type_isGeneric, type_isAbstract,
+                type_isSealed, parentTypeInfo,   parent_token};
+    }
+    return {};
 }
 
 // Searches for an AssemblyRef whose name and version match exactly.

diff --git a/tracer/src/Datadog.Tracer.Native/clr_helpers.h b/tracer/src/Datadog.Tracer.Native/clr_helpers.h
@@ -178,6 +178,16 @@ static Enumerator<mdAssemblyRef> EnumAssemblyRefs(const ComPtr<IMetaDataAssembly
         [assembly_import](HCORENUM ptr) -> void { assembly_import->CloseEnum(ptr); });
 }
 
+static Enumerator<mdTypeSpec> EnumTypeSpecs(const ComPtr<IMetaDataImport2>& metadata_import)
+{
+    return Enumerator<mdTypeSpec>(
+        [metadata_import](HCORENUM* ptr, mdTypeSpec arr[], ULONG max, ULONG* cnt) -> HRESULT {
+            return metadata_import->EnumTypeSpecs(ptr, arr, max, cnt);
+        },
+        [metadata_import](HCORENUM ptr) -> void { metadata_import->CloseEnum(ptr); });
+}
+
+
 static Enumerator<mdInterfaceImpl> EnumInterfaceImpls(const ComPtr<IMetaDataImport2>& metadata_import, const mdTypeDef typeDef)
 {
     return Enumerator<mdInterfaceImpl>(

diff --git a/tracer/src/Datadog.Tracer.Native/cor_profiler.cpp b/tracer/src/Datadog.Tracer.Native/cor_profiler.cpp
@@ -1818,6 +1818,7 @@ void CorProfiler::InternalAddInstrumentation(WCHAR* id, CallTargetDefinition* it
 long CorProfiler::RegisterCallTargetDefinitions(WCHAR* id, CallTargetDefinition2* items, int size, UINT32 enabledCategories)
 {
     long numReJITs = 0;
+    long enabledTargets = 0;
     shared::WSTRING definitionsId = shared::WSTRING(id);
     auto definitions = definitions_ids.Get();
 
@@ -1862,11 +1863,22 @@ long CorProfiler::RegisterCallTargetDefinitions(WCHAR* id, CallTargetDefinition2
                 TypeReference(integrationAssembly, integrationType, {}, {}), current.GetIsDerived(),
                 current.GetIsInterface(), true, current.categories, enabledCategories);
 
+            if (integration.GetEnabled())
+            {
+                enabledTargets++;
+            }
+
             if (Logger::IsDebugEnabled())
             {
+                std::string kind = current.GetIsDerived() ? "DERIVED" : "DEFAULT";
+                if (current.GetIsInterface())
+                {
+                    kind += " INTERFACE";
+                }
                 Logger::Debug("  * Target: ", targetAssembly, " | ", targetType, ".", targetMethod, "(",
                               signatureTypes.size(), ") { ", minVersion.str(), " - ", maxVersion.str(), " } [",
-                              integrationAssembly, " | ", integrationType, "]");
+                              integrationAssembly, " | ", integrationType, " | kind: ", kind, " | categories: ", current.categories,
+                              integration.GetEnabled() ? " ENABLED " : " DISABLED ", "]");
             }
 
             integrationDefinitions.push_back(integration);
@@ -1892,10 +1904,11 @@ long CorProfiler::RegisterCallTargetDefinitions(WCHAR* id, CallTargetDefinition2
             Logger::Debug("Total number of ReJIT Requested: ", numReJITs);
         }
 
-        Logger::Info("RegisterCallTargetDefinitions: Total integrations in profiler: ", integration_definitions_.size());
+        Logger::Info("RegisterCallTargetDefinitions: Added ", size, " call targets (enabled: ", enabledTargets,
+                      ", enabled categories: ", enabledCategories ,") ");
     }
 
-    return numReJITs;
+    return enabledTargets;
 }
 long CorProfiler::EnableCallTargetDefinitions(UINT32 enabledCategories)
 {

diff --git a/...est/Datadog.Trace.ClrProfiler.IntegrationTests/SmokeTests/DeepNestedHierarchySmokeTest.cs b/...est/Datadog.Trace.ClrProfiler.IntegrationTests/SmokeTests/DeepNestedHierarchySmokeTest.cs
@@ -0,0 +1,24 @@
+// <copyright file="DeepNestedHierarchySmokeTest.cs" company="Datadog">
+// Unless explicitly stated otherwise all files in this repository are licensed under the Apache 2 License.
+// This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2017 Datadog, Inc.
+// </copyright>
+
+using Xunit;
+using Xunit.Abstractions;
+
+namespace Datadog.Trace.ClrProfiler.IntegrationTests.SmokeTests;
+
+public class DeepNestedHierarchySmokeTest : SmokeTestBase
+{
+    public DeepNestedHierarchySmokeTest(ITestOutputHelper output)
+        : base(output, "DeepNestedHierarchy")
+    {
+    }
+
+    [SkippableFact]
+    [Trait("Category", "Smoke")]
+    public void NoExceptions()
+    {
+        CheckForSmoke(shouldDeserializeTraces: false);
+    }
+}
diff --git a/tracer/test/Datadog.Trace.Security.Unit.Tests/_launch_test.ps1 b/tracer/test/Datadog.Trace.Security.Unit.Tests/_launch_test.ps1
@@ -0,0 +1,7 @@
+$n = 0
+While($n -lt 100)
+{
+#nuke runmanagedunittests --verbosity normal -filter Waf
+dotnet test --no-restore --configuration release
+$n++
+}
diff --git a/tracer/test/Datadog.Tracer.Native.Tests/clr_helper_test.cpp b/tracer/test/Datadog.Tracer.Native.Tests/clr_helper_test.cpp
@@ -147,6 +147,33 @@ TEST_F(CLRHelperTest, GetsTypeInfoFromTypeRefs) {
   EXPECT_EQ(expected, actual);
 }
 
+TEST_F(CLRHelperTest, GetsTypeInfoFromTypeSpecs)
+{
+    std::set<shared::WSTRING> expected = {
+	WStr("<StayAndLayDown>d__4`2"), 
+	WStr("Samples.ExampleLibrary.Class1"), 
+	WStr("Samples.ExampleLibrary.FakeClient.Biscuit`1"), 
+	WStr("Samples.ExampleLibrary.FakeClient.DogClient`2"), 
+	WStr("Samples.ExampleLibrary.FakeClient.DogTrick`1"), 
+	WStr("Samples.ExampleLibrary.GenericTests.GenericTarget`2"), 
+	WStr("Samples.ExampleLibrary.GenericTests.StructContainer`1"),
+	WStr("System.Collections.Generic.List`1"),
+	WStr("System.Func`3"),
+	WStr("System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1"),
+	WStr("System.Runtime.CompilerServices.TaskAwaiter`1"),
+	WStr("System.Threading.Tasks.Task`1") };
+  std::set<shared::WSTRING> actual;
+  for (auto& type_def : EnumTypeSpecs(metadata_import_))
+  {
+    auto type_info = GetTypeInfo(metadata_import_, type_def);
+    if (type_info.IsValid())
+    {
+      actual.insert(type_info.name);
+    }
+  }
+  EXPECT_EQ(actual, expected);
+}
+
 TEST_F(CLRHelperTest, GetsTypeInfoFromModuleRefs) {
   // TODO(cbd): figure out how to create a module ref, for now its empty
   std::set<shared::WSTRING> expected = {};

diff --git a/tracer/test/test-applications/regression/DeepNestedHierarchy/DeepNestedHierarchy.csproj b/tracer/test/test-applications/regression/DeepNestedHierarchy/DeepNestedHierarchy.csproj
@@ -0,0 +1,13 @@
+<Project Sdk="Microsoft.NET.Sdk.Web">
+
+  <PropertyGroup>
+    <TargetFrameworks>net462;netcoreapp2.1;netcoreapp3.0;netcoreapp3.1;net5.0;net6.0;net7.0</TargetFrameworks>
+  </PropertyGroup>
+
+  <ItemGroup Condition="$(TargetFramework) == 'netcoreapp2.1' OR $(TargetFramework) == 'netcoreapp3.0'">
+    <PackageReference Include="Microsoft.AspNetCore.OData" Version="7.7.0" />
+  </ItemGroup>
+  <ItemGroup Condition="$(TargetFramework) != 'netcoreapp2.1' AND $(TargetFramework) != 'netcoreapp3.0' AND $(TargetFramework) != 'net462'">
+    <PackageReference Include="Microsoft.AspNetCore.OData" Version="8.2.0" />
+  </ItemGroup>
+</Project>