Runtime: Add MemoryHandle.HasPointer

Created on 12 Sep 2017  路  12Comments  路  Source: dotnet/runtime

@stephentoub ran into cases where he needed to check if a handle has a pinned pointer. Today, it can be done using unsafe code (get the PinnedPinter and check if it's null). This new API would allow the same check in safe code.

api-approved area-System.Memory

Most helpful comment

We have standardized on using unmanaged pointers (e.g. void*) for all new public methods that involve pointers.

All 12 comments

Another option would be to make PinnedPointer an IntPtr instead of a void*.

Another option would be to make PinnedPointer an IntPtr instead of a void*.

What would be the consequences (if any) of doing that?

@KrzysztofCwalina, what was the motivation, initially, to have PinnedPointer return void*? In most cases, we end up casting to IntPtr anyway:
https://github.com/dotnet/corefxlab/search?utf8=%E2%9C%93&q=PinnedPointer&type=

Also, when we are constructing MemoryHandle, we usually have an IntPtr from GCHandle.AddrOfPinnedObject that we cast to void* (or call ToPointer on):
https://github.com/dotnet/corefxlab/search?utf8=%E2%9C%93&q=MemoryHandle&type=

From [Pipelines]:(https://github.com/dotnet/corefxlab/blob/master/src/System.IO.Pipelines/System/Buffers/MemoryPoolBlock.cs#L140)
C# return new MemoryHandle(this, (Slab.NativePointer + _offset).ToPointer());

We have standardized on using unmanaged pointers (e.g. void*) for all new public methods that involve pointers.

We have standardized on using unmanaged pointers (e.g. void*) for all new public methods that involve pointers.

In that case, I believe the API is ready for review.
```C#
namespace System.Buffers
{
public unsafe struct MemoryHandle : IDisposable
{
public bool HasPointer { get { throw null; } }
}
}

Implementation like:
```C#
public bool HasPointer => _pointer != null;

Is there an example of code where this would be used? I would naively assume that once you find that there is a pointer, you will want to fetch the pointer that requires unsafe code.

e.g. https://github.com/dotnet/corefx/blob/master/src/System.IO.Compression/src/System/IO/Compression/DeflateZLib/Deflater.cs#L179

Again, it's minor... one can use unsafe and compare to null. It's just a nicety.

IMO property name should better communicate it's information content i.e. IsNonNullPointer or IsNullPointer. HasPointer may equally well mean (semantically) that MemoryHandle has pointer which could be null. Assumption that it is not null is implicit and not directly communicated by property name. Another issue is whether _non null pointer_ is a _valid pointer_. Non null pointers may point to non user memory range (system memory) which would lead to unexpected exceptions when dereferenced or written to unless user of the property is informed about it's implicit contract explicitly by documentation what should prevent inappropriate usage or implicit contract is implemented in property.

This new API would allow the same check in safe code.

As long as check does not verify if pointer points to valid (accessible) memory range it would merely provide false sense of security due to omission of unsafe keyword while at the same time not providing any guarantees about validity of the pointer.

Video

The member on MemoryHandle is called PinnedPointer. We should align the name here, so HasPinnedPointer. It seems @KrzysztofCwalina is unsure whether the existing property is named correctly. He'll chase this up offline.

FYI: The API review discussion was recorded - see https://youtu.be/m4BUM3nJZRw?t=6200 (6 min duration)

We can rename the property on handle to Pointer. I filed an issue for this.

I attempted to run a test on another issue against master and ran into a perhaps unanticipated consequence to this change. Please see: https://github.com/dotnet/corefx/issues/24142

Was this page helpful?
0 / 5 - 0 ratings

Related issues

jzabroski picture jzabroski  路  3Comments

Timovzl picture Timovzl  路  3Comments

jchannon picture jchannon  路  3Comments

GitAntoinee picture GitAntoinee  路  3Comments

jamesqo picture jamesqo  路  3Comments