Support pinning of IPythonBuffer (#1411)

* Support pinning of IPythonBuffer

* Update from_buffer to accept an IBufferProtocol

* Align error messages with CPython

* Update after review

Author: BCSharp

Src/IronPython.Modules/_ctypes/ArrayType.cs

@@ -100,26 +100,15 @@ public _Array from_address(CodeContext/*!*/ context, BigInteger ptr) {
                 return res;
             }
 
-            public _Array from_buffer(CodeContext/*!*/ context, ArrayModule.array array, [DefaultParameterValue(0)] int offset) {
-                ValidateArraySizes(array, offset, ((INativeType)this).Size);
-
+            public _Array from_buffer(CodeContext/*!*/ context, object/*?*/ data, int offset = 0) {
                 _Array res = (_Array)CreateInstance(context);
-                IntPtr addr = array.GetArrayAddress();
-                res.MemHolder = new MemoryHolder(addr.Add(offset), ((INativeType)this).Size);
-                res.MemHolder.AddObject("ffffffff", array);
+                res.InitializeFromBuffer(data, offset, ((INativeType)this).Size);
                 return res;
             }
 
-            public _Array from_buffer_copy(CodeContext/*!*/ context, [NotNull] IBufferProtocol data, int offset = 0) {
-                using var buffer = data.GetBuffer();
-                var span = buffer.AsReadOnlySpan();
-                var size = ((INativeType)this).Size;
-                ValidateArraySizes(span.Length, offset, size);
-                span = span.Slice(offset, size);
-
+            public _Array from_buffer_copy(CodeContext/*!*/ context, object/*?*/ data, int offset = 0) {
                 _Array res = (_Array)CreateInstance(context);
-                res.MemHolder = new MemoryHolder(size);
-                res.MemHolder.WriteSpan(0, span);
+                res.InitializeFromBufferCopy(data, offset, ((INativeType)this).Size);
                 return res;
             }
 

Src/IronPython.Modules/_ctypes/CData.cs

@@ -7,10 +7,13 @@
 #if FEATURE_CTYPES
 
 using System;
+using System.Buffers;
 using System.Collections.Generic;
 using System.Diagnostics;
 
 using IronPython.Runtime;
+using IronPython.Runtime.Exceptions;
+using IronPython.Runtime.Operations;
 using IronPython.Runtime.Types;
 
 namespace IronPython.Modules {
@@ -38,7 +41,10 @@ public static partial class CTypes {
         public abstract class CData : IBufferProtocol, IPythonBuffer {
             internal MemoryHolder MemHolder {
                 get => _memHolder ?? throw new InvalidOperationException($"{nameof(CData)} object not fully initialized.");
-                set => _memHolder = value;
+                set {
+                    _memHolder?.Dispose();
+                    _memHolder = value;
+                }
             }
             private MemoryHolder? _memHolder;
 
@@ -55,13 +61,50 @@ protected CData() { }
 
             internal INativeType NativeType => (INativeType)DynamicHelpers.GetPythonType(this);
 
-            public virtual object _objects => MemHolder.Objects;
+            public virtual object? _objects => MemHolder.Objects;
 
             internal void SetAddress(IntPtr address) {
-                Debug.Assert(_memHolder == null);
+                // TODO: Debug.Assert(_memHolder == null); // fails for structures
                 MemHolder = new MemoryHolder(address, NativeType.Size);
             }
 
+            internal void InitializeFromBuffer(object? data, int offset, int size) {
+                var bp = data as IBufferProtocol
+                    ?? throw PythonOps.TypeErrorForBadInstance("{0} object does not have the buffer interface", data);
+                    // Python 3.5: PythonOps.TypeErrorForBytesLikeTypeMismatch(data);
+
+                IPythonBuffer buffer = bp.GetBuffer(BufferFlags.FullRO);
+                if (buffer.IsReadOnly) {
+                    buffer.Dispose();
+                    throw PythonOps.TypeError("underlying buffer is not writable");
+                }
+                if (!buffer.IsCContiguous()) {
+                    buffer.Dispose();
+                    throw PythonOps.TypeError("underlying buffer is not C contiguous");
+                }
+
+                try {
+                    ValidateArraySizes(buffer.NumBytes(), offset, size);
+                    MemHolder = new MemoryHolder(buffer, offset, size);
+                } catch {
+                    buffer.Dispose();
+                    throw;
+                }
+                MemHolder.AddObject("ffffffff", buffer.Object);
+            }
+
+            internal void InitializeFromBufferCopy(object? data, int offset, int size) {
+                var bp = data as IBufferProtocol
+                    ?? throw PythonOps.TypeErrorForBadInstance("{0} object does not have the buffer interface", data);
+                    // Python 3.5: PythonOps.TypeErrorForBytesLikeTypeMismatch(data);
+
+                using IPythonBuffer buffer = bp.GetBuffer();
+                var span = buffer.AsReadOnlySpan();
+                ValidateArraySizes(span.Length, offset, size);
+                MemHolder = new MemoryHolder(size);
+                MemHolder.WriteSpan(0, span.Slice(offset, size));
+            }
+
             internal virtual PythonTuple GetBufferInfo()
                 => PythonTuple.MakeTuple(NativeType.TypeFormat, 0, PythonTuple.EMPTY);
 
@@ -70,7 +113,7 @@ internal virtual PythonTuple GetBufferInfo()
 
             IPythonBuffer IBufferProtocol.GetBuffer(BufferFlags flags) => this;
 
-            void IDisposable.Dispose() { }
+            void IDisposable.Dispose() { } // TODO
 
             object IPythonBuffer.Object => this;
 
@@ -80,6 +123,9 @@ unsafe ReadOnlySpan<byte> IPythonBuffer.AsReadOnlySpan()
             unsafe Span<byte> IPythonBuffer.AsSpan()
                 => new Span<byte>(MemHolder.UnsafeAddress.ToPointer(), MemHolder.Size);
 
+            unsafe MemoryHandle IPythonBuffer.Pin()
+                => new MemoryHandle(MemHolder.UnsafeAddress.ToPointer());
+
             int IPythonBuffer.Offset => 0;
 
             [PythonHidden]

Src/IronPython.Modules/_ctypes/MemoryHolder.cs

@@ -2,10 +2,14 @@
 // The .NET Foundation licenses this file to you under the Apache 2.0 License.
 // See the LICENSE file in the project root for more information.
 
+#nullable enable
+
 #if FEATURE_CTYPES
 #pragma warning disable SYSLIB0004 // The Constrained Execution Region (CER) feature is not supported in .NET 5.0.
 
 using System;
+using System.Buffers;
+using System.Diagnostics;
 using System.IO;
 using System.Runtime.CompilerServices;
 using System.Runtime.ConstrainedExecution;
@@ -19,17 +23,22 @@ namespace IronPython.Modules {
     /// A wrapper around allocated memory to ensure it gets released and isn't accessed
     /// when it could be finalized.
     /// </summary>
-    internal sealed class MemoryHolder : CriticalFinalizerObject {
+    internal sealed class MemoryHolder : CriticalFinalizerObject, IDisposable {
         private readonly IntPtr _data;
         private readonly bool _ownsData;
         private readonly int _size;
-        private PythonDictionary _objects;
-#pragma warning disable 414 // TODO: unused field?
-        private readonly MemoryHolder _parent;
-#pragma warning restore 414
+        private readonly IPythonBuffer? _buffer;
+        private readonly MemoryHandle _handle;
+        private bool _disposed;
+        private PythonDictionary? _objects;
+#pragma warning disable IDE0052 // Remove unread private members
+        // Field not accessed but keeps a reference to the parent holder preventing it from being garbage-collected.
+        private readonly MemoryHolder? _parent;
+#pragma warning restore IDE0052 // Remove unread private members
 
         /// <summary>
         /// Creates a new MemoryHolder and allocates a buffer of the specified size.
+        /// The buffer will be released on a call to <see cref="Dispose"/> or by the finalizer.
         /// </summary>
         [ReliabilityContract(Consistency.WillNotCorruptState, Cer.MayFail)]
         public MemoryHolder(int size) {
@@ -49,6 +58,7 @@ public MemoryHolder(int size) {
         /// <summary>
         /// Creates a new MemoryHolder at the specified address which is not tracked
         /// by us and we will never free.
+        /// Therefore no need to call <see cref="Dispose"/>.
         /// </summary>
         public MemoryHolder(IntPtr data, int size) {
             GC.SuppressFinalize(this);
@@ -59,6 +69,7 @@ public MemoryHolder(IntPtr data, int size) {
         /// <summary>
         /// Creates a new MemoryHolder at the specified address which will keep alive the 
         /// parent memory holder.
+        /// TODO: Dispose() usage? Reference-count the parent?
         /// </summary>
         public MemoryHolder(IntPtr data, int size, MemoryHolder parent) {
             GC.SuppressFinalize(this);
@@ -69,39 +80,43 @@ public MemoryHolder(IntPtr data, int size, MemoryHolder parent) {
         }
 
         /// <summary>
-        /// Gets the address of the held memory.  The caller should ensure the MemoryHolder
-        /// is always alive as long as the address will continue to be accessed.
+        /// Creates a new MemoryHolder from a Python buffer
+        /// that will be disposed on a call to <see cref="Dispose"/> or by the finalizer.
         /// </summary>
-        public IntPtr UnsafeAddress {
-            get {
-                return _data;
-            }
+        public MemoryHolder(IPythonBuffer buffer, int offset, int size) {
+            if (buffer.IsReadOnly) throw new ArgumentException("Buffer must be writable.");
+            if (!buffer.IsCContiguous()) throw new ArgumentException("Buffer must be c-contiguous.");
+            if (size < 0) throw new ArgumentOutOfRangeException(nameof(size), size, $"Non-negative number required.");
+            if (offset < 0) throw new ArgumentOutOfRangeException(nameof(offset), offset, $"Non-negative number required.");
+            if (size > buffer.NumBytes() - offset) throw new ArgumentException($"Requested memory block exceeds buffer boundaries.");
+
+            _buffer = buffer;
+            _handle = buffer.Pin();
+            unsafe { _data = (IntPtr)_handle.Pointer + offset; }
+            _size = size;
         }
 
-        public int Size {
-            get {
-                return _size;
-            }
-        }
+        /// <summary>
+        /// Gets the address of the held memory.  The caller should ensure the MemoryHolder
+        /// is always alive and not disposed as long as the address will continue to be accessed.
+        /// </summary>
+        public IntPtr UnsafeAddress => !_disposed ? _data : IntPtr.Zero;
+
+        public int Size => _size;
 
         /// <summary>
         /// Gets a list of objects which need to be kept alive for this MemoryHolder to be 
         /// remain valid.
         /// </summary>
-        public PythonDictionary Objects {
-            get {
-                return _objects;
-            }
-            set {
-                _objects = value;
-            }
+        public PythonDictionary? Objects {
+            get => _objects;
+            set => _objects = value;
         }
 
         internal PythonDictionary EnsureObjects() {
-            if (_objects == null) {
+            if (_objects is null) {
                 Interlocked.CompareExchange(ref _objects, new PythonDictionary(), null);
             }
-
             return _objects;
         }
 
@@ -178,15 +193,15 @@ public MemoryHolder ReadMemoryHolder(int offset) {
             return new MemoryHolder(res, IntPtr.Size, this);
         }
 
-        internal Bytes ReadBytes(int offset) {
+        internal Bytes? ReadBytes(int offset) {
             try {
                 return ReadBytes(_data, offset);
             } finally {
                 GC.KeepAlive(this);
             }
         }
 
-        internal string ReadUnicodeString(int offset) {
+        internal string? ReadUnicodeString(int offset) {
             try {
                 return Marshal.PtrToStringUni(_data.Add(offset));
             } finally {
@@ -214,8 +229,8 @@ internal static Bytes ReadBytes(IntPtr addr, int offset, int length) {
             return Bytes.Make(buffer);
         }
 
-        internal static Bytes ReadBytes(IntPtr addr, int offset) {
-            if (addr == IntPtr.Zero && offset == 0) return null;
+        internal static Bytes? ReadBytes(IntPtr addr, int offset) {
+            if (addr == IntPtr.Zero) return null;
 
             // instead of Marshal.PtrToStringAnsi we do this because
             // ptrToStringAnsi gives special treatment to values >= 128.
@@ -305,10 +320,30 @@ public void CopyTo(MemoryHolder/*!*/ destAddress, int writeOffset, int size) {
             GC.KeepAlive(this);
         }
 
+        public void Dispose() {
+            if (!_disposed) {
+                _disposed = true;
+
+                if (_ownsData) {
+                    Marshal.FreeHGlobal(_data);
+                }
+                _handle.Dispose();
+                _buffer?.Dispose();
+
+                GC.SuppressFinalize(this);
+            }
+        }
+
         [ReliabilityContract(Consistency.WillNotCorruptState, Cer.Success)]
         ~MemoryHolder() {
-            if (_ownsData) {
-                Marshal.FreeHGlobal(_data);
+            if (!_disposed) {
+                _disposed = true;
+
+                if (_ownsData) {
+                    Marshal.FreeHGlobal(_data);
+                }
+                try { _handle.Dispose(); } catch { /*ignore*/ }
+                try { _buffer?.Dispose(); } catch { /*ignore*/ }
             }
         }
     }

Src/IronPython.Modules/_ctypes/SimpleType.cs

@@ -13,6 +13,7 @@
 using System.Runtime.InteropServices;
 
 using IronPython.Runtime;
+using IronPython.Runtime.Exceptions;
 using IronPython.Runtime.Operations;
 using IronPython.Runtime.Types;
 
@@ -131,26 +132,15 @@ public SimpleCData from_address(CodeContext/*!*/ context, IntPtr ptr) {
                 return res;
             }
 
-            public SimpleCData from_buffer(ArrayModule.array array, int offset = 0) {
-                ValidateArraySizes(array, offset, ((INativeType)this).Size);
-
-                SimpleCData res = (SimpleCData)CreateInstance(Context.SharedContext);
-                IntPtr addr = array.GetArrayAddress();
-                res.MemHolder = new MemoryHolder(addr.Add(offset), ((INativeType)this).Size);
-                res.MemHolder.AddObject("ffffffff", array);
+            public SimpleCData from_buffer(CodeContext/*!*/ context, object/*?*/ data, int offset = 0) {
+                SimpleCData res = (SimpleCData)CreateInstance(context);
+                res.InitializeFromBuffer(data, offset, ((INativeType)this).Size);
                 return res;
             }
 
-            public SimpleCData from_buffer_copy(CodeContext/*!*/ context, [NotNull] IBufferProtocol data, int offset = 0) {
-                using var buffer = data.GetBuffer();
-                var span = buffer.AsReadOnlySpan();
-                var size = ((INativeType)this).Size;
-                ValidateArraySizes(span.Length, offset, size);
-                span = span.Slice(offset, size);
-
+            public SimpleCData from_buffer_copy(CodeContext/*!*/ context, object/*?*/ data, int offset = 0) {
                 SimpleCData res = (SimpleCData)CreateInstance(context);
-                res.MemHolder = new MemoryHolder(size);
-                res.MemHolder.WriteSpan(0, span);
+                res.InitializeFromBufferCopy(data, offset, ((INativeType)this).Size);
                 return res;
             }