[nt-0] fix: Protect against null pointers & disposed objects in C# wrapper (#786)

* [nt-0] fix: Protect against null pointers

Attempt to prevent the class of native level crash caused by accessing invalid pointers.

* `Dispose()` sets the internal pointer to zero
* Changes `_ptr` to a property
* Accessing _ptr when it's `IntPtr.Zero` throws a NullReferenceException
* Prevent modification of the underlying pointer for `NativeClassWrapper`

* [nt-0] docs: xmldocs for NativeClassWrapper

Author: magnopus-swifty

Library/CSharpWrapper/src/NativeClassWrapper.cs

@@ -37,18 +37,92 @@ internal NativePointer(IntPtr pointer, byte ownsOwnData)
         public static NativePointer Zero => new NativePointer(IntPtr.Zero, 0);
     }
 
+    /// <summary>
+    /// Base class for all CSP objects.
+    /// Provides base memory management for derived interop types.
+    /// </summary>
     public class NativeClassWrapper
     {
-        internal IntPtr _ptr;
+        /// <summary>
+        /// The native C pointer to the object for use with P/Invoke methods.
+        /// </summary>
+        /// <remarks>
+        /// This must be accessed through <see cref="_ptr"/> to avoid passing `nullptr` to
+        /// member functions in native code.
+        /// </remarks>
+        private IntPtr _ptrValue = IntPtr.Zero;
+
+        /// <summary>
+        /// Cached value of <see cref="NativePointer.OwnsOwnData"/>.
+        /// If true, this object owns the underlying pointer and is responsible for
+        /// calling its destructor when disposed.
+        /// </summary>
         internal bool _ownsPtr;
+
+        /// <summary>
+        /// Indicates whether the object has been disposed.
+        /// Used to implement the <see cref="System.IDisposable"/> pattern in subclasses.
+        /// </summary>
         internal bool _disposed = false;
 
+        /// <summary>
+        /// Gets the name of the name-mangled native type pointed to by <see cref="_ptrValue"/>.
+        /// </summary>
         internal virtual string _safeTypeName { get; }
 
-        public bool PointerIsValid => _ptr != IntPtr.Zero;
+        /// <summary>
+        /// Is the underlying pointer (<see cref="_ptr"/>) valid.
+        /// </summary>
+        /// <remarks>
+        /// This can be used to prevent a <see cref="NullReferenceException"/> when calling <see cref="_ptr"/>.
+        /// </remarks>
+        public bool PointerIsValid => _ptrValue != IntPtr.Zero;
+
+        /// <summary>
+        /// Pointer to the native object.
+        /// </summary>
+        /// <exception cref="NullReferenceException">Thrown if the pointer is null.</exception>
+        /// <exception cref="InvalidOperationException">Thrown if attempting to change the pointer once it's set to a non-null value.</exception>
+        internal IntPtr _ptr
+        {
+            get
+            {
+                // Prevent accessing the pointer if it's null
+                if (_ptrValue == IntPtr.Zero)
+                {
+                    throw new NullReferenceException($"Attempting to access a null pointer for {_safeTypeName}");
+                }
+                return _ptrValue;
+            }
+            set
+            {
+                // Prevent changing the pointer once it's set to a non-null value
+                if (_ptrValue != IntPtr.Zero && value != IntPtr.Zero)
+                {
+                    throw new InvalidOperationException($"Attempting to change the native pointer for {_safeTypeName} from {_ptrValue} to {value}");
+                }
+
+                _ptrValue = value;
+            }
+        }
 
+        /// <summary>
+        /// Construct an empty instance of the object.
+        /// </summary>
+        /// <remarks>
+        /// This is generally an invalid operation since it creates an object where
+        /// <see cref="_ptr"/> is guaranteed to throw an exception.
+        /// This empty constructor is currently required by generated generic types
+        /// (such as <see cref="Csp.Common.List<T>"/>) which look up the underlying
+        /// base type stored in <see cref="_safeTypeName"/>.
+        /// </remarks>
         public NativeClassWrapper() { }
 
+        /// <summary>
+        /// Construct an instance of the object given a <see cref="NativePointer"/>
+        /// from the native runtime.
+        /// </summary>
+        /// <param name="ptr">A valid <see cref="NativePointer"/> representing the runtime object.</param>
         internal NativeClassWrapper(NativePointer ptr)
         {
             _ptr = ptr.Pointer;

Tools/WrapperGenerator/Templates/CSharp/Partials/Class/Method/Destructor.mustache

@@ -9,9 +9,9 @@ void RealDispose() {
     if (_ownsPtr && !_disposed)
     {
         {{ unique_name }}(_ptr);
-        _disposed = true;
     }
 
+    _ptr = IntPtr.Zero;
     _disposed = true;
 }
 

Tools/WrapperGenerator/Templates/CSharp/Partials/Template/Method/Destructor.mustache

@@ -8,9 +8,9 @@ void RealDispose() {
     if (_ownsPtr && !_disposed)
     {
         {{ unique_name }}(_ptr);
-        _disposed = true;
     }
 
+    _ptr = IntPtr.Zero;
     _disposed = true;
 }
 

UnitTesting/include/classes.h

@@ -24,5 +24,7 @@ class CSP_API SimpleClass
 public:
     SimpleClass();
     ~SimpleClass();
+
+    int GetValue() const;
 };
 }

UnitTesting/src/classes.cpp

@@ -21,4 +21,7 @@ namespace csp::Tests
 SimpleClass::SimpleClass() { }
 
 SimpleClass::~SimpleClass() { }
-}
+
+int SimpleClass::GetValue() const { return 42; }
+
+} // namespace csp::Tests

UnitTesting/tests/csharp/Assets/Tests/ClassTests.cs

@@ -15,6 +15,7 @@
  */
 
 using NUnit.Framework;
+using System;
 
 namespace Csp.Tests
 {
@@ -29,13 +30,13 @@ public void CreateAndDestroySimpleClass()
 
             // Verify the internal pointer is set.
             // Note that we have to use reflection here to access the internal field.
-            var ptrField = typeof(SimpleClass).GetField("_ptr", System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance);
-            var ptrValue = ptrField.GetValue(simpleClass);
+            var ptrField = typeof(NativeClassWrapper).GetField("_ptrValue", System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance);
+            IntPtr ptrValue = (IntPtr)ptrField.GetValue(simpleClass);
             Assert.IsNotNull(ptrValue);
 
             // Verify that the instance owns the native pointer.
             // Note that we have to use reflection here to access the internal field.
-            var ownsPtrField = typeof(SimpleClass).GetField("_ownsPtr", System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance);
+            var ownsPtrField = typeof(NativeClassWrapper).GetField("_ownsPtr", System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance);
             var ownsPtrValue = ownsPtrField.GetValue(simpleClass);
             Assert.IsTrue((bool)ownsPtrValue);
 
@@ -44,14 +45,19 @@ public void CreateAndDestroySimpleClass()
             simpleClass.Dispose();
 
             // Once Dispose is called, the internal pointer is no longer valid.
-            // However, Dispose does not null out the pointer so the class is
-            // left in an indeterminate state.
-            // There is also no way to query whether Dispose has been called.
-            Assert.IsTrue(simpleClass.PointerIsValid);
+            Assert.IsFalse(simpleClass.PointerIsValid);
 
-            // Verify the internal pointer is still set.
-            var ptrValueDisposed = ptrField.GetValue(simpleClass);
-            Assert.IsNotNull(ptrValueDisposed);
+            // Verify the internal pointer has been nulled.
+            IntPtr ptrValueDisposed = (IntPtr)ptrField.GetValue(simpleClass);
+            Assert.AreEqual(IntPtr.Zero, ptrValueDisposed);
+        }
+
+        [Test]
+        public void GetValueReturnsCorrectValue()
+        {
+            var simpleClass = new SimpleClass();
+            int value = simpleClass.GetValue();
+            Assert.AreEqual(42, value);
         }
 
         [Test]
@@ -63,5 +69,17 @@ public void MultipleDisposes()
             simpleClass.Dispose();
             simpleClass.Dispose();
         }
+
+        [Test]
+        public void AccessAfterDisposeThrows()
+        {
+            var simpleClass = new SimpleClass();
+            simpleClass.Dispose();
+
+            // Accessing the pointer after Dispose should throw a NullReferenceException rather
+            // than creating a segfault.
+            var ex = Assert.Throws<NullReferenceException>(() => { int value = simpleClass.GetValue(); });
+            StringAssert.Contains("Attempting to access a null pointer", ex.Message);
+        }
     }
 }