C#: Choose between .NET framework or core DLLs in standalone #14368

tamasvajk · 2023-10-04T08:45:29Z

This PR

fixes the lookup path of the Mono lib folder,
adds logic to choose between .NET framework/core DLLs ,
adds integration tests for standalone extraction showing the extracted references.

The changes help with the extraction of projects that target the full framework. For example:

Results for rapPayne/WebGoat.Net
 Query CallTargets.ql:
-  Result count - both/traced/standalone: 894/459/36
-  Result count in the matching 87 files - both/traced/standalone: 894/459/36
-   Matching 64.36%
+  Result count - both/traced/standalone: 1353/0/0
+  Result count in the matching 87 files - both/traced/standalone: 1353/0/0
+   Matching 100.0%

Results for jerryhoff/WebGoat.NET
 Query CallTargets.ql:
-  Result count - both/traced/standalone: 989/737/1685
-  Result count in the matching 156 files - both/traced/standalone: 989/737/25
-   Matching 56.48%
+  Result count - both/traced/standalone: 1654/72/1887
+  Result count in the matching 156 files - both/traced/standalone: 1654/72/0
+   Matching 95.83%

tamasvajk · 2023-10-04T13:11:43Z

DCA results don't look great, I checked a couple of the projects, and compiler errors were reported because of conflicting framework DLLs. The conflict was between nuget restored reference assemblies and installed DLLs. So I'll handle this todo: https://github.com/github/codeql/pull/14368/files#diff-0179d8d74e2e78735e1d6cde08b1cff43d2a132f2c53a86c277cfdf3bbd95017R102-R104

csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/DependencyManager.cs

tamasvajk · 2023-10-05T12:46:47Z

csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/DependencyManager.cs

+                existsNetCoreRefNugetPackage = IsNugetPackageAvailable("microsoft.netcore.app.ref");
+                existsNetFrameworkRefNugetPackage = IsNugetPackageAvailable("microsoft.netframework.referenceassemblies");
+
+                if (existsNetCoreRefNugetPackage || existsNetFrameworkRefNugetPackage)


The DCA changes show that this logic is not working great. If dotnet core 6 is installed, and there are net6.0 and net5.0 targets, then the reference assemblies of net5.0 are downloaded and preferred over the installed dotnet 6 DLLs. And this causes an issue if there are other references which are compiled against dotnet 6.0.

We could probably force the download of the ref assemblies for all target platforms, and we should choose the latest one from those.

csharp/ql/integration-tests/all-platforms/standalone_dependencies/Assemblies.expected

csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/DotNet.cs

tamasvajk · 2023-10-10T07:57:55Z

csharp/ql/integration-tests/all-platforms/standalone_dependencies/test.py

+import os
+from create_database_utils import *
+
+os.environ['PROCESSOR_ARCHITECTURE'] = 'x64'


This should not be merged. I added a fix for this in the internal repo.

tamasvajk · 2023-10-11T11:13:31Z

csharp/ql/integration-tests/all-platforms/standalone_dependencies_net48/DB-CHECK.expected

@@ -0,0 +1,5 @@
+[VALUE_NOT_IN_TYPE] predicate attributes(@attribute id, int kind, @type_or_ref type_id, @attributable target): Value 25 of field target is not in type @attributable. The value is however in the following types: @void_type. Appears in tuple (949897,0,960,25)


This is being fixed in #14436

csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/DependencyManager.cs

+            catch (Exception exc)
+            {
+                progressMonitor.LogInfo("Couldn't delete package directory: " + exc.Message);
+            }


csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/DependencyManager.cs

+                catch (Exception exc)
+                {
+                    progressMonitor.LogInfo("Couldn't delete temporary working directory: " + exc.Message);
+                }


tamasvajk · 2023-10-12T06:56:07Z

csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/DependencyManager.cs

+            }
+            catch (Exception exc)
+            {
+                progressMonitor.LogInfo("Couldn't delete package directory: " + exc.Message);


This logs Couldn't delete package directory: The process cannot access the file 'System.EnterpriseServices.Wrapper.dll' because it is being used by another process. on Windows. The DLL can't be loaded as an assembly, and it looks like System.Reflection.PortableExecutable.PEReader doesn't release it.

The DLL is coming from the microsoft.netframework.referenceassemblies.net48.1.0.3 nuget package.

tamasvajk · 2023-10-12T07:00:18Z

csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/DotNet.cs

+                    Directory.CreateDirectory(path);
+                }
+
+                args += $" /p:TargetFrameworkRootPath=\"{path}\" /p:NetCoreTargetingPackRoot=\"{path}\"";


This feels like an ugly hack, but I couldn't find any other msbuild parameters that would do the same.

The goal is to depend on the nuget reference assemblies instead of the locally installed ones (for both dotnet core and the full framework)

Looks fine to me :-D

tamasvajk · 2023-10-12T07:02:33Z

csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/Runtime.cs

+                var monoPath = FileUtils.FindProgramOnPath(Win32.IsWindows() ? "mono.exe" : "mono");
+                var monoDirs = monoPath is not null
+                    ? new[] { Path.GetFullPath(Path.Combine(monoPath, "..", "lib", "mono")), monoPath }
+                    : new[] { "/usr/lib/mono", "/usr/local/mono", "/usr/local/bin/mono", @"C:\Program Files\Mono\lib\mono" };


Added a couple more mono installation directories.

tamasvajk · 2023-10-12T07:04:52Z

csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/Runtime.cs

                if (Directory.Exists(@"C:\Windows\Microsoft.NET\Framework64"))
                {
                    return Directory.EnumerateDirectories(@"C:\Windows\Microsoft.NET\Framework64", "v*")
                        .OrderByDescending(Path.GetFileName);
                }

+                var monoPath = FileUtils.FindProgramOnPath(Win32.IsWindows() ? "mono.exe" : "mono");
+                var monoDirs = monoPath is not null
+                    ? new[] { Path.GetFullPath(Path.Combine(monoPath, "..", "lib", "mono")), monoPath }


The paths didn't match:

❯ where mono /Library/Frameworks/Mono.framework/Versions/Current/Commands/mono

vs

/Library/Frameworks/Mono.framework/Versions/Current/lib/mono

tamasvajk · 2023-10-12T07:05:57Z

...mmle.Extraction.CSharp.DependencyFetching/Semmle.Extraction.CSharp.DependencyFetching.csproj

@@ -8,6 +8,7 @@
    <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
    <RuntimeIdentifiers>win-x64;linux-x64;osx-x64</RuntimeIdentifiers>
    <Nullable>enable</Nullable>
+    <NoWarn>$(NoWarn);CA1822</NoWarn>


Not reporting warnings on members that could be static, such as Runtime.ExecutingRuntime.

Why not make it static instead of disabling the warning?

This is just stylistic. This way AspNetCoreRuntime, ExecutingRuntime, DesktopRuntime, and NetCoreRuntime can be called the same way.

tamasvajk · 2023-10-12T07:07:27Z

csharp/ql/integration-tests/windows-only/standalone_dependencies/Assemblies.expected

+| /microsoft.netcore.app.ref/7.0.2/ref/net7.0/System.Xml.XmlSerializer.dll |
+| /microsoft.netcore.app.ref/7.0.2/ref/net7.0/mscorlib.dll |
+| /microsoft.netcore.app.ref/7.0.2/ref/net7.0/netstandard.dll |
+| /microsoft.windowsdesktop.app.ref/7.0.2/analyzers/dotnet/System.Windows.Forms.Analyzers.dll |


The same test is added for unix-only too. The dependencies differ because of the platform. On Windows microsoft.windowsdesktop.app.ref is also fetched.

This reverts commit 2e8a91e.

csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/FileContent.cs

csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/Runtime.cs

csharp/extractor/Semmle.Extraction.Tests/FileContent.cs

michaelnebel · 2023-10-13T11:50:47Z

csharp/ql/integration-tests/all-platforms/standalone_dependencies_net48/Assemblies.ql

+  not a.getCompilation().getOutputAssembly() = a and
+  exists(string s | s = a.getFile().getAbsolutePath() |
+    result =
+      s.substring(s.indexOf("GitHub/packages/") + "GitHub/packages/".length() + 16, s.length())


Why is 16 also added explicitly (looks like this is also the length of "GitHub/packages/"?

We're using a 16-character-long hash as a subfolder name.

https://github.com/github/codeql/blob/main/csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/DependencyManager.cs#L283-L284

csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/DependencyManager.cs

michaelnebel · 2023-10-13T12:38:52Z

csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/DependencyManager.cs

+                    }
+                    var subFolderIndex = secondDirectorySeparatorCharIndex + 1;
+                    var isInAnalyzersFolder = lowerFilename.IndexOf("analyzers", subFolderIndex) == subFolderIndex;
+                    if (isInAnalyzersFolder)


Why is this needed?

Nuget packages can ship analyzers, code fixes, and source generators in the analyzers folder. These DLLs need to be passed in the analyzers csc argument and not in references. The base classes used in these DLLs are not part of the dotnet runtime (but the dotnet SDK), so if these are passed as references, they will produce compile errors, so we remove them from the reference list.

michaelnebel

Excellent work 👍

github-actions bot added the C# label Oct 4, 2023

github-advanced-security bot found potential problems Oct 5, 2023

View reviewed changes

csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/DependencyManager.cs Fixed Show fixed Hide fixed

tamasvajk commented Oct 5, 2023

View reviewed changes

tamasvajk force-pushed the standalone/use-legacy-framework-dlls branch from dbaa0b8 to 52ccc2a Compare October 6, 2023 08:23

tamasvajk commented Oct 6, 2023

View reviewed changes

csharp/ql/integration-tests/all-platforms/standalone_dependencies/Assemblies.expected Outdated Show resolved Hide resolved

tamasvajk force-pushed the standalone/use-legacy-framework-dlls branch 3 times, most recently from 83e5057 to 3f9b989 Compare October 6, 2023 21:56

github-advanced-security bot found potential problems Oct 9, 2023

View reviewed changes

csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/DotNet.cs Fixed Show fixed Hide fixed

tamasvajk force-pushed the standalone/use-legacy-framework-dlls branch 4 times, most recently from fdc8db5 to c2099fd Compare October 9, 2023 10:37

tamasvajk commented Oct 10, 2023

View reviewed changes

This was referenced Oct 10, 2023

C#: Make conflicting assembly selection deterministic in standalone #14428

Merged

C#: Remove keyset from metadata_handle relation #14429

Merged

tamasvajk force-pushed the standalone/use-legacy-framework-dlls branch 2 times, most recently from 8c600ac to 0061dfe Compare October 11, 2023 11:12

tamasvajk commented Oct 11, 2023

View reviewed changes

tamasvajk force-pushed the standalone/use-legacy-framework-dlls branch from 59faeb3 to 7e30485 Compare October 11, 2023 12:49

tamasvajk added 8 commits October 11, 2023 16:25

C#: Choose between .NET framework or core DLLs in standalone

4887c69

C#: Consider possibly restored reference assemblies

4e2c6ff

Add integration test listing standalone extracted assemblies

b8effa3

Add integration test targetting .NET 4.8

12fdb34

Force the nuget download of dotnet framework reference assemblies

93380f8

Exclude nuget packages with prefix runtime. from the references

e1b283c

Separate standalone dependency integration tests by platform

e718796

Adjust expected test result in windows standalone test

ada5dcc

tamasvajk force-pushed the standalone/use-legacy-framework-dlls branch from 39351be to ada5dcc Compare October 11, 2023 14:25

tamasvajk added 2 commits October 11, 2023 16:40

C#: Fix stub generator to use framework references from nuget packages

4f31b5a

Do not throw exception when working directory cleanup fails

da09655

github-advanced-security bot found potential problems Oct 11, 2023

View reviewed changes

tamasvajk added 3 commits October 11, 2023 22:28

Lock SDK version in standalone test

e82076d

Adjust stub generator test to fix the nuget package version

534ea3e

Add source generators to framework stubs

2e8a91e

tamasvajk commented Oct 12, 2023

View reviewed changes

tamasvajk added 3 commits October 12, 2023 09:35

Revert "Add source generators to framework stubs"

8a0dc31

This reverts commit 2e8a91e.

Filter analyzer assemblies from nuget packages

09c1c71

Be more explicit about expected assembly loading exceptions

3b4ea27

tamasvajk force-pushed the standalone/use-legacy-framework-dlls branch from e6c0a89 to 3b4ea27 Compare October 12, 2023 09:10

tamasvajk marked this pull request as ready for review October 12, 2023 09:53

tamasvajk requested review from a team as code owners October 12, 2023 09:53

tamasvajk removed the request for review from a team October 12, 2023 10:55