From 83ba4fe729e39d0e7ceefc59f59fc017130d690b Mon Sep 17 00:00:00 2001 From: Ladi Prosek Date: Fri, 21 Aug 2020 17:33:37 +0200 Subject: [PATCH 1/5] Don't expand full drive globs with false condition --- .../Definition/ProjectItem_Tests.cs | 40 +++++++++++++ src/Build/Evaluation/ItemSpec.cs | 2 + .../LazyItemEvaluator.IncludeOperation.cs | 57 ++++++++++--------- src/Shared/UnitTests/ObjectModelHelpers.cs | 11 ++-- 4 files changed, 79 insertions(+), 31 deletions(-) diff --git a/src/Build.OM.UnitTests/Definition/ProjectItem_Tests.cs b/src/Build.OM.UnitTests/Definition/ProjectItem_Tests.cs index 3172b22b806..9135affdb29 100644 --- a/src/Build.OM.UnitTests/Definition/ProjectItem_Tests.cs +++ b/src/Build.OM.UnitTests/Definition/ProjectItem_Tests.cs @@ -406,6 +406,46 @@ public void RecursiveDirWithSemicolonSeparatedInclude() } } + [Theory] + [InlineData(@"")] + [InlineData(@"")] + public void FullFileSystemScanGlobWithFalseCondition(string itemDefinition) + { + IList items = ObjectModelHelpers.GetItemsFromFragment(itemDefinition, allItems: false, ignoreCondition: true); + items.ShouldBeEmpty(); + } + + [Theory] + [InlineData(@"")] + [InlineData(@"")] + public void PartialFileSystemScanGlobWithFalseCondition(string itemDefinition) + { + string directory = null; + string file = null; + + try + { + directory = Path.Combine(Path.GetTempPath(), "somedir"); + if (File.Exists(directory)) + { + File.Delete(directory); + } + + file = Path.Combine(directory, "a.cs"); + Directory.CreateDirectory(directory); + + File.WriteAllText(file, String.Empty); + + IList items = ObjectModelHelpers.GetItemsFromFragment(itemDefinition.Replace("somedir", directory), allItems: false, ignoreCondition: true); + items.ShouldNotBeEmpty(); + } + finally + { + File.Delete(file); + FileUtilities.DeleteWithoutTrailingBackslash(directory); + } + } + /// /// Basic exclude case /// diff --git a/src/Build/Evaluation/ItemSpec.cs b/src/Build/Evaluation/ItemSpec.cs index 165407a7f02..c7c15928b23 100644 --- a/src/Build/Evaluation/ItemSpec.cs +++ b/src/Build/Evaluation/ItemSpec.cs @@ -498,5 +498,7 @@ public GlobFragment(string textFragment, string projectDirectory) : base(textFragment, projectDirectory) { } + + public bool IsFullFileSystemScan => (TextFragment.StartsWith(@"\**\") || TextFragment.StartsWith(@"/**/")); } } diff --git a/src/Build/Evaluation/LazyItemEvaluator.IncludeOperation.cs b/src/Build/Evaluation/LazyItemEvaluator.IncludeOperation.cs index 528b2d85cbd..b754e82e5cb 100644 --- a/src/Build/Evaluation/LazyItemEvaluator.IncludeOperation.cs +++ b/src/Build/Evaluation/LazyItemEvaluator.IncludeOperation.cs @@ -89,34 +89,37 @@ protected override ImmutableList SelectItems(ImmutableList.Builder } else if (fragment is GlobFragment globFragment) { - string glob = globFragment.TextFragment; - - if (excludePatternsForGlobs == null) - { - excludePatternsForGlobs = BuildExcludePatternsForGlobs(globsToIgnore, excludePatterns); - } - - string[] includeSplitFilesEscaped; - if (MSBuildEventSource.Log.IsEnabled()) - { - MSBuildEventSource.Log.ExpandGlobStart(_rootDirectory, glob, string.Join(", ", excludePatternsForGlobs)); - } - using (_lazyEvaluator._evaluationProfiler.TrackGlob(_rootDirectory, glob, excludePatternsForGlobs)) - { - includeSplitFilesEscaped = EngineFileUtilities.GetFileListEscaped( - _rootDirectory, - glob, - excludePatternsForGlobs - ); - } - if (MSBuildEventSource.Log.IsEnabled()) - { - MSBuildEventSource.Log.ExpandGlobStop(_rootDirectory, glob, string.Join(", ", excludePatternsForGlobs)); - } - - foreach (string includeSplitFileEscaped in includeSplitFilesEscaped) + if (_conditionResult || !globFragment.IsFullFileSystemScan) { - itemsToAdd.Add(_itemFactory.CreateItem(includeSplitFileEscaped, glob, _itemElement.ContainingProject.FullPath)); + string glob = globFragment.TextFragment; + + if (excludePatternsForGlobs == null) + { + excludePatternsForGlobs = BuildExcludePatternsForGlobs(globsToIgnore, excludePatterns); + } + + string[] includeSplitFilesEscaped; + if (MSBuildEventSource.Log.IsEnabled()) + { + MSBuildEventSource.Log.ExpandGlobStart(_rootDirectory, glob, string.Join(", ", excludePatternsForGlobs)); + } + using (_lazyEvaluator._evaluationProfiler.TrackGlob(_rootDirectory, glob, excludePatternsForGlobs)) + { + includeSplitFilesEscaped = EngineFileUtilities.GetFileListEscaped( + _rootDirectory, + glob, + excludePatternsForGlobs + ); + } + if (MSBuildEventSource.Log.IsEnabled()) + { + MSBuildEventSource.Log.ExpandGlobStop(_rootDirectory, glob, string.Join(", ", excludePatternsForGlobs)); + } + + foreach (string includeSplitFileEscaped in includeSplitFilesEscaped) + { + itemsToAdd.Add(_itemFactory.CreateItem(includeSplitFileEscaped, glob, _itemElement.ContainingProject.FullPath)); + } } } else diff --git a/src/Shared/UnitTests/ObjectModelHelpers.cs b/src/Shared/UnitTests/ObjectModelHelpers.cs index 7cc65ebdf2f..61f02fec550 100644 --- a/src/Shared/UnitTests/ObjectModelHelpers.cs +++ b/src/Shared/UnitTests/ObjectModelHelpers.cs @@ -1042,11 +1042,11 @@ internal static string[] GetTempFiles(int number, DateTime lastWriteTime) /// /// Get items of item type "i" with using the item xml fragment passed in /// - internal static IList GetItemsFromFragment(string fragment, bool allItems = false) + internal static IList GetItemsFromFragment(string fragment, bool allItems = false, bool ignoreCondition = false) { string content = FormatProjectContentsWithItemGroupFragment(fragment); - IList items = GetItems(content, allItems); + IList items = GetItems(content, allItems, ignoreCondition); return items; } @@ -1058,11 +1058,14 @@ internal static string GetConcatenatedItemsOfType(this Project project, string i /// /// Get the items of type "i" in the project provided /// - internal static IList GetItems(string content, bool allItems = false) + internal static IList GetItems(string content, bool allItems = false, bool ignoreCondition = false) { var projectXml = ProjectRootElement.Create(XmlReader.Create(new StringReader(CleanupFileContents(content)))); Project project = new Project(projectXml); - IList item = Helpers.MakeList(allItems ? project.Items : project.GetItems("i")); + IList item = Helpers.MakeList( + ignoreCondition ? + (allItems ? project.ItemsIgnoringCondition : project.GetItemsIgnoringCondition("i")) : + (allItems ? project.Items : project.GetItems("i"))); return item; } From af3d70cc4c6545e2569d22da5eadd89d85a6033c Mon Sep 17 00:00:00 2001 From: Ladi Prosek Date: Sun, 23 Aug 2020 21:13:46 +0200 Subject: [PATCH 2/5] PR feedback: Use TestEnvironment --- .../Definition/ProjectItem_Tests.cs | 24 ++++--------------- 1 file changed, 5 insertions(+), 19 deletions(-) diff --git a/src/Build.OM.UnitTests/Definition/ProjectItem_Tests.cs b/src/Build.OM.UnitTests/Definition/ProjectItem_Tests.cs index 9135affdb29..60f33954bfc 100644 --- a/src/Build.OM.UnitTests/Definition/ProjectItem_Tests.cs +++ b/src/Build.OM.UnitTests/Definition/ProjectItem_Tests.cs @@ -420,30 +420,16 @@ public void FullFileSystemScanGlobWithFalseCondition(string itemDefinition) [InlineData(@"")] public void PartialFileSystemScanGlobWithFalseCondition(string itemDefinition) { - string directory = null; - string file = null; - - try + using (TestEnvironment env = TestEnvironment.Create()) { - directory = Path.Combine(Path.GetTempPath(), "somedir"); - if (File.Exists(directory)) - { - File.Delete(directory); - } + TransientTestFolder directory = env.CreateFolder(createFolder: true); + TransientTestFile file = env.CreateFile(directory, "a.cs"); - file = Path.Combine(directory, "a.cs"); - Directory.CreateDirectory(directory); + File.WriteAllText(file.Path, String.Empty); - File.WriteAllText(file, String.Empty); - - IList items = ObjectModelHelpers.GetItemsFromFragment(itemDefinition.Replace("somedir", directory), allItems: false, ignoreCondition: true); + IList items = ObjectModelHelpers.GetItemsFromFragment(itemDefinition.Replace("somedir", directory.Path), allItems: false, ignoreCondition: true); items.ShouldNotBeEmpty(); } - finally - { - File.Delete(file); - FileUtilities.DeleteWithoutTrailingBackslash(directory); - } } /// From 45af1f0774ea85c7eaaacf524a992d32fec30b23 Mon Sep 17 00:00:00 2001 From: Ladi Prosek Date: Sun, 23 Aug 2020 21:35:53 +0200 Subject: [PATCH 3/5] Add an escape hatch --- src/Build/Evaluation/LazyItemEvaluator.IncludeOperation.cs | 6 +++++- src/Shared/Traits.cs | 5 +++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/Build/Evaluation/LazyItemEvaluator.IncludeOperation.cs b/src/Build/Evaluation/LazyItemEvaluator.IncludeOperation.cs index b754e82e5cb..a5482b221d9 100644 --- a/src/Build/Evaluation/LazyItemEvaluator.IncludeOperation.cs +++ b/src/Build/Evaluation/LazyItemEvaluator.IncludeOperation.cs @@ -5,6 +5,7 @@ using Microsoft.Build.Eventing; using Microsoft.Build.Internal; using Microsoft.Build.Shared; +using Microsoft.Build.Utilities; using System; using System.Collections.Generic; using System.Collections.Immutable; @@ -89,7 +90,10 @@ protected override ImmutableList SelectItems(ImmutableList.Builder } else if (fragment is GlobFragment globFragment) { - if (_conditionResult || !globFragment.IsFullFileSystemScan) + // If this item is behind a false condition and represents a full drive/filesystem scan, expanding it is + // almost certainly undesired. It should be skipped to avoid evaluation taking an excessive amount of time. + bool skipGlob = !_conditionResult && globFragment.IsFullFileSystemScan && !Traits.Instance.EscapeHatches.AlwaysEvaluateDangerousGlobs; + if (!skipGlob) { string glob = globFragment.TextFragment; diff --git a/src/Shared/Traits.cs b/src/Shared/Traits.cs index 652ad518e4c..0d7768a5ece 100644 --- a/src/Shared/Traits.cs +++ b/src/Shared/Traits.cs @@ -141,6 +141,11 @@ internal class EscapeHatches /// public readonly bool DoNotTruncateConditions = Environment.GetEnvironmentVariable("MSBuildDoNotTruncateConditions") == "1"; + /// + /// Disables skipping full drive/filesystem globs that are behind a false condition. + /// + public readonly bool AlwaysEvaluateDangerousGlobs = Environment.GetEnvironmentVariable("MSBuildAlwaysEvaluateDangerousGlobs") == "1"; + /// /// Emit events for project imports. /// From 585793ef97ecb9109581d94959e70141d9383aee Mon Sep 17 00:00:00 2001 From: Ladi Prosek Date: Mon, 24 Aug 2020 08:23:44 +0200 Subject: [PATCH 4/5] Update src/Build.OM.UnitTests/Definition/ProjectItem_Tests.cs Co-authored-by: Forgind --- src/Build.OM.UnitTests/Definition/ProjectItem_Tests.cs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/Build.OM.UnitTests/Definition/ProjectItem_Tests.cs b/src/Build.OM.UnitTests/Definition/ProjectItem_Tests.cs index 60f33954bfc..e6f3b806634 100644 --- a/src/Build.OM.UnitTests/Definition/ProjectItem_Tests.cs +++ b/src/Build.OM.UnitTests/Definition/ProjectItem_Tests.cs @@ -423,9 +423,7 @@ public void PartialFileSystemScanGlobWithFalseCondition(string itemDefinition) using (TestEnvironment env = TestEnvironment.Create()) { TransientTestFolder directory = env.CreateFolder(createFolder: true); - TransientTestFile file = env.CreateFile(directory, "a.cs"); - - File.WriteAllText(file.Path, String.Empty); + TransientTestFile file = env.CreateFile(directory, "a.cs", String.Empty); IList items = ObjectModelHelpers.GetItemsFromFragment(itemDefinition.Replace("somedir", directory.Path), allItems: false, ignoreCondition: true); items.ShouldNotBeEmpty(); From 3b76fd8a359bcbc4de112debc941ff7c48aa94b5 Mon Sep 17 00:00:00 2001 From: Ladi Prosek Date: Mon, 24 Aug 2020 22:05:19 +0200 Subject: [PATCH 5/5] PR feedback: Make full FS scan detection fully slash-agnostic --- src/Build.OM.UnitTests/Definition/ProjectItem_Tests.cs | 4 ++++ src/Build/Evaluation/ItemSpec.cs | 9 ++++++++- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/src/Build.OM.UnitTests/Definition/ProjectItem_Tests.cs b/src/Build.OM.UnitTests/Definition/ProjectItem_Tests.cs index e6f3b806634..c305a27ed6c 100644 --- a/src/Build.OM.UnitTests/Definition/ProjectItem_Tests.cs +++ b/src/Build.OM.UnitTests/Definition/ProjectItem_Tests.cs @@ -409,6 +409,8 @@ public void RecursiveDirWithSemicolonSeparatedInclude() [Theory] [InlineData(@"")] [InlineData(@"")] + [InlineData(@"")] + [InlineData(@"")] public void FullFileSystemScanGlobWithFalseCondition(string itemDefinition) { IList items = ObjectModelHelpers.GetItemsFromFragment(itemDefinition, allItems: false, ignoreCondition: true); @@ -418,6 +420,8 @@ public void FullFileSystemScanGlobWithFalseCondition(string itemDefinition) [Theory] [InlineData(@"")] [InlineData(@"")] + [InlineData(@"")] + [InlineData(@"")] public void PartialFileSystemScanGlobWithFalseCondition(string itemDefinition) { using (TestEnvironment env = TestEnvironment.Create()) diff --git a/src/Build/Evaluation/ItemSpec.cs b/src/Build/Evaluation/ItemSpec.cs index c7c15928b23..5177e944471 100644 --- a/src/Build/Evaluation/ItemSpec.cs +++ b/src/Build/Evaluation/ItemSpec.cs @@ -499,6 +499,13 @@ public GlobFragment(string textFragment, string projectDirectory) { } - public bool IsFullFileSystemScan => (TextFragment.StartsWith(@"\**\") || TextFragment.StartsWith(@"/**/")); + /// + /// True if TextFragment starts with /**/ or a variation thereof with backslashes. + /// + public bool IsFullFileSystemScan => TextFragment.Length >= 4 + && FileUtilities.IsAnySlash(TextFragment[0]) + && TextFragment[1] == '*' + && TextFragment[2] == '*' + && FileUtilities.IsAnySlash(TextFragment[3]); } }