Skip to content

Dispose LdapConnections used by PrincipalContext.ValidateCredentials #62036

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.

Sign up for GitHub

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

Merged
joperezr merged 1 commit into from
Jan 23, 2022
Merged

Dispose LdapConnections used by PrincipalContext.ValidateCredentials #62036

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 11 additions & 1 deletion ...ctoryServices.AccountManagement/src/System/DirectoryServices/AccountManagement/Context.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ internal static class CapabilityMap
public const string LDAP_CAP_ACTIVE_DIRECTORY_V61_OID = "1.2.840.113556.1.4.1935";
}

internal sealed class CredentialValidator
internal sealed class CredentialValidator : IDisposable
{
private enum AuthMethod
{
Expand Down Expand Up @@ -341,6 +341,14 @@ public bool Validate(string userName, string password, ContextOptions connection
return (BindSam(_serverName, userName, password));
}
}

public void Dispose()
{
foreach (LdapConnection connection in _connCache.Values)
{
connection.Dispose();
}
}
Copy link
Contributor Author

@juliushardt juliushardt Nov 25, 2021

Choose a reason for hiding this comment

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

In the other methods of CredentialValidator, access to _connCache is guarded by locks. However, I'm not sure what the guidelines with respect to multithreading in IDisposable.Dispose() methods are. I assumed that Dispose() is only supposed to be called when the object is definitely not used anymore (by any thread), so adding a lock may not be necessary. If this is not in line with the guildelines, feel free to tell me and I will update the PR.

}
// ********************************************
public class PrincipalContext : IDisposable
Expand Down Expand Up @@ -1014,6 +1022,8 @@ public void Dispose()
if (_queryCtx != null)
_queryCtx.Dispose();

_credValidate.Dispose();
Copy link
Contributor Author

@juliushardt juliushardt Nov 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

We do not need a null check here because the constructor of PrincipalContext always assigns an instance of CredentialValidator to _credValidate and this is the only place where _credValidate is modified (the field is readonly).

Copy link
Contributor

@Alexx999 Alexx999 Jan 22, 2022

Choose a reason for hiding this comment

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

The funny bit to remember about readonly fields is that they might be null if the class has a finalizer. Namely, if the constructor throws before initializing such a field you will get a finalizer call where the field will be null.
In this case, no finalizer so it should be OK to omit a null check.


_disposed = true;
GC.SuppressFinalize(this);
}
Expand Down