From 9c6912143ac569ba312e03cb4183c655125c25d8 Mon Sep 17 00:00:00 2001 From: Marc Brooks Date: Fri, 21 Mar 2025 18:37:39 -0500 Subject: [PATCH 1/4] Fix break in DirectoryServices SearchResultCollection Added enumeration handling when the internal ArrayList has already been loaded but an enumeration follows. This fixes the issue with 9.x Linq statements against the collection caused by the access of the .Count Fixes #113265 --- .../SearchResultCollection.cs | 60 ++++++++++++-- .../System.DirectoryServices.Tests.csproj | 4 +- .../DirectorySearcherTests.cs | 83 +++++++++++++++++++ 3 files changed, 137 insertions(+), 10 deletions(-) create mode 100644 src/libraries/System.DirectoryServices/tests/System/DirectoryServices/DirectorySearcherTests.cs diff --git a/src/libraries/System.DirectoryServices/src/System/DirectoryServices/SearchResultCollection.cs b/src/libraries/System.DirectoryServices/src/System/DirectoryServices/SearchResultCollection.cs index 32a0666bdb4e90..6369b190988025 100644 --- a/src/libraries/System.DirectoryServices/src/System/DirectoryServices/SearchResultCollection.cs +++ b/src/libraries/System.DirectoryServices/src/System/DirectoryServices/SearchResultCollection.cs @@ -47,7 +47,7 @@ private ArrayList InnerList { if (_innerList == null) { - _innerList = new ArrayList(); + var eagerList = new ArrayList(); var enumerator = new ResultsEnumerator( this, _rootEntry.GetUsername(), @@ -55,7 +55,9 @@ private ArrayList InnerList _rootEntry.AuthenticationType); while (enumerator.MoveNext()) - _innerList.Add(enumerator.Current); + eagerList.Add(enumerator.Current); + + _innerList = eagerList; } return _innerList; @@ -188,12 +190,10 @@ protected virtual void Dispose(bool disposing) public IEnumerator GetEnumerator() { - // Two ResultsEnumerators can't exist at the same time over the - // same object. Need to get a new handle, which means re-querying. - return new ResultsEnumerator(this, - _rootEntry.GetUsername(), - _rootEntry.GetPassword(), - _rootEntry.AuthenticationType); + if (_innerList != null) + return new AlreadyReadResultsEnumerator(_innerList); + else + return new ResultsEnumerator(this, _rootEntry.GetUsername(), _rootEntry.GetPassword(), _rootEntry.AuthenticationType); } public bool Contains(SearchResult result) => InnerList.Contains(result); @@ -214,6 +214,49 @@ void ICollection.CopyTo(Array array, int index) InnerList.CopyTo(array, index); } + /// + /// Supports a simple type-specific wrapper for the underlying cached list + /// + private sealed class AlreadyReadResultsEnumerator : IEnumerator + { + private readonly IEnumerator _innerEnumerator; + + internal AlreadyReadResultsEnumerator(ArrayList innerList) + { + _innerEnumerator = innerList.GetEnumerator(); + } + + /// + /// Gets the current element in the collection. + /// + public SearchResult Current + { + get + { + return (SearchResult)(_innerEnumerator.Current); + } + } + + /// + /// Advances the enumerator to the next element of the collection + /// and returns a Boolean value indicating whether a valid element is available. + /// + public bool MoveNext() + { + return _innerEnumerator.MoveNext(); + } + + /// + /// Resets the enumerator back to its initial position before the first element in the collection. + /// + public void Reset() + { + _innerEnumerator.Reset(); + } + + object IEnumerator.Current => Current; + } + /// /// Supports a simple ForEach-style iteration over a collection. /// @@ -310,6 +353,7 @@ public bool MoveNext() return false; _currentResult = null; + if (!_initialized) { int hr = _results.SearchObject.GetFirstRow(_results.Handle); diff --git a/src/libraries/System.DirectoryServices/tests/System.DirectoryServices.Tests.csproj b/src/libraries/System.DirectoryServices/tests/System.DirectoryServices.Tests.csproj index b553252d53f604..4dd407cbed0e47 100644 --- a/src/libraries/System.DirectoryServices/tests/System.DirectoryServices.Tests.csproj +++ b/src/libraries/System.DirectoryServices/tests/System.DirectoryServices.Tests.csproj @@ -9,6 +9,7 @@ + @@ -20,8 +21,7 @@ - + diff --git a/src/libraries/System.DirectoryServices/tests/System/DirectoryServices/DirectorySearcherTests.cs b/src/libraries/System.DirectoryServices/tests/System/DirectoryServices/DirectorySearcherTests.cs new file mode 100644 index 00000000000000..f78ab9b11344d5 --- /dev/null +++ b/src/libraries/System.DirectoryServices/tests/System/DirectoryServices/DirectorySearcherTests.cs @@ -0,0 +1,83 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Runtime.InteropServices; +using System.Collections.Generic; +using System.Collections; +using Xunit; +using Xunit.Sdk; +using System.Reflection; + +namespace System.DirectoryServices.Tests +{ + public partial class DirectorySearcherTests + { + internal static bool IsLdapConfigurationExist => LdapConfiguration.Configuration != null; + internal static bool IsActiveDirectoryServer => IsLdapConfigurationExist && LdapConfiguration.Configuration.IsActiveDirectoryServer; + + private const int ADS_SYSTEMFLAG_CR_NTDS_NC = 0x1; + private const int ADS_SYSTEMFLAG_CR_NTDS_DOMAIN = 0x2; + + [ConditionalFact(nameof(IsLdapConfigurationExist))] + public void DirectorySearch_IteratesCorrectly_SimpleEnumeration() + { + var e = GetDomains(); + Assert.NotNull(e); + + foreach (var result in e) + { + Assert.NotNull(result); + } + } + + [ConditionalFact(nameof(IsLdapConfigurationExist))] + public void DirectorySearch_IteratesCorrectly_AfterCount() + { + var e = GetDomains(); + Assert.NotNull(e); + Assert.NotEqual(0, e.Count); + + foreach (var result in e) + { + Assert.NotNull(result); + } + } + + + [ConditionalFact(nameof(IsLdapConfigurationExist))] + public void DirectorySearch_IteratesCorrectly_MixedCount() + { + var e = GetDomains(); + Assert.NotNull(e); + Assert.NotEqual(0, e.Count); + + foreach (var result in e) + { + Assert.NotEqual(0, e.Count); + Assert.NotNull(result); + } + } + + private static SearchResultCollection GetDomains() + { + using var entry = new DirectoryEntry("LDAP://rootDSE"); + var namingContext = entry.Properties["configurationNamingContext"][0]!.ToString(); + using var searchRoot = new DirectoryEntry($"LDAP://CN=Partitions,{namingContext}"); + using var ds = new DirectorySearcher(searchRoot) + { + PageSize = 1000, + CacheResults = false + }; + ds.SearchScope = SearchScope.OneLevel; + ds.PropertiesToLoad.Add("distinguishedName"); + ds.PropertiesToLoad.Add("nETBIOSName"); + ds.PropertiesToLoad.Add("nCName"); + ds.PropertiesToLoad.Add("dnsRoot"); + ds.PropertiesToLoad.Add("trustParent"); + ds.PropertiesToLoad.Add("objectSid"); + ds.Filter = string.Format("(&(objectCategory=crossRef)(systemFlags={0}))", ADS_SYSTEMFLAG_CR_NTDS_DOMAIN | ADS_SYSTEMFLAG_CR_NTDS_NC); + + return ds.FindAll(); + } + } +} From d05ec50d46a34df9878746bb8dde1d5e8253cf15 Mon Sep 17 00:00:00 2001 From: Marc Brooks Date: Mon, 24 Mar 2025 09:16:08 -0500 Subject: [PATCH 2/4] PR feedback Switched enumerator to struct Improved testing actual scenario of the bug. --- .../SearchResultCollection.cs | 2 +- .../DirectorySearcherTests.cs | 21 +++++++------------ 2 files changed, 8 insertions(+), 15 deletions(-) diff --git a/src/libraries/System.DirectoryServices/src/System/DirectoryServices/SearchResultCollection.cs b/src/libraries/System.DirectoryServices/src/System/DirectoryServices/SearchResultCollection.cs index 6369b190988025..9291d9c5901eb8 100644 --- a/src/libraries/System.DirectoryServices/src/System/DirectoryServices/SearchResultCollection.cs +++ b/src/libraries/System.DirectoryServices/src/System/DirectoryServices/SearchResultCollection.cs @@ -217,7 +217,7 @@ void ICollection.CopyTo(Array array, int index) /// /// Supports a simple type-specific wrapper for the underlying cached list /// - private sealed class AlreadyReadResultsEnumerator : IEnumerator + private struct AlreadyReadResultsEnumerator : IEnumerator { private readonly IEnumerator _innerEnumerator; diff --git a/src/libraries/System.DirectoryServices/tests/System/DirectoryServices/DirectorySearcherTests.cs b/src/libraries/System.DirectoryServices/tests/System/DirectoryServices/DirectorySearcherTests.cs index f78ab9b11344d5..e4d60586a7079d 100644 --- a/src/libraries/System.DirectoryServices/tests/System/DirectoryServices/DirectorySearcherTests.cs +++ b/src/libraries/System.DirectoryServices/tests/System/DirectoryServices/DirectorySearcherTests.cs @@ -21,18 +21,23 @@ public partial class DirectorySearcherTests [ConditionalFact(nameof(IsLdapConfigurationExist))] public void DirectorySearch_IteratesCorrectly_SimpleEnumeration() { + bool seen = false; var e = GetDomains(); Assert.NotNull(e); foreach (var result in e) { Assert.NotNull(result); + seen = true; } + + Assert.True(seen); } [ConditionalFact(nameof(IsLdapConfigurationExist))] public void DirectorySearch_IteratesCorrectly_AfterCount() { + bool seen = false; var e = GetDomains(); Assert.NotNull(e); Assert.NotEqual(0, e.Count); @@ -40,22 +45,10 @@ public void DirectorySearch_IteratesCorrectly_AfterCount() foreach (var result in e) { Assert.NotNull(result); + seen = true; } - } - - [ConditionalFact(nameof(IsLdapConfigurationExist))] - public void DirectorySearch_IteratesCorrectly_MixedCount() - { - var e = GetDomains(); - Assert.NotNull(e); - Assert.NotEqual(0, e.Count); - - foreach (var result in e) - { - Assert.NotEqual(0, e.Count); - Assert.NotNull(result); - } + Assert.True(seen); } private static SearchResultCollection GetDomains() From 9c1e43513fe21ffeddf0020d41165908ccf73051 Mon Sep 17 00:00:00 2001 From: Marc Brooks Date: Mon, 24 Mar 2025 11:55:05 -0500 Subject: [PATCH 3/4] PR feedback Switched the enumerator doc comments to XML docs. Resorted the test class in the csproj Strong type the SearchResultCollection in the test class. --- .../SearchResultCollection.cs | 28 ++++++++++--------- .../System.DirectoryServices.Tests.csproj | 2 +- .../DirectorySearcherTests.cs | 4 +-- 3 files changed, 18 insertions(+), 16 deletions(-) diff --git a/src/libraries/System.DirectoryServices/src/System/DirectoryServices/SearchResultCollection.cs b/src/libraries/System.DirectoryServices/src/System/DirectoryServices/SearchResultCollection.cs index 9291d9c5901eb8..98f1a577f5a2ad 100644 --- a/src/libraries/System.DirectoryServices/src/System/DirectoryServices/SearchResultCollection.cs +++ b/src/libraries/System.DirectoryServices/src/System/DirectoryServices/SearchResultCollection.cs @@ -214,21 +214,21 @@ void ICollection.CopyTo(Array array, int index) InnerList.CopyTo(array, index); } - /// - /// Supports a simple type-specific wrapper for the underlying cached list - /// + ///

Provides an enumerator implementation for the array of .

private struct AlreadyReadResultsEnumerator : IEnumerator { private readonly IEnumerator _innerEnumerator; + ///

Initialize the enumerator.

+ /// The ArrayList to enumerate. internal AlreadyReadResultsEnumerator(ArrayList innerList) { _innerEnumerator = innerList.GetEnumerator(); } - /// - /// Gets the current element in the collection. - /// + ///

+ /// Current value of the + ///

public SearchResult Current { get @@ -237,23 +237,25 @@ public SearchResult Current } } - /// - /// Advances the enumerator to the next element of the collection - /// and returns a Boolean value indicating whether a valid element is available. - /// + ///

+ /// Advances the enumerator to the next element of the arrray. + ///

public bool MoveNext() { return _innerEnumerator.MoveNext(); } - /// - /// Resets the enumerator back to its initial position before the first element in the collection. - /// + ///

+ /// Resets the enumerator to the beginning of the array. + ///

public void Reset() { _innerEnumerator.Reset(); } + ///

+ /// Current of the + ///

object IEnumerator.Current => Current; } diff --git a/src/libraries/System.DirectoryServices/tests/System.DirectoryServices.Tests.csproj b/src/libraries/System.DirectoryServices/tests/System.DirectoryServices.Tests.csproj index 4dd407cbed0e47..6b6fb4db2a5f69 100644 --- a/src/libraries/System.DirectoryServices/tests/System.DirectoryServices.Tests.csproj +++ b/src/libraries/System.DirectoryServices/tests/System.DirectoryServices.Tests.csproj @@ -8,8 +8,8 @@ - + diff --git a/src/libraries/System.DirectoryServices/tests/System/DirectoryServices/DirectorySearcherTests.cs b/src/libraries/System.DirectoryServices/tests/System/DirectoryServices/DirectorySearcherTests.cs index e4d60586a7079d..4cc9031e404b04 100644 --- a/src/libraries/System.DirectoryServices/tests/System/DirectoryServices/DirectorySearcherTests.cs +++ b/src/libraries/System.DirectoryServices/tests/System/DirectoryServices/DirectorySearcherTests.cs @@ -22,7 +22,7 @@ public partial class DirectorySearcherTests public void DirectorySearch_IteratesCorrectly_SimpleEnumeration() { bool seen = false; - var e = GetDomains(); + SearchResultCollection e = GetDomains(); Assert.NotNull(e); foreach (var result in e) @@ -38,7 +38,7 @@ public void DirectorySearch_IteratesCorrectly_SimpleEnumeration() public void DirectorySearch_IteratesCorrectly_AfterCount() { bool seen = false; - var e = GetDomains(); + SearchResultCollection e = GetDomains(); Assert.NotNull(e); Assert.NotEqual(0, e.Count); From 844ea778c0aaecf1c50c91eacbebe18bb28cbe8d Mon Sep 17 00:00:00 2001 From: Marc Brooks Date: Mon, 24 Mar 2025 13:01:58 -0500 Subject: [PATCH 4/4] PR feedback Made the unit test helper use actual variable types (not var) --- .../System/DirectoryServices/DirectorySearcherTests.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.DirectoryServices/tests/System/DirectoryServices/DirectorySearcherTests.cs b/src/libraries/System.DirectoryServices/tests/System/DirectoryServices/DirectorySearcherTests.cs index 4cc9031e404b04..c9676f7ffe3d30 100644 --- a/src/libraries/System.DirectoryServices/tests/System/DirectoryServices/DirectorySearcherTests.cs +++ b/src/libraries/System.DirectoryServices/tests/System/DirectoryServices/DirectorySearcherTests.cs @@ -53,10 +53,10 @@ public void DirectorySearch_IteratesCorrectly_AfterCount() private static SearchResultCollection GetDomains() { - using var entry = new DirectoryEntry("LDAP://rootDSE"); - var namingContext = entry.Properties["configurationNamingContext"][0]!.ToString(); - using var searchRoot = new DirectoryEntry($"LDAP://CN=Partitions,{namingContext}"); - using var ds = new DirectorySearcher(searchRoot) + using DirectoryEntry entry = new DirectoryEntry("LDAP://rootDSE"); + string namingContext = entry.Properties["configurationNamingContext"][0]!.ToString(); + using DirectoryEntry searchRoot = new DirectoryEntry($"LDAP://CN=Partitions,{namingContext}"); + using DirectorySearcher ds = new DirectorySearcher(searchRoot) { PageSize = 1000, CacheResults = false