C#: Exclude Semmle.* dlls when using the executing runtime. #15993

michaelnebel · 2024-03-20T14:19:28Z

In this PR we

Introduce a new container class AssemblyPath for paths to assemblies (or folders containing assemblies) and make it possible to add a filter function for relevant filenames on the path.
Introduce logic that exclude the Semmle.* dlls from being included in the dependencies.
Add an integration tests that uses the execution runtime.

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

tamasvajk

I've added some minor comments.

Additionally, we could consider extending the logic in AssemblyPath, which currently covers two cases:

file path
all files from a directory (recursively)

A third case would be

all files from a directory (only top level)

We're already using this latter case in AddFrameworkDlls.

tamasvajk · 2024-04-09T08:09:09Z

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

+            }
+            else
+            {
+                logger.LogInfo("AssemblyCache: Path not found: " + p);


Copy-paste issue: AssemblyCache vs AssemblyPath

tamasvajk · 2024-04-09T08:14:20Z

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

@@ -71,7 +71,7 @@ public DependencyManager(string srcDir, ILogger logger)
            this.generatedSources = new();
            var allProjects = allNonBinaryFiles.SelectFileNamesByExtension(".csproj").ToList();
            var allSolutions = allNonBinaryFiles.SelectFileNamesByExtension(".sln").ToList();
-            var dllPaths = allFiles.SelectFileNamesByExtension(".dll").ToHashSet();
+            var dllPaths = allFiles.SelectFileNamesByExtension(".dll").Select<string, AssemblyPath>(x => x).ToHashSet();


Just an opinion: I find .Select<string, AssemblyPath>(x => x) difficult to read, I think .Select(x => new AssemblyPath(x)) is easier to understand.

tamasvajk · 2024-04-09T08:19:25Z

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

-                {
-                    logger.LogInfo("AssemblyCache: Path not found: " + path);
-                }
+                path.Process(dllsToIndex, logger);


This Process call could be changed to simply return a list of DLL paths, and then the return value could be used: dllsToIndex.AddRange(path.GetFilePaths(logger)). This way the AssemblyPath is more self contained as we're not pushing "state" into it.

tamasvajk · 2024-04-09T08:26:18Z

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

+    /// Used to represent a path to an assembly or a directory containing assemblies
+    /// and a selector function to determine which files to include, when indexing the assemblies.
+    /// </summary>
+    internal sealed class AssemblyPath(string p, Func<string, bool> includeFileName)


Maybe AssemblyLookupLocation is a somewhat better name for this, as it's not necessarily an assembly path that we're storing here.

michaelnebel · 2024-04-09T11:41:38Z

I will extend the scope a bit more on this PR.

…stead of strings.

…framework.

…on and log if no dlls are found in a directory.

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

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

michaelnebel · 2024-04-10T11:28:32Z

@tamasvajk : I Discovered a difference in the file enumeration (which can be contributed to your tireless effort of making proper integration test coverage - otherwise it would definitely have been missed): It turns out that when using enumeration options to enumerate files with the EnumerateFiles, then hidden files and folders are excluded by default (contrary to EnumerateFiles(pattern, SearchOption.AllDirectories)). This means that folders like .NETFramework are excluded as everything starting with . is perceived as hidden. This has been fixed at least for the dll indexing. This means that the dependency manager in general might also exclude hidden files and folders. Should we consider to change that?

michaelnebel · 2024-04-10T11:32:38Z

With this change we might be able to collapse dllLocations and frameworkLocations but it will require some work as the asset detection needs to properly identify framework dependencies.

tamasvajk · 2024-04-10T11:58:58Z

This means that the dependency manager in general might also exclude hidden files and folders. Should we consider to change that?

Hmm, interesting finding. For the time being, I wouldn't change this. My fear is that it might have significant impact on the file operations that we do. For example, reading all the files in the .git folder might be slow.

tamasvajk

Looks good. I've added some minor comments.

tamasvajk · 2024-04-10T12:06:04Z

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

+    /// Used to represent a path to an assembly or a directory containing assemblies
+    /// and a selector function to determine which files to include, when indexing the assemblies.
+    /// </summary>
+    internal sealed class AssemblyLookupLocation(string p, Func<string, bool> includeFileName, bool indexSubdirectories = true)


Nit: Maybe use path instead of p.

tamasvajk · 2024-04-10T12:09:19Z

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

+        /// and adds them to the a list of assembly names to index.
+        /// Indexing is performed at a later stage. This only collects the names.
+        /// </summary>
+        /// <param name="dir">The directory to index.</param>


The parameters have diverged from the comment.

tamasvajk · 2024-04-10T12:24:14Z

csharp/ql/integration-tests/posix-only/standalone_dependencies_executing_runtime/global.json

@@ -0,0 +1,5 @@
+{


This file is probably not needed for this test.

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

+            catch (Exception e)
+            {
+                logger.LogError($"AssemblyLookupLocation: Error while searching for DLLs in '{path}': {e.Message}");
+            }


tamasvajk

LGTM

github-actions bot added the C# label Mar 20, 2024

michaelnebel force-pushed the csharp/assemblycachefiltering branch from 8fa54a0 to 8fb924f Compare April 5, 2024 08:01

github-advanced-security bot found potential problems Apr 5, 2024

View reviewed changes

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

michaelnebel force-pushed the csharp/assemblycachefiltering branch 5 times, most recently from afdb6eb to 5a70f70 Compare April 8, 2024 14:18

michaelnebel added the no-change-note-required This PR does not need a change note label Apr 8, 2024

michaelnebel marked this pull request as ready for review April 9, 2024 07:40

michaelnebel requested a review from a team as a code owner April 9, 2024 07:40

tamasvajk reviewed Apr 9, 2024

View reviewed changes

michaelnebel added 9 commits April 10, 2024 12:35

C#: Add desktop runtime dependencies integration test.

bee54e4

C#: Introduce AssemblyPath and re-factor AssemblyCache to use this in…

6299d9c

…stead of strings.

C#: Move the AssemblyPath class to its own file.

646b272

C#: Don't include Semmle.* dlls, if the executing runtime is used as …

d04bf6b

…framework.

C#: Update expected test output.

2bea927

C#: Code quality improvements.

9eb1383

C#: Make the assembly lookup case insensitive on the dll file extensi…

99f0ed2

…on and log if no dlls are found in a directory.

C#: Also use AssemblyLookupLocation for framework dlls.

3b42dc2

C#: Some renaming.

ff498f6

michaelnebel force-pushed the csharp/assemblycachefiltering branch from 5a70f70 to ff498f6 Compare April 10, 2024 11:21

michaelnebel commented Apr 10, 2024

View reviewed changes

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

github-advanced-security bot found potential problems Apr 10, 2024

View reviewed changes

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

michaelnebel changed the title ~~C#: Introduce AssemblyPath and re-factor AssemblyCache to use this in…~~ C#: Exclude Semmle.* dlls when using the executing runtime. Apr 10, 2024

michaelnebel requested a review from tamasvajk April 10, 2024 11:46

tamasvajk reviewed Apr 10, 2024

View reviewed changes

C#: Address review comments.

ef68e33

github-advanced-security bot found potential problems Apr 10, 2024

View reviewed changes

tamasvajk approved these changes Apr 10, 2024

View reviewed changes

michaelnebel merged commit b79d738 into github:main Apr 10, 2024
16 checks passed

michaelnebel deleted the csharp/assemblycachefiltering branch April 10, 2024 14:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

C#: Exclude Semmle.* dlls when using the executing runtime. #15993

C#: Exclude Semmle.* dlls when using the executing runtime. #15993

michaelnebel commented Mar 20, 2024 •

edited

Loading

tamasvajk left a comment

tamasvajk Apr 9, 2024

tamasvajk Apr 9, 2024

tamasvajk Apr 9, 2024

tamasvajk Apr 9, 2024

michaelnebel commented Apr 9, 2024

michaelnebel commented Apr 10, 2024 •

edited

Loading

michaelnebel commented Apr 10, 2024

tamasvajk commented Apr 10, 2024

tamasvajk left a comment

tamasvajk Apr 10, 2024

tamasvajk Apr 10, 2024

tamasvajk Apr 10, 2024

tamasvajk left a comment

C#: Exclude Semmle.* dlls when using the executing runtime. #15993

C#: Exclude Semmle.* dlls when using the executing runtime. #15993

Conversation

michaelnebel commented Mar 20, 2024 • edited Loading

tamasvajk left a comment

Choose a reason for hiding this comment

tamasvajk Apr 9, 2024

Choose a reason for hiding this comment

tamasvajk Apr 9, 2024

Choose a reason for hiding this comment

tamasvajk Apr 9, 2024

Choose a reason for hiding this comment

tamasvajk Apr 9, 2024

Choose a reason for hiding this comment

michaelnebel commented Apr 9, 2024

michaelnebel commented Apr 10, 2024 • edited Loading

michaelnebel commented Apr 10, 2024

tamasvajk commented Apr 10, 2024

tamasvajk left a comment

Choose a reason for hiding this comment

tamasvajk Apr 10, 2024

Choose a reason for hiding this comment

tamasvajk Apr 10, 2024

Choose a reason for hiding this comment

tamasvajk Apr 10, 2024

Choose a reason for hiding this comment

tamasvajk left a comment

Choose a reason for hiding this comment

michaelnebel commented Mar 20, 2024 •

edited

Loading

michaelnebel commented Apr 10, 2024 •

edited

Loading