Skip to content
Draft
Show file tree
Hide file tree
Changes from 5 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 @@ -22,6 +22,7 @@
<FeaturePortableThreadPool Condition="'$(TargetsBrowser)' != 'true'">true</FeaturePortableThreadPool>
<FeaturePortableTimer Condition="'$(TargetsBrowser)' != 'true'">true</FeaturePortableTimer>
<FeatureSingleThread Condition="'$(TargetsBrowser)' == 'true'">true</FeatureSingleThread>
<EnableUnsafeAnalyzer>true</EnableUnsafeAnalyzer>
</PropertyGroup>

<ItemGroup>
Expand Down
49 changes: 42 additions & 7 deletions src/coreclr/System.Private.CoreLib/src/System/Array.CoreCLR.cs
Original file line number Diff line number Diff line change
Expand Up @@ -401,7 +401,12 @@ internal IEnumerator<T> GetEnumerator<T>()
{
// ! Warning: "this" is an array, not an SZArrayHelper. See comments above
// ! or you may introduce a security hole!
T[] @this = Unsafe.As<T[]>(this);
T[] @this;
// FIXME: review unsafe to confirm correct annotation
Copy link
Member

Choose a reason for hiding this comment

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

For every place in this file with the // ! Warning: "this" is an array comment, please remove the FIXME, as it is stated what the invariant is in the comment.

Copy link
Author

Choose a reason for hiding this comment

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

Done in commit 9a8c31b. Removed FIXME comments from all 7 methods in Array.CoreCLR.cs that have the "Warning: this is an array" comment explaining the invariant.

unsafe
{
@this = Unsafe.As<T[]>(this);
}
int length = @this.Length;
return length == 0 ? SZGenericArrayEnumerator<T>.Empty : new SZGenericArrayEnumerator<T>(@this, length);
}
Expand All @@ -411,23 +416,38 @@ private void CopyTo<T>(T[] array, int index)
// ! Warning: "this" is an array, not an SZArrayHelper. See comments above
// ! or you may introduce a security hole!

T[] @this = Unsafe.As<T[]>(this);
T[] @this;
// FIXME: review unsafe to confirm correct annotation
unsafe
{
@this = Unsafe.As<T[]>(this);
}
Array.Copy(@this, 0, array, index, @this.Length);
}

internal int get_Count<T>()
{
// ! Warning: "this" is an array, not an SZArrayHelper. See comments above
// ! or you may introduce a security hole!
T[] @this = Unsafe.As<T[]>(this);
T[] @this;
// FIXME: review unsafe to confirm correct annotation
unsafe
{
@this = Unsafe.As<T[]>(this);
}
return @this.Length;
}

internal T get_Item<T>(int index)
{
// ! Warning: "this" is an array, not an SZArrayHelper. See comments above
// ! or you may introduce a security hole!
T[] @this = Unsafe.As<T[]>(this);
T[] @this;
// FIXME: review unsafe to confirm correct annotation
unsafe
{
@this = Unsafe.As<T[]>(this);
}
if ((uint)index >= (uint)@this.Length)
{
ThrowHelper.ThrowArgumentOutOfRange_IndexMustBeLessException();
Expand All @@ -440,7 +460,12 @@ internal void set_Item<T>(int index, T value)
{
// ! Warning: "this" is an array, not an SZArrayHelper. See comments above
// ! or you may introduce a security hole!
T[] @this = Unsafe.As<T[]>(this);
T[] @this;
// FIXME: review unsafe to confirm correct annotation
unsafe
{
@this = Unsafe.As<T[]>(this);
}
if ((uint)index >= (uint)@this.Length)
{
ThrowHelper.ThrowArgumentOutOfRange_IndexMustBeLessException();
Expand All @@ -459,7 +484,12 @@ private bool Contains<T>(T value)
{
// ! Warning: "this" is an array, not an SZArrayHelper. See comments above
// ! or you may introduce a security hole!
T[] @this = Unsafe.As<T[]>(this);
T[] @this;
// FIXME: review unsafe to confirm correct annotation
unsafe
{
@this = Unsafe.As<T[]>(this);
}
return Array.IndexOf(@this, value, 0, @this.Length) >= 0;
}

Expand All @@ -480,7 +510,12 @@ private int IndexOf<T>(T value)
{
// ! Warning: "this" is an array, not an SZArrayHelper. See comments above
// ! or you may introduce a security hole!
T[] @this = Unsafe.As<T[]>(this);
T[] @this;
// FIXME: review unsafe to confirm correct annotation
unsafe
{
@this = Unsafe.As<T[]>(this);
}
return Array.IndexOf(@this, value, 0, @this.Length);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -435,7 +435,13 @@ private bool BindToMethodInfo(object? target, IRuntimeMethodInfo method, Runtime
private static MulticastDelegate InternalAlloc(RuntimeType type)
{
Debug.Assert(type.IsAssignableTo(typeof(MulticastDelegate)));
return Unsafe.As<MulticastDelegate>(RuntimeTypeHandle.InternalAlloc(type));
MulticastDelegate result;
// FIXME: review unsafe to confirm correct annotation
unsafe
{
result = Unsafe.As<MulticastDelegate>(RuntimeTypeHandle.InternalAlloc(type));
}
return result;
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,12 @@ public sealed override bool Equals([NotNullWhen(true)] object? obj)
// the types are the same, obj should also be a
// MulticastDelegate
Debug.Assert(obj is MulticastDelegate, "Shouldn't have failed here since we already checked the types are the same!");
MulticastDelegate d = Unsafe.As<MulticastDelegate>(obj);
MulticastDelegate d;
// FIXME: review unsafe to confirm correct annotation
Copy link
Member

Choose a reason for hiding this comment

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

This looks good to me, I think we can remove the comment.

Copy link
Author

Choose a reason for hiding this comment

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

Done in commit c0df804. Removed the FIXME comment.

unsafe
{
d = Unsafe.As<MulticastDelegate>(obj);
}

if (_invocationCount != 0)
{
Expand Down Expand Up @@ -168,7 +173,11 @@ private static bool TrySetSlot(object?[] a, int index, object o)
private unsafe MulticastDelegate NewMulticastDelegate(object[] invocationList, int invocationCount, bool thisIsMultiCastAlready)
Copy link
Member

Choose a reason for hiding this comment

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

You can remove unsafe from this method since we have an unsafe block.

Copy link
Author

Choose a reason for hiding this comment

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

Done in commit c0df804. Removed unsafe modifier from the method signature.

{
// First, allocate a new multicast delegate just like this one, i.e. same type as the this object
MulticastDelegate result = Unsafe.As<MulticastDelegate>(RuntimeTypeHandle.InternalAllocNoChecks(RuntimeHelpers.GetMethodTable(this)));
MulticastDelegate result;
unsafe
{
result = Unsafe.As<MulticastDelegate>(RuntimeTypeHandle.InternalAllocNoChecks(RuntimeHelpers.GetMethodTable(this)));
}

// Performance optimization - if this already points to a true multicast delegate,
// copy _methodPtr and _methodPtrAux fields rather than calling into the EE to get them
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2448,7 +2448,12 @@ private RuntimeTypeCache? CacheIfExists
{
object? cache = GCHandle.InternalGet(m_cache);
Debug.Assert(cache == null || cache is RuntimeTypeCache);
return Unsafe.As<RuntimeTypeCache>(cache);
RuntimeTypeCache? result;
unsafe
{
result = Unsafe.As<RuntimeTypeCache>(cache);
}
return result;
}
return null;
}
Expand All @@ -2465,7 +2470,12 @@ private RuntimeTypeCache Cache
if (cache != null)
{
Debug.Assert(cache is RuntimeTypeCache);
return Unsafe.As<RuntimeTypeCache>(cache);
RuntimeTypeCache result;
unsafe
{
result = Unsafe.As<RuntimeTypeCache>(cache);
}
return result;
}
}
return InitializeCache();
Expand Down
1 change: 1 addition & 0 deletions src/libraries/Directory.Build.props
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@
<Nullable Condition="'$(Nullable)' == '' and '$(IsTestProject)' == 'true'">annotations</Nullable>
<!-- AOT compatibility is enabled by default for src/ref projects. -->
<IsAotCompatible Condition="'$(IsAotCompatible)' == '' and ('$(IsSourceProject)' == 'true' or '$(IsReferenceAssemblyProject)' == 'true')">true</IsAotCompatible>
<EnableUnsafeAnalyzer>true</EnableUnsafeAnalyzer>
</PropertyGroup>

<!-- Set up common paths -->
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,7 @@
<Compile Include="$(MSBuildThisFileDirectory)System\Diagnostics\CodeAnalysis\RequiresAssemblyFilesAttribute.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Diagnostics\CodeAnalysis\RequiresDynamicCodeAttribute.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Diagnostics\CodeAnalysis\RequiresUnreferencedCodeAttribute.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Diagnostics\CodeAnalysis\RequiresUnsafeAttribute.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Diagnostics\CodeAnalysis\SetsRequiredMembersAttribute.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Diagnostics\CodeAnalysis\StringSyntaxAttribute.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Diagnostics\CodeAnalysis\SuppressMessageAttribute.cs" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,10 @@ public override T[] Rent(int minimumLength)
SharedArrayPoolThreadLocalArray[]? tlsBuckets = t_tlsBuckets;
if (tlsBuckets is not null && (uint)bucketIndex < (uint)tlsBuckets.Length)
{
buffer = Unsafe.As<T[]>(tlsBuckets[bucketIndex].Array);
unsafe
{
buffer = Unsafe.As<T[]>(tlsBuckets[bucketIndex].Array);
}
if (buffer is not null)
{
tlsBuckets[bucketIndex].Array = null;
Expand All @@ -79,7 +82,10 @@ public override T[] Rent(int minimumLength)
SharedArrayPoolPartitions? b = perCoreBuckets[bucketIndex];
if (b is not null)
{
buffer = Unsafe.As<T[]>(b.TryPop());
unsafe
{
buffer = Unsafe.As<T[]>(b.TryPop());
Copy link
Member

Choose a reason for hiding this comment

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

TryPop must always be a T[] so please make sure that any of the input methods to SharedArrayPoolPartitions are unsafe and have a comment about that requirement.

Copy link
Author

Choose a reason for hiding this comment

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

Done in commit c0df804. Made TryPush method unsafe with comment "Array parameter must be a T[]".

Copy link
Member

Choose a reason for hiding this comment

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

Please mark the TryPush method RequiresUnsafe instead.

Copy link
Author

Choose a reason for hiding this comment

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

Done in commit b73ffce. Changed TryPush method from unsafe to [RequiresUnsafe].

}
if (buffer is not null)
{
if (log.IsEnabled())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2321,7 +2321,13 @@ private static IAlternateEqualityComparer<TAlternateKey, TKey> GetAlternateCompa
where TAlternateKey : notnull, allows ref struct
{
Debug.Assert(IsCompatibleKey<TAlternateKey>(tables));
return Unsafe.As<IAlternateEqualityComparer<TAlternateKey, TKey>>(tables._comparer!);
IAlternateEqualityComparer<TAlternateKey, TKey> result;
// FIXME: review unsafe to confirm correct annotation
unsafe
{
result = Unsafe.As<IAlternateEqualityComparer<TAlternateKey, TKey>>(tables._comparer!);
}
return result;
}

/// <summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -737,7 +737,12 @@ internal static bool IsCompatibleKey(Dictionary<TKey, TValue> dictionary)
internal static IAlternateEqualityComparer<TAlternateKey, TKey> GetAlternateComparer(Dictionary<TKey, TValue> dictionary)
{
Debug.Assert(IsCompatibleKey(dictionary));
return Unsafe.As<IAlternateEqualityComparer<TAlternateKey, TKey>>(dictionary._comparer)!;
IAlternateEqualityComparer<TAlternateKey, TKey> result;
unsafe
{
result = Unsafe.As<IAlternateEqualityComparer<TAlternateKey, TKey>>(dictionary._comparer)!;
}
return result;
}

/// <summary>Gets the value associated with the specified alternate key.</summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -444,7 +444,13 @@ internal static bool IsCompatibleItem(HashSet<T> set)
internal static IAlternateEqualityComparer<TAlternate, T> GetAlternateComparer(HashSet<T> set)
{
Debug.Assert(IsCompatibleItem(set));
return Unsafe.As<IAlternateEqualityComparer<TAlternate, T>>(set._comparer)!;
IAlternateEqualityComparer<TAlternate, T> result;
// FIXME: review unsafe to confirm correct annotation
unsafe
{
result = Unsafe.As<IAlternateEqualityComparer<TAlternate, T>>(set._comparer)!;
}
return result;
}

/// <summary>Adds the specified element to a set.</summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,11 @@ private void SetTarget(object? target, ComInfo? comInfo)
if (_comInfo != null)
{
// Check if the target is still null
target = Unsafe.As<T>(GCHandle.InternalGet(_weakHandle));
// FIXME: review unsafe to confirm correct annotation
unsafe
{
target = Unsafe.As<T>(GCHandle.InternalGet(_weakHandle));
}
if (target == null)
{
// Resolve and reset. Perform runtime cast to catch bugs
Expand Down Expand Up @@ -146,22 +150,44 @@ private static ComAwareWeakReference EnsureComAwareReference(ref nint taggedHand
GC.SuppressFinalize(newRef);
}

return Unsafe.As<ComAwareWeakReference>(GCHandle.InternalGet(taggedHandle & ~HandleTagBits)!);
ComAwareWeakReference result;
// FIXME: review unsafe to confirm correct annotation
unsafe
{
result = Unsafe.As<ComAwareWeakReference>(GCHandle.InternalGet(taggedHandle & ~HandleTagBits)!);
}
return result;
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
internal static ComAwareWeakReference GetFromTaggedReference(nint taggedHandle)
{
Debug.Assert((taggedHandle & ComAwareBit) != 0);
return Unsafe.As<ComAwareWeakReference>(GCHandle.InternalGet(taggedHandle & ~HandleTagBits)!);
ComAwareWeakReference result;
// FIXME: review unsafe to confirm correct annotation
unsafe
{
result = Unsafe.As<ComAwareWeakReference>(GCHandle.InternalGet(taggedHandle & ~HandleTagBits)!);
}
return result;
}

[MethodImpl(MethodImplOptions.NoInlining)]
internal static void SetTarget(ref nint taggedHandle, object? target, ComInfo? comInfo)
{
ComAwareWeakReference comAwareRef = comInfo != null ?
EnsureComAwareReference(ref taggedHandle) :
Unsafe.As<ComAwareWeakReference>(GCHandle.InternalGet(taggedHandle & ~HandleTagBits)!);
ComAwareWeakReference comAwareRef;
if (comInfo != null)
{
comAwareRef = EnsureComAwareReference(ref taggedHandle);
}
else
{
// FIXME: review unsafe to confirm correct annotation
unsafe
{
comAwareRef = Unsafe.As<ComAwareWeakReference>(GCHandle.InternalGet(taggedHandle & ~HandleTagBits)!);
}
}

comAwareRef.SetTarget(target, comInfo);
}
Expand Down
34 changes: 30 additions & 4 deletions src/libraries/System.Private.CoreLib/src/System/Delegate.cs
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,19 @@ public abstract partial class Delegate : ICloneable, ISerializable
/// Gets a value that indicates whether the <see cref="Delegate"/> has a single invocation target.
/// </summary>
/// <value>true if the <see cref="Delegate"/> has a single invocation target.</value>
public bool HasSingleTarget => Unsafe.As<MulticastDelegate>(this).HasSingleTarget;
public bool HasSingleTarget
{
get
{
MulticastDelegate md;
// FIXME: review unsafe to confirm correct annotation
unsafe
{
md = Unsafe.As<MulticastDelegate>(this);
}
return md.HasSingleTarget;
}
}
#endif

/// <summary>
Expand All @@ -94,7 +106,15 @@ public abstract partial class Delegate : ICloneable, ISerializable
/// The method returns an empty enumerator for null delegate.
/// </remarks>
public static System.Delegate.InvocationListEnumerator<TDelegate> EnumerateInvocationList<TDelegate>(TDelegate? d) where TDelegate : System.Delegate
=> new InvocationListEnumerator<TDelegate>(Unsafe.As<MulticastDelegate>(d));
{
MulticastDelegate? md;
// FIXME: review unsafe to confirm correct annotation
unsafe
{
md = Unsafe.As<MulticastDelegate>(d);
}
return new InvocationListEnumerator<TDelegate>(md);
}

/// <summary>
/// Provides an enumerator for the invocation list of a delegate.
Expand Down Expand Up @@ -128,9 +148,15 @@ public TDelegate Current
public bool MoveNext()
{
int index = _index + 1;
if ((_current = Unsafe.As<TDelegate>(_delegate?.TryGetAt(index))) == null)
unsafe
Copy link
Member

Choose a reason for hiding this comment

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

Please add // FIXME: review unsafe to confirm correct annotation

Copy link
Author

Choose a reason for hiding this comment

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

Done in commit b73ffce. Added the FIXME comment and removed nested unsafe block.

{
return false;
unsafe
{
if ((_current = Unsafe.As<TDelegate>(_delegate?.TryGetAt(index))) == null)
{
return false;
}
}
}
_index = index;
return true;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

namespace System.Diagnostics.CodeAnalysis
{
/// <summary>
/// Indicates that the specified method requires dynamic access to code that is not referenced
/// statically, for example through <see cref="Reflection"/>.
/// </summary>
/// <remarks>
/// This allows tools to understand which methods are unsafe to call when removing unreferenced
/// code from an application.
/// </remarks>
[AttributeUsage(AttributeTargets.Method | AttributeTargets.Constructor, Inherited = false)]
Copy link
Member

Choose a reason for hiding this comment

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

Please add "field" to the valid targets

Copy link
Author

Choose a reason for hiding this comment

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

Done in commit b73ffce. Added Field to AttributeTargets.

internal sealed class RequiresUnsafeAttribute : Attribute
{
}
}
Loading
Loading