Skip to content
Draft
Show file tree
Hide file tree
Changes from 7 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
42 changes: 35 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,11 @@ 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;
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 +415,35 @@ 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;
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;
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;
unsafe
{
@this = Unsafe.As<T[]>(this);
}
if ((uint)index >= (uint)@this.Length)
{
ThrowHelper.ThrowArgumentOutOfRange_IndexMustBeLessException();
Expand All @@ -440,7 +456,11 @@ 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;
unsafe
{
@this = Unsafe.As<T[]>(this);
}
if ((uint)index >= (uint)@this.Length)
{
ThrowHelper.ThrowArgumentOutOfRange_IndexMustBeLessException();
Expand All @@ -459,7 +479,11 @@ 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;
unsafe
{
@this = Unsafe.As<T[]>(this);
}
return Array.IndexOf(@this, value, 0, @this.Length) >= 0;
}

Expand All @@ -480,7 +504,11 @@ 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;
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,11 @@ 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));
// FIXME: review unsafe to confirm correct annotation
unsafe
{
return Unsafe.As<MulticastDelegate>(RuntimeTypeHandle.InternalAlloc(type));
}
}

[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,10 @@ private RuntimeTypeCache? CacheIfExists
{
object? cache = GCHandle.InternalGet(m_cache);
Debug.Assert(cache == null || cache is RuntimeTypeCache);
return Unsafe.As<RuntimeTypeCache>(cache);
unsafe
{
return Unsafe.As<RuntimeTypeCache>(cache);
}
}
return null;
}
Expand All @@ -2465,7 +2468,10 @@ private RuntimeTypeCache Cache
if (cache != null)
{
Debug.Assert(cache is RuntimeTypeCache);
return Unsafe.As<RuntimeTypeCache>(cache);
unsafe
{
return Unsafe.As<RuntimeTypeCache>(cache);
}
}
}
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,11 @@ 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!);
// FIXME: review unsafe to confirm correct annotation
unsafe
{
return Unsafe.As<IAlternateEqualityComparer<TAlternateKey, TKey>>(tables._comparer!);
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 unsafe to _comparer and a comment that it must be IAlternateEqualityComparer<TAlternateKey, TKey>. Please also remove the unsafe block and add RequiresUnsafe to the parent method.

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. Added unsafe to _comparer field with comment "Must be IAlternateEqualityComparer<TAlternateKey, TKey>", removed unsafe block, and added RequiresUnsafe to GetAlternateComparer.

}
}

/// <summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -737,7 +737,10 @@ 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)!;
unsafe
{
return Unsafe.As<IAlternateEqualityComparer<TAlternateKey, TKey>>(dictionary._comparer)!;
Copy link
Member

Choose a reason for hiding this comment

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

Same feedback here around _comparer.

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. Added unsafe to _comparer field with appropriate comment, removed unsafe block, and added RequiresUnsafe attribute.

}
}

/// <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,11 @@ 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)!;
// FIXME: review unsafe to confirm correct annotation
unsafe
{
return Unsafe.As<IAlternateEqualityComparer<TAlternate, T>>(set._comparer)!;
Copy link
Member

Choose a reason for hiding this comment

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

Same feedback here around _comparer.

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. Added unsafe to _comparer field with appropriate comment, removed unsafe block, and added RequiresUnsafe attribute.

}
}

/// <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,40 @@ private static ComAwareWeakReference EnsureComAwareReference(ref nint taggedHand
GC.SuppressFinalize(newRef);
}

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

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

[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
30 changes: 26 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,17 @@ 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
{
// FIXME: review unsafe to confirm correct annotation
unsafe
{
return Unsafe.As<MulticastDelegate>(this).HasSingleTarget;
}
}
}
#endif

/// <summary>
Expand All @@ -94,7 +104,13 @@ 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));
{
// FIXME: review unsafe to confirm correct annotation
unsafe
{
return new InvocationListEnumerator<TDelegate>(Unsafe.As<MulticastDelegate>(d));
}
}

/// <summary>
/// Provides an enumerator for the invocation list of a delegate.
Expand Down Expand Up @@ -128,9 +144,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
{
}
}
13 changes: 11 additions & 2 deletions src/libraries/System.Private.CoreLib/src/System/IO/BinaryReader.cs
Original file line number Diff line number Diff line change
Expand Up @@ -357,7 +357,11 @@ private int InternalReadChars(Span<char> buffer)
if (_isMemoryStream)
{
Debug.Assert(_stream is MemoryStream);
MemoryStream mStream = Unsafe.As<MemoryStream>(_stream);
MemoryStream mStream;
unsafe
{
mStream = Unsafe.As<MemoryStream>(_stream);
}

int position = mStream.InternalGetPosition();
numBytes = mStream.InternalEmulateRead(numBytes);
Expand Down Expand Up @@ -477,7 +481,12 @@ private ReadOnlySpan<byte> InternalRead(Span<byte> buffer)
{
// read directly from MemoryStream buffer
Debug.Assert(_stream is MemoryStream);
return Unsafe.As<MemoryStream>(_stream).InternalReadSpan(buffer.Length);
MemoryStream mStream;
unsafe
{
mStream = Unsafe.As<MemoryStream>(_stream);
}
return mStream.InternalReadSpan(buffer.Length);
}
else
{
Expand Down
Loading
Loading