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.

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

Jump to bottom

Add nullable annotations to System.DirectoryServices #48454

Merged
merged 3 commits into from
Mar 4, 2021
Merged

Add nullable annotations to System.DirectoryServices #48454

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7a6c378
Add nullable annotations to System.DirectoryServices
krwq Feb 18, 2021
4ff89aa
apply PR feedback
krwq Feb 24, 2021
b155f55
Fix nullability of classes deriving from CollectionBase
krwq Mar 4, 2021
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
315 changes: 160 additions & 155 deletions src/libraries/System.DirectoryServices/ref/System.DirectoryServices.cs
View file
Open in desktop

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions src/libraries/System.DirectoryServices/ref/System.DirectoryServices.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
<PropertyGroup>
<TargetFrameworks>netstandard2.0</TargetFrameworks>
<IncludeInternalObsoleteAttribute>true</IncludeInternalObsoleteAttribute>
<Nullable>enable</Nullable>
</PropertyGroup>
<ItemGroup>
<Compile Include="System.DirectoryServices.cs" />
Expand Down
4 changes: 2 additions & 2 deletions src/libraries/System.DirectoryServices/src/Interop/AdsValueHelper2.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ public object GetValue()
case AdsType.ADSTYPE_NUMERIC_STRING:
case AdsType.ADSTYPE_OBJECT_CLASS:
// The value is a String.
return Marshal.PtrToStringUni(adsvalue.pointer.value);
return Marshal.PtrToStringUni(adsvalue.pointer.value)!;

case AdsType.ADSTYPE_BOOLEAN:
// The value is a bool.
Expand Down Expand Up @@ -235,7 +235,7 @@ public object GetVlvValue()
var vlv = new AdsVLV();
Marshal.PtrToStructure(adsvalue.octetString.value, vlv);

byte[] bytes = null;
byte[]? bytes = null;
if (vlv.contextID != (IntPtr)0 && vlv.contextIDlength != 0)
{
bytes = new byte[vlv.contextIDlength];
Expand Down
4 changes: 2 additions & 2 deletions src/libraries/System.DirectoryServices/src/Interop/SafeNativeMethods.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ public const int
public static extern unsafe int ADsGetLastError(out int error, char* errorBuffer, int errorBufferLength, char* nameBuffer, int nameBufferLength);

[DllImport(ExternDll.Activeds, CharSet = CharSet.Unicode)]
public static extern int ADsSetLastError(int error, string errorString, string provider);
public static extern int ADsSetLastError(int error, string? errorString, string? provider);

public class EnumVariant
{
Expand Down Expand Up @@ -93,7 +93,7 @@ private void Advance()
if (numRead[0] > 0)
{
#pragma warning disable 612, 618
_currentValue = Marshal.GetObjectForNativeVariant(addr);
_currentValue = Marshal.GetObjectForNativeVariant(addr)!;
#pragma warning restore 612, 618
}
}
Expand Down
24 changes: 12 additions & 12 deletions src/libraries/System.DirectoryServices/src/Interop/UnsafeNativeMethods.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,9 @@ internal struct Variant
internal class UnsafeNativeMethods
{
[DllImport(ExternDll.Activeds, ExactSpelling = true, EntryPoint = "ADsOpenObject", CharSet = System.Runtime.InteropServices.CharSet.Unicode)]
private static extern int IntADsOpenObject(string path, string userName, string password, int flags, [In, Out] ref Guid iid, [Out, MarshalAs(UnmanagedType.Interface)] out object ppObject);
private static extern int IntADsOpenObject(string path, string? userName, string? password, int flags, [In, Out] ref Guid iid, [Out, MarshalAs(UnmanagedType.Interface)] out object ppObject);

public static int ADsOpenObject(string path, string userName, string password, int flags, [In, Out] ref Guid iid, [Out, MarshalAs(UnmanagedType.Interface)] out object ppObject)
public static int ADsOpenObject(string path, string? userName, string? password, int flags, [In, Out] ref Guid iid, [Out, MarshalAs(UnmanagedType.Interface)] out object ppObject)
{
try
{
Expand Down Expand Up @@ -86,17 +86,17 @@ string Schema

void SetInfo();

object Get([In, MarshalAs(UnmanagedType.BStr)] string bstrName);
object? Get([In, MarshalAs(UnmanagedType.BStr)] string bstrName);

void Put([In, MarshalAs(UnmanagedType.BStr)] string bstrName, [In] object vProp);
void Put([In, MarshalAs(UnmanagedType.BStr)] string bstrName, [In] object? vProp);

[PreserveSig]
int GetEx([In, MarshalAs(UnmanagedType.BStr)] string bstrName, [Out] out object value);
int GetEx([In, MarshalAs(UnmanagedType.BStr)] string bstrName, [Out] out object? value);

void PutEx(
[In, MarshalAs(UnmanagedType.U4)] int lnControlCode,
[In, MarshalAs(UnmanagedType.BStr)] string bstrName,
[In] object vProp);
[In] object? vProp);

void GetInfoEx([In] object vProperties, [In, MarshalAs(UnmanagedType.U4)] int lnReserved);
}
Expand All @@ -116,13 +116,13 @@ object _NewEnum
get;
}

object Filter { get; set; }
object? Filter { get; set; }

object Hints { get; set; }

[return: MarshalAs(UnmanagedType.Interface)]
object GetObject(
[In, MarshalAs(UnmanagedType.BStr)] string className,
[In, MarshalAs(UnmanagedType.BStr)] string? className,
[In, MarshalAs(UnmanagedType.BStr)] string relativeName);

[return: MarshalAs(UnmanagedType.Interface)]
Expand All @@ -137,12 +137,12 @@ void Delete(
[return: MarshalAs(UnmanagedType.Interface)]
object CopyHere(
[In, MarshalAs(UnmanagedType.BStr)] string sourceName,
[In, MarshalAs(UnmanagedType.BStr)] string newName);
[In, MarshalAs(UnmanagedType.BStr)] string? newName);

[return: MarshalAs(UnmanagedType.Interface)]
object MoveHere(
[In, MarshalAs(UnmanagedType.BStr)] string sourceName,
[In, MarshalAs(UnmanagedType.BStr)] string newName);
[In, MarshalAs(UnmanagedType.BStr)] string? newName);
}

[ComImport, Guid("B2BD0902-8878-11D1-8C21-00C04FD8D503")]
Expand Down Expand Up @@ -320,8 +320,8 @@ public interface IDirectorySearch
void SetSearchPreference([In] IntPtr /*ads_searchpref_info * */pSearchPrefs, int dwNumPrefs);

void ExecuteSearch(
[In, MarshalAs(UnmanagedType.LPWStr)] string pszSearchFilter,
[In, MarshalAs(UnmanagedType.LPArray)] string[] pAttributeNames,
[In, MarshalAs(UnmanagedType.LPWStr)] string? pszSearchFilter,
[In, MarshalAs(UnmanagedType.LPArray)] string[]? pAttributeNames,
[In] int dwNumberAttributes,
[Out] out IntPtr hSearchResult);

Expand Down
1 change: 1 addition & 0 deletions src/libraries/System.DirectoryServices/src/System.DirectoryServices.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
<TargetFrameworks>$(NetCoreAppCurrent)-windows;netstandard2.0;netcoreapp2.0-windows</TargetFrameworks>
<ExcludeCurrentNetCoreAppFromPackage>true</ExcludeCurrentNetCoreAppFromPackage>
<IncludeInternalObsoleteAttribute>true</IncludeInternalObsoleteAttribute>
<Nullable>enable</Nullable>
</PropertyGroup>
<!-- DesignTimeBuild requires all the TargetFramework Derived Properties to not be present in the first property group. -->
<PropertyGroup>
Expand Down
56 changes: 28 additions & 28 deletions ...ies/System.DirectoryServices/src/System/DirectoryServices/ActiveDirectory/ADAMInstance.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,18 +14,18 @@ public class AdamInstance : DirectoryServer
private bool _disposed;

// for public properties
private string _cachedHostName;
private string? _cachedHostName;
private int _cachedLdapPort = -1;
private int _cachedSslPort = -1;
private bool _defaultPartitionInitialized;
private bool _defaultPartitionModified;
private ConfigurationSet _currentConfigSet;
private string _cachedDefaultPartition;
private AdamRoleCollection _cachedRoles;
private ConfigurationSet? _currentConfigSet;
private string? _cachedDefaultPartition;
private AdamRoleCollection? _cachedRoles;

private IntPtr _ADAMHandle = (IntPtr)0;
private IntPtr _authIdentity = IntPtr.Zero;
private SyncUpdateCallback _userDelegate;
private SyncUpdateCallback? _userDelegate;
private readonly SyncReplicaFromAllServersCallback _syncAllFunctionPointer;

#region constructors
Expand Down Expand Up @@ -55,8 +55,8 @@ internal AdamInstance(DirectoryContext context, string adamHostName, DirectoryEn

// the replica name should be in the form dnshostname:port
this.replicaName = adamHostName;
string portNumber;
Utils.SplitServerNameAndPortNumber(context.Name, out portNumber);
string? portNumber;
Utils.SplitServerNameAndPortNumber(context.Name!, out portNumber);
if (portNumber != null)
{
this.replicaName = this.replicaName + ":" + portNumber;
Expand Down Expand Up @@ -109,8 +109,8 @@ protected override void Dispose(bool disposing)

public static AdamInstance GetAdamInstance(DirectoryContext context)
{
DirectoryEntryManager directoryEntryMgr = null;
string dnsHostName = null;
DirectoryEntryManager? directoryEntryMgr = null;
string? dnsHostName = null;

// check that the context is not null
if (context == null)
Expand Down Expand Up @@ -144,7 +144,7 @@ public static AdamInstance GetAdamInstance(DirectoryContext context)
{
throw new ActiveDirectoryObjectNotFoundException(SR.Format(SR.AINotFound, context.Name), typeof(AdamInstance), context.Name);
}
dnsHostName = (string)PropertyManager.GetPropertyValue(context, rootDSE, PropertyManager.DnsHostName);
dnsHostName = (string)PropertyManager.GetPropertyValue(context, rootDSE, PropertyManager.DnsHostName)!;
Copy link
Member

Choose a reason for hiding this comment

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

Here and everywhere else PropertyManager.GetPropertyValue(context, rootDSE, PropertyManager.DnsHostName) is not returning null, instead it throws

Suggested change
dnsHostName = (string)PropertyManager.GetPropertyValue(context, rootDSE, PropertyManager.DnsHostName)!;
dnsHostName = (string)PropertyManager.GetPropertyValue(context, rootDSE, PropertyManager.DnsHostName);

Copy link
Member Author

@krwq krwq Feb 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Note it will only throw if property value is not there at all but if null value is present it will not throw and return null instead. Also note that DirectoryEntry.Properties (which this helper accesses) is public and while there is a code path guarding against null in DirectoryEntry.Properties[anyName].Value (it's a no-op) then there is nothing guarding against DirectoryEntry.Properties[anyName].Add(null).

Copy link
Member

@buyaa-n buyaa-n Feb 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

You are right, the code is not guaranteeing against null, but somehow PropertyValueCollection[index] and PropertyValueCollection.Add(object value) both documented to be non null (it says it throws ANE for null value). Literally for all references we are suppressing the return value from (string)PropertyManager.GetPropertyValue(context, rootDSE, PropertyManager.DnsHostName)! which also illustrates the intention was non-null, there is something wrong here or we are missing something.

Copy link
Member

Choose a reason for hiding this comment

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

With that being said I am resolving this and all comments related to this, please take a look at other unrelated comments

Copy link
Member

Choose a reason for hiding this comment

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

there is nothing guarding against DirectoryEntry.Properties[anyName].Add(null)

Even it is allowing null I think setting it into null should be an error, not sure who is the expert in this area, I am OK leaving this nullable for this PR but i think we should file an issue for this

Copy link
Member Author

Choose a reason for hiding this comment

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

#49128

}
catch (COMException e)
{
Expand Down Expand Up @@ -195,7 +195,7 @@ public static AdamInstance FindOne(DirectoryContext context, string partitionNam

public static AdamInstanceCollection FindAll(DirectoryContext context, string partitionName)
{
AdamInstanceCollection adamInstanceCollection = null;
AdamInstanceCollection? adamInstanceCollection = null;

// validate parameters (partitionName validated by the call to ConfigSet)
if (context == null)
Expand Down Expand Up @@ -264,7 +264,7 @@ public void SeizeRoleOwnership(AdamRole role)
{
// set the "fsmoRoleOwner" attribute on the appropriate role object
// to the NTDSAObjectName of this ADAM Instance
string roleObjectDN = null;
string? roleObjectDN = null;

CheckIfDisposed();

Expand All @@ -286,7 +286,7 @@ public void SeizeRoleOwnership(AdamRole role)
}
}

DirectoryEntry roleObjectEntry = null;
DirectoryEntry? roleObjectEntry = null;
try
{
roleObjectEntry = DirectoryEntryManager.GetDirectoryEntry(context, roleObjectDN);
Expand Down Expand Up @@ -518,7 +518,7 @@ public string HostName
if (_cachedHostName == null)
{
DirectoryEntry serverEntry = directoryEntryMgr.GetCachedDirectoryEntry(ServerObjectName);
_cachedHostName = (string)PropertyManager.GetPropertyValue(context, serverEntry, PropertyManager.DnsHostName);
_cachedHostName = (string)PropertyManager.GetPropertyValue(context, serverEntry, PropertyManager.DnsHostName)!;
stephentoub marked this conversation as resolved.
Show resolved Hide resolved
}
return _cachedHostName;
}
Expand All @@ -532,7 +532,7 @@ public int LdapPort
if (_cachedLdapPort == -1)
{
DirectoryEntry ntdsaEntry = directoryEntryMgr.GetCachedDirectoryEntry(NtdsaObjectName);
_cachedLdapPort = (int)PropertyManager.GetPropertyValue(context, ntdsaEntry, PropertyManager.MsDSPortLDAP);
_cachedLdapPort = (int)PropertyManager.GetPropertyValue(context, ntdsaEntry, PropertyManager.MsDSPortLDAP)!;
}
return _cachedLdapPort;
}
Expand All @@ -546,7 +546,7 @@ public int SslPort
if (_cachedSslPort == -1)
{
DirectoryEntry ntdsaEntry = directoryEntryMgr.GetCachedDirectoryEntry(NtdsaObjectName);
_cachedSslPort = (int)PropertyManager.GetPropertyValue(context, ntdsaEntry, PropertyManager.MsDSPortSSL);
_cachedSslPort = (int)PropertyManager.GetPropertyValue(context, ntdsaEntry, PropertyManager.MsDSPortSSL)!;
}
return _cachedSslPort;
}
Expand All @@ -559,8 +559,8 @@ public AdamRoleCollection Roles
get
{
CheckIfDisposed();
DirectoryEntry schemaEntry = null;
DirectoryEntry partitionsEntry = null;
DirectoryEntry? schemaEntry = null;
DirectoryEntry? partitionsEntry = null;

try
{
Expand All @@ -570,14 +570,14 @@ public AdamRoleCollection Roles
ArrayList roleList = new ArrayList();
schemaEntry = DirectoryEntryManager.GetDirectoryEntry(context, directoryEntryMgr.ExpandWellKnownDN(WellKnownDN.SchemaNamingContext));

if (NtdsaObjectName.Equals((string)PropertyManager.GetPropertyValue(context, schemaEntry, PropertyManager.FsmoRoleOwner)))
if (NtdsaObjectName.Equals((string)PropertyManager.GetPropertyValue(context, schemaEntry, PropertyManager.FsmoRoleOwner)!))
{
roleList.Add(AdamRole.SchemaRole);
}

partitionsEntry = DirectoryEntryManager.GetDirectoryEntry(context, directoryEntryMgr.ExpandWellKnownDN(WellKnownDN.PartitionsContainer));

if (NtdsaObjectName.Equals((string)PropertyManager.GetPropertyValue(context, partitionsEntry, PropertyManager.FsmoRoleOwner)))
if (NtdsaObjectName.Equals((string)PropertyManager.GetPropertyValue(context, partitionsEntry, PropertyManager.FsmoRoleOwner)!))
{
roleList.Add(AdamRole.NamingRole);
}
Expand All @@ -604,7 +604,7 @@ public AdamRoleCollection Roles
}
}

public string DefaultPartition
public string? DefaultPartition
{
get
{
Expand All @@ -622,7 +622,7 @@ public string DefaultPartition
}
else
{
_cachedDefaultPartition = (string)PropertyManager.GetPropertyValue(context, ntdsaEntry, PropertyManager.MsDSDefaultNamingContext);
_cachedDefaultPartition = (string)PropertyManager.GetPropertyValue(context, ntdsaEntry, PropertyManager.MsDSDefaultNamingContext)!;
stephentoub marked this conversation as resolved.
Show resolved Hide resolved
}
}
catch (COMException e)
Expand Down Expand Up @@ -667,7 +667,7 @@ public string DefaultPartition
}
}

public override string IPAddress
public override string? IPAddress
{
get
{
Expand Down Expand Up @@ -695,7 +695,7 @@ public override string SiteName
DirectoryEntry siteEntry = DirectoryEntryManager.GetDirectoryEntry(context, SiteObjectName);
try
{
cachedSiteName = (string)PropertyManager.GetPropertyValue(context, siteEntry, PropertyManager.Cn);
cachedSiteName = (string)PropertyManager.GetPropertyValue(context, siteEntry, PropertyManager.Cn)!;
stephentoub marked this conversation as resolved.
Show resolved Hide resolved
}
finally
{
Expand Down Expand Up @@ -742,7 +742,7 @@ internal string ServerObjectName
DirectoryEntry rootDSE = DirectoryEntryManager.GetDirectoryEntry(context, WellKnownDN.RootDSE);
try
{
cachedServerObjectName = (string)PropertyManager.GetPropertyValue(context, rootDSE, PropertyManager.ServerName);
cachedServerObjectName = (string)PropertyManager.GetPropertyValue(context, rootDSE, PropertyManager.ServerName)!;
}
catch (COMException e)
{
Expand All @@ -767,7 +767,7 @@ internal string NtdsaObjectName
DirectoryEntry rootDSE = DirectoryEntryManager.GetDirectoryEntry(context, WellKnownDN.RootDSE);
try
{
cachedNtdsaObjectName = (string)PropertyManager.GetPropertyValue(context, rootDSE, PropertyManager.DsServiceName);
cachedNtdsaObjectName = (string)PropertyManager.GetPropertyValue(context, rootDSE, PropertyManager.DsServiceName)!;
}
catch (COMException e)
{
Expand All @@ -790,14 +790,14 @@ internal Guid NtdsaObjectGuid
if (cachedNtdsaObjectGuid == Guid.Empty)
{
DirectoryEntry ntdsaEntry = directoryEntryMgr.GetCachedDirectoryEntry(NtdsaObjectName);
byte[] guidByteArray = (byte[])PropertyManager.GetPropertyValue(context, ntdsaEntry, PropertyManager.ObjectGuid);
byte[] guidByteArray = (byte[])PropertyManager.GetPropertyValue(context, ntdsaEntry, PropertyManager.ObjectGuid)!;
buyaa-n marked this conversation as resolved.
Show resolved Hide resolved
cachedNtdsaObjectGuid = new Guid(guidByteArray);
}
return cachedNtdsaObjectGuid;
}
}

public override SyncUpdateCallback SyncFromAllServersCallback
public override SyncUpdateCallback? SyncFromAllServersCallback
{
get
{
Expand Down
6 changes: 3 additions & 3 deletions ....DirectoryServices/src/System/DirectoryServices/ActiveDirectory/ADAMInstanceCollection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ internal AdamInstanceCollection(ArrayList values)
}
}

public AdamInstance this[int index] => (AdamInstance)InnerList[index];
public AdamInstance this[int index] => (AdamInstance)InnerList[index]!;

public bool Contains(AdamInstance adamInstance)
{
Expand All @@ -28,7 +28,7 @@ public bool Contains(AdamInstance adamInstance)

for (int i = 0; i < InnerList.Count; i++)
{
AdamInstance tmp = (AdamInstance)InnerList[i];
AdamInstance tmp = (AdamInstance)InnerList[i]!;
if (Utils.Compare(tmp.Name, adamInstance.Name) == 0)
{
return true;
Expand All @@ -46,7 +46,7 @@ public int IndexOf(AdamInstance adamInstance)

for (int i = 0; i < InnerList.Count; i++)
{
AdamInstance tmp = (AdamInstance)InnerList[i];
AdamInstance tmp = (AdamInstance)InnerList[i]!;
if (Utils.Compare(tmp.Name, adamInstance.Name) == 0)
{
return i;
Expand Down
6 changes: 3 additions & 3 deletions ...aries/System.DirectoryServices/src/System/DirectoryServices/ActiveDirectory/ADSearcher.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ public ADSearcher(DirectoryEntry searchRoot, string filter, string[] propertiesT
_searcher.PageSize = 512;
}

public ADSearcher(DirectoryEntry searchRoot, string filter, string[] propertiesToLoad, SearchScope scope, bool pagedSearch, bool cacheResults)
public ADSearcher(DirectoryEntry searchRoot, string? filter, string[] propertiesToLoad, SearchScope scope, bool pagedSearch, bool cacheResults)
{
_searcher = new DirectorySearcher(searchRoot, filter, propertiesToLoad, scope);
// set proper time out
Expand All @@ -40,13 +40,13 @@ public ADSearcher(DirectoryEntry searchRoot, string filter, string[] propertiesT
_searcher.CacheResults = cacheResults;
}

public SearchResult FindOne() => _searcher.FindOne();
public SearchResult? FindOne() => _searcher.FindOne();

public SearchResultCollection FindAll() => _searcher.FindAll();

public StringCollection PropertiesToLoad => _searcher.PropertiesToLoad;

public string Filter
public string? Filter
{
get => _searcher.Filter;
set => _searcher.Filter = value;
Expand Down
Loading