Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix break in DirectoryServices SearchResultCollection #113775

Merged
merged 4 commits into from
Mar 25, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -47,15 +47,17 @@ private ArrayList InnerList
{
if (_innerList == null)
{
_innerList = new ArrayList();
var eagerList = new ArrayList();
var enumerator = new ResultsEnumerator(
this,
_rootEntry.GetUsername(),
_rootEntry.GetPassword(),
_rootEntry.AuthenticationType);

while (enumerator.MoveNext())
_innerList.Add(enumerator.Current);
eagerList.Add(enumerator.Current);

_innerList = eagerList;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change to build the inner list in it's entirety and then set the member is to ensure that if an exception in thrown while populating this inner list we do NOT memoize the incomplete list because then we would (if enumerated again) not throw again and would instead silently only yield the entries we previously gathered. Of course anyone attempting _re_enumeration of a list that yielded an error the first time is probably wrong, but why add to the problem?

}

return _innerList;
Expand Down Expand Up @@ -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);
Expand All @@ -214,6 +214,51 @@ void ICollection.CopyTo(Array array, int index)
InnerList.CopyTo(array, index);
}

/// <summary>Provides an enumerator implementation for the array of <see cref="SearchResult"/>.</summary>
private struct AlreadyReadResultsEnumerator : IEnumerator
{
private readonly IEnumerator _innerEnumerator;

/// <summary>Initialize the enumerator.</summary>
/// <param name="innerList">The ArrayList to enumerate.</param>
internal AlreadyReadResultsEnumerator(ArrayList innerList)
{
_innerEnumerator = innerList.GetEnumerator();
}

/// <summary>
/// Current <see cref="SearchResult"/> value of the <see cref="IEnumerator"/>
/// </summary>
public SearchResult Current
{
get
{
return (SearchResult)(_innerEnumerator.Current);
}
}

/// <summary>
/// Advances the enumerator to the next <see cref="SearchResult"/> element of the arrray.
/// </summary>
public bool MoveNext()
{
return _innerEnumerator.MoveNext();
}

/// <summary>
/// Resets the enumerator to the beginning of the array.
/// </summary>
public void Reset()
{
_innerEnumerator.Reset();
}

/// <summary>
/// Current <see cref="object"/> of the <see cref="IEnumerator"/>
/// </summary>
object IEnumerator.Current => Current;
}

/// <devdoc>
/// Supports a simple ForEach-style iteration over a collection.
/// </devdoc>
Expand Down Expand Up @@ -310,6 +355,7 @@ public bool MoveNext()
return false;

_currentResult = null;

if (!_initialized)
{
int hr = _results.SearchObject.GetFirstRow(_results.Handle);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
<Compile Include="System\DirectoryServices\ActiveDirectorySecurityTests.cs" />
<Compile Include="System\DirectoryServices\ActiveDirectory\DomainControllerTests.cs" />
<Compile Include="System\DirectoryServices\DirectoryEntryTests.cs" />
<Compile Include="System\DirectoryServices\DirectorySearcherTests.cs" />
<Compile Include="System\DirectoryServices\DirectoryServicesPermissionTests.cs" />
<Compile Include="System\DirectoryServices\DirectoryServicesTests.cs" />
<Compile Include="System\DirectoryServices\DirectorySynchronizationTests.cs" />
Expand All @@ -20,8 +21,7 @@
<Compile Include="System\DirectoryServices\ActiveDirectory\ForestTests.cs" />
<Compile Include="System\DirectoryServices\ActiveDirectory\ActiveDirectoryTests.cs" />
<Compile Include="System\DirectoryServices\ActiveDirectory\TrustHelperTests.cs" />
<Compile Include="$(CommonTestPath)System\DirectoryServices\LdapConfiguration.cs"
Link="Common\DirectoryServices\LdapConfiguration.cs" />
<Compile Include="$(CommonTestPath)System\DirectoryServices\LdapConfiguration.cs" Link="Common\DirectoryServices\LdapConfiguration.cs" />
</ItemGroup>
<ItemGroup>
<Content Include="$(CommonTestPath)System\DirectoryServices\LDAP.Configuration.xml">
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
// 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()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did all of these new tests fail prior?

Copy link
Contributor Author

@IDisposable IDisposable Mar 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There were never any tests against this class as far as I could find, so no :)

The two tests come from from @stephentoub's minimal reproduction in this comment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, that might have been unclear... the first test would have always succeeded (and I put this test in to ensure I didn't break anything). The second test would have failed without the changes in this PR.

{
bool seen = false;
SearchResultCollection 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;
SearchResultCollection e = GetDomains();
Assert.NotNull(e);
Assert.NotEqual(0, e.Count);

foreach (var result in e)
{
Assert.NotNull(result);
seen = true;
}

Assert.True(seen);
}

private static SearchResultCollection GetDomains()
{
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
};
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();
}
}
}
Loading