iot: `Iot.Device.Common.ValueArray` is not safe and contains GC holes
Describe the bug I want to preface this by stating that I’m not a consumer of these APIs, I just happened to come across this while perusing source.dot.net.
The ValueArray<T> type introduced in https://github.com/dotnet/iot/pull/1701 stores a sequence of 8 elements, and allows consumers to retrieve a Span<T> wrapping them. However, the method for which it does this is not safe and causes the GC to lose track of the references, meaning that the object they belong to may be eligible for garbage collection while the Span<T> is in use.
For reference, this is the current implementation of ValueArray<T>.AsSpan:
public unsafe Span<T> AsSpan()
{
return new Span<T>(Unsafe.AsPointer(ref _e0), _count);
}
And this would be an implementation that retains GC tracking (compatible with .NET Core/Standard 2.1 and newer):
public Span<T> AsSpan()
{
return MemoryMarshal.CreateSpan(ref _e0, _count);
}
Unfortunately, MemoryMarshal.CreateSpan is not available on .NET Standard 2.0. A close equivalent to this API used to exist that would have made it possible to create Span<T> instances for ValueArray<T>s stored within classes safely (see https://github.com/dotnet/runtime/issues/27690 and https://github.com/dotnet/runtime/issues/24562).
As the ValueArray<T> type is stored within classes, changing it to a ref struct is also not an option, meaning that there is no safe way to implement AsSpan on ValueArray<T> under .NET Standard 2.0.
About this issue
- Original URL
- State: closed
- Created 2 years ago
- Reactions: 5
- Comments: 17 (9 by maintainers)
Commits related to this issue
- Fix a possible GC hole See issue #1886. Will need another update when the new [RefEscapes] attribute is available, to make this finally really safe. Note: This removes the public AsSpan() method for... — committed to pgrawehr/iot by pgrawehr 2 years ago
- Fix a possible GC hole See issue #1886. Will need another update when the new [RefEscapes] attribute is available, to make this finally really safe. Note: This removes the public AsSpan() method for... — committed to pgrawehr/iot by pgrawehr 2 years ago
- Fix a possible GC hole (#1890) * Fix a possible GC hole See issue #1886. Will need another update when the new [RefEscapes] attribute is available, to make this finally really safe. Note: Thi... — committed to dotnet/iot by pgrawehr 2 years ago
An
unmanagedtype is merely astructthat doesn’t contain any GC pointers. I assume you meant that it does not allow managed types? This is also not true though, as while it disallows using aclassdirectly forT, you can still use astructthat contains aclassas one of its fields.So, as a result,
ValueArray<T>can indirectly contain a GC pointer ifTdoes, because it is constrained tostructand notunmanaged. Either way though, the GC issues still apply.To be clear here: there are two separate issues at play.
On .NET Framework, Span uses a “slower” implementation that needs to keep track of an
objectto keep the object live. This is not the case on modern runtimes where the GC allows it to track arefdirectly. This is why creating the span like this can cause the object to be prematurely GC’d as the above code shows, and also why there’s no safe way to do this in NS2.0.The usage of
Unsafe.AsPointerhowever is still a problem on modern runtimes. If the GC runs the split moment after the call toAsPointerbut before the span is constructed, the GC could move the backing object without updating the unmanaged pointer (depending on how aggressive the JIT is with tracking locals this might also result in “object freed prematurely” though).Also if you use this value array in a local, you can return the span up the stack and the language won’t tell you, allowing you to access garbage memory.