Remove a number of calls to Unsafe.AsPointer

@tannergooding

There are a number of unnecessary uses of Unsafe.AsPointer in this repository (e.g.

runtime/src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs

Line 868 in eaa6f01

_EnumerateConfigurationValues(Unsafe.AsPointer(ref context), &ConfigCallback);

) - this is a extra dangerous API that can easily be used wrongly (see #99138 as an example that slipped by a number of people). While the current remaining uses seem safe currently at a glance, we might be better off replacing a number of the unnecessary uses with other APIs, and/or adding a comment explaining why it's safe in this case (like in

runtime/src/libraries/System.Private.CoreLib/src/System/Number.BigInteger.cs

Line 1237 in 9863bf1

    
           (byte*)Unsafe.AsPointer(ref _blocks[0]), // This is safe to do since we are a ref struct

), and disrecommending their use in the future without a very good reason.

Most of the uses can be broken in to 2 categories:

Aligning or detecting alignment of a reference (we can introduce helper APIs for this, e.g.

runtime/src/libraries/System.Private.CoreLib/src/System/Text/Ascii.cs

Line 144 in eaa6f01

nuint misalignedElements = ((nuint)Unsafe.AsPointer(ref searchSpace) & (nuint)(Vector256<byte>.Count - 1)) / (nuint)sizeof(T);

)
Getting a pointer to something that is known to be pinned (many of these cases could be converted to ref fields instead, using refs/spans directly, or passing by pointer, e.g.

runtime/src/mono/System.Private.CoreLib/src/System/Reflection/TypeNameParser.Mono.cs

Line 48 in eaa6f01

_stackMark = Unsafe.AsPointer(ref stackMark)

)

The idea was mentioned by @tannergooding on the discord, and one of the suggested solutions was to create an internal API bool IsOpportunisticallyAligned(ref T address, nuint aligment) or similar for the alignment uses.

I am happy to make PRs for these. @tannergooding suggested that I make one for each area separately. Just making this issue to track it and get support for the general idea from people.

/cc @stephentoub @jkotas @tannergooding

I'm generally supportive of removing unsafe code in favor of safer patterns, particularly where newer features like ref fields can be used to avoid the issue or special helpers like some bool IsOpportunisticallyAligned(ref T address, nuint aligment) could exist to document the intent and centralize the logic.

Notably a lot of these places where its being used are fairly core APIs, so there is risk in making changes to them even if to remove unsafe code. It's something that we need to be extra cautious around and ensure gets the right scrutiny.

List of areas with calls to Unsafe.AsPointer (tick indicates it's been reviewed and/or had a PR made for it):

Common/System.Private.CoreLib
Runtime.Base (nativeaot)
System.Private.TypeLoader (nativeaot)
System.Numerics
Microsoft.CSharp
Microsoft.Extensions.Logging.EventSource
System.Memory
System.Net.Sockets (tests)
System.Diagnostics.Tracing
System.Globalization
System.Runtime.CompilerServices
System.Runtime.InteropServices
System.Runtime.InteropServices.Marshalling
System.Text
System.Text.Unicode
System.Threading
System.Threading.Tasks
System.Runtime.InteropServices.JavaScript
System.Security.Cryptography
System.Text.Json
System.Reflection (mono)
Other code (HostRuntimeContract.cs, MonoSDBHelper.cs, simple-raytracer/Program.cs, poison.cs, classloader/generics/Pointers/Pointers.cs, universal_generics.cs)
Some regression tests - Runtime_61510.cs, Runtime_92349.cs

_EnumerateConfigurationValues(Unsafe.AsPointer(ref context), &ConfigCallback);

Why do you think that this use is unnecessary? What would you replace it with (while preserving the perf characteristics). I do not see a problem with this one.

this is a extra dangerous API that can easily be used wrongly

No much different from every other Unsafe.* API. You have to know what you are doing when using Unsafe.* APIs.

IsOpportunisticallyAligned

Sounds good to me. I see ~5 places where this can be used.

Getting a pointer to something that is known to be pinned (many of these cases could be converted to ref fields instead

I am not convinced that it is that easy. Many of these uses have to do with low-level interop.

_EnumerateConfigurationValues(Unsafe.AsPointer(ref context), &ConfigCallback);

Why do you think that this use is unnecessary? What would you replace it with (while preserving the perf characteristics). I do not see a problem with this one.

_EnumerateConfigurationValues(&context, &ConfigCallback);

_EnumerateConfigurationValues(&context, &ConfigCallback);

Ok this works but it requires pragma disable.

IsOpportunisticallyAligned

Sounds good to me. I see ~5 places where this can be used.

Which file/type should I define this on?

It can be internal method on System.Runtime.CompilerServices.Unsafe

_EnumerateConfigurationValues(&context, &ConfigCallback);

Ok this works but it requires pragma disable.

Do we want to disable it globally like it was mentioned in #80484 (review)?

Do we want to disable it globally like it was mentioned in #80484 (review)?

This issue is itself about trying to reduce unsafe patterns where unnecessary. Globally suppressing the warning that you're taking a pointer to a managed object is kind-of counter intuitive to that ;)

I think we want to be explicit when we're taking a pointer to a managed object to help reduce risk and make it very apparent we're taking the necessary approaches to keep the underlying object alive, etc.

In my opinion, Unsafe.AsPointer is more clear than a disabled CS8500 combined with the & operator. One of the reasons why I prefer it is the disabled CS8500 applies to entire lines of code between the disable/restore, while the use of Unsafe.AsPointer applies only to the exact call where it was needed.

It's also not clear to me how the changes proposed here would have prevented the GC hole mentioned in the original post above.

In my opinion, Unsafe.AsPointer is more clear than a disabled CS8500 combined with the & operator. One of the reasons why I prefer it is the disabled CS8500 applies to entire lines of code between the disable/restore, while the use of Unsafe.AsPointer applies only to the exact call where it was needed.

CS8500 is a bit of a silly warning anyway imo. The issue with Unsafe.AsPointer is that it will happily allow unsafe cases where the memory is not fixed, whereas & will not.

e.g., consider I have this:

SomeType v = ...;
...
CallMethod(&v);

if I moved (or otherwise have a) v anywhere that's not obviously on the stack I will get an error, whereas Unsafe.AsPointer will happily continue to "work" even when not pinned otherwise.

It's also not clear to me how the changes proposed here would have prevented the GC hole mentioned in the original post above.

If we avoid using Unsafe.AsPointer without a good reason that other alternatives couldn't work, then it would have been more likely to have been questioned and caught.

Navigation Menu

Repository navigation

Remove a number of calls to `Unsafe.AsPointer` #99144

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Remove a number of calls to Unsafe.AsPointer #99144

Description

Activity

tannergooding commented on Mar 1, 2024

hamarb123 commented on Mar 1, 2024

jkotas commented on Mar 1, 2024

hamarb123 commented on Mar 1, 2024

jkotas commented on Mar 1, 2024

hamarb123 commented on Mar 1, 2024

jkotas commented on Mar 1, 2024

MichalPetryka commented on Mar 1, 2024

tannergooding commented on Mar 1, 2024

sharwell commented on Mar 4, 2024

sharwell commented on Mar 4, 2024

hamarb123 commented on Mar 4, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

Remove a number of calls to `Unsafe.AsPointer` #99144