Fix: del obj[key] on a reflected indexer crashed the process (#149)

* Fix: del obj[key] on a reflected indexer crashed the process

CPython calls mp_ass_subscript with a null value for del, which mp_ass_subscript_impl forwarded into PyTuple_SetItem and threw across the native boundary. Handle deletion first: IDictionary<K,V>.Remove / IList<T>.RemoveAt through the binder (KeyError on a missing dictionary key), TypeError for every other type and for arrays.

* Resolve the item deleter lazily on the first del

Most types are never deleted from, so the interface walk that finds IDictionary<K,V>.Remove or IList<T>.RemoveAt now runs on the first del instead of at class creation.

* Bump version to 2.0.66

Author: jhonabreul

src/embed_tests/TestIndexerDelete.cs

@@ -0,0 +1,101 @@
+using System.Collections.Concurrent;
+using System.Collections.Generic;
+
+using NUnit.Framework;
+
+using Python.Runtime;
+
+namespace Python.EmbeddingTest
+{
+    /// <summary>
+    /// `del ob[key]` reaches mp_ass_subscript with a null value. It must raise a catchable Python
+    /// exception (or delete, for IDictionary/IList types) instead of aborting the process.
+    /// </summary>
+    [TestFixture]
+    public class TestIndexerDelete
+    {
+        [OneTimeSetUp]
+        public void SetUp()
+        {
+            PythonEngine.Initialize();
+        }
+
+        [OneTimeTearDown]
+        public void Dispose()
+        {
+            PythonEngine.Shutdown();
+        }
+
+        public class SettableIndexer
+        {
+            private readonly Dictionary<int, string> _items = new();
+
+            public string this[int key]
+            {
+                get => _items[key];
+                set => _items[key] = value;
+            }
+
+            public string Marker => "alive";
+        }
+
+        [Test]
+        public void DelOnSettableIndexerRaisesTypeError()
+        {
+            using (Py.GIL())
+            {
+                using var scope = Py.CreateScope();
+                scope.Set("ob", new SettableIndexer().ToPython());
+                scope.Exec(@"
+ob[1] = 'one'
+raised = None
+try:
+    del ob[1]
+except TypeError as e:
+    raised = e
+");
+                using var raised = scope.Get("raised");
+                Assert.IsFalse(raised.IsNone(), "del must raise TypeError");
+                Assert.AreEqual("alive", scope.Eval("ob.Marker").As<string>());
+                Assert.AreEqual("one", scope.Eval("ob[1]").As<string>());
+            }
+        }
+
+        [Test]
+        public void DelOnConcurrentDictionaryRemovesKey()
+        {
+            using (Py.GIL())
+            {
+                using var scope = Py.CreateScope();
+                var dict = new ConcurrentDictionary<string, string>();
+                dict["MyKey"] = "MyValue";
+                scope.Set("d", dict.ToPython());
+
+                scope.Exec("del d['MyKey']");
+
+                Assert.IsFalse(dict.ContainsKey("MyKey"));
+                Assert.AreEqual(0, scope.Eval("d.Count").As<int>());
+            }
+        }
+
+        [Test]
+        public void DelOnDictionaryMissingKeyRaisesKeyError()
+        {
+            using (Py.GIL())
+            {
+                using var scope = Py.CreateScope();
+                scope.Set("d", new Dictionary<string, int> { ["a"] = 1 }.ToPython());
+                scope.Exec(@"
+raised = None
+try:
+    del d['missing']
+except KeyError as e:
+    raised = e
+");
+                using var raised = scope.Get("raised");
+                Assert.IsFalse(raised.IsNone(), "del of a missing key must raise KeyError");
+                Assert.AreEqual(1, scope.Eval("d.Count").As<int>());
+            }
+        }
+    }
+}

src/perf_tests/Python.PerformanceTests.csproj

@@ -13,7 +13,7 @@
       <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
     </PackageReference>
     <PackageReference Include="Microsoft.NET.Test.Sdk" Version="16.*" />
-    <PackageReference Include="quantconnect.pythonnet" Version="2.0.65" GeneratePathProperty="true">
+    <PackageReference Include="quantconnect.pythonnet" Version="2.0.66" GeneratePathProperty="true">
       <IncludeAssets>compile</IncludeAssets>
     </PackageReference>
   </ItemGroup>
@@ -25,7 +25,7 @@
   </Target>
 
   <Target Name="CopyBaseline" AfterTargets="Build">
-    <Copy SourceFiles="$(NuGetPackageRoot)quantconnect.pythonnet\2.0.65\lib\net10.0\Python.Runtime.dll" DestinationFolder="$(OutDir)baseline" />
+    <Copy SourceFiles="$(NuGetPackageRoot)quantconnect.pythonnet\2.0.66\lib\net10.0\Python.Runtime.dll" DestinationFolder="$(OutDir)baseline" />
   </Target>
 
   <Target Name="CopyNewBuild" AfterTargets="Build">

src/runtime/Properties/AssemblyInfo.cs

@@ -4,5 +4,5 @@
 [assembly: InternalsVisibleTo("Python.EmbeddingTest, PublicKey=00240000048000009400000006020000002400005253413100040000110000005ffd8f49fb44ab0641b3fd8d55e749f716e6dd901032295db641eb98ee46063cbe0d4a1d121ef0bc2af95f8a7438d7a80a3531316e6b75c2dae92fb05a99f03bf7e0c03980e1c3cfb74ba690aca2f3339ef329313bcc5dccced125a4ffdc4531dcef914602cd5878dc5fbb4d4c73ddfbc133f840231343e013762884d6143189")]
 [assembly: InternalsVisibleTo("Python.Test, PublicKey=00240000048000009400000006020000002400005253413100040000110000005ffd8f49fb44ab0641b3fd8d55e749f716e6dd901032295db641eb98ee46063cbe0d4a1d121ef0bc2af95f8a7438d7a80a3531316e6b75c2dae92fb05a99f03bf7e0c03980e1c3cfb74ba690aca2f3339ef329313bcc5dccced125a4ffdc4531dcef914602cd5878dc5fbb4d4c73ddfbc133f840231343e013762884d6143189")]
 
-[assembly: AssemblyVersion("2.0.65")]
-[assembly: AssemblyFileVersion("2.0.65")]
+[assembly: AssemblyVersion("2.0.66")]
+[assembly: AssemblyFileVersion("2.0.66")]

src/runtime/Python.Runtime.csproj

@@ -5,7 +5,7 @@
     <RootNamespace>Python.Runtime</RootNamespace>
     <AssemblyName>Python.Runtime</AssemblyName>
     <PackageId>QuantConnect.pythonnet</PackageId>
-    <Version>2.0.65</Version>
+    <Version>2.0.66</Version>
     <GenerateAssemblyInfo>false</GenerateAssemblyInfo>
     <PackageLicenseFile>LICENSE</PackageLicenseFile>
     <RepositoryUrl>https://github.com/pythonnet/pythonnet</RepositoryUrl>

src/runtime/Types/ArrayObject.cs

@@ -245,6 +245,13 @@ public static NewReference mp_subscript(BorrowedReference ob, BorrowedReference
         /// </summary>
         public static int mp_ass_subscript(BorrowedReference ob, BorrowedReference idx, BorrowedReference v)
         {
+            // `del arr[i]` arrives here with a null value; arrays are fixed-size, so refuse it up front.
+            if (v.IsNull)
+            {
+                Exceptions.RaiseTypeError("array does not support item deletion");
+                return -1;
+            }
+
             var obj = (CLRObject)GetManagedObject(ob)!;
             var items = (Array)obj.inst;
             Type itemType = obj.inst.GetType().GetElementType();

src/runtime/Types/ClassBase.cs

@@ -507,6 +507,13 @@ static int mp_ass_subscript_impl(BorrowedReference ob, BorrowedReference idx, Bo
             BorrowedReference tp = Runtime.PyObject_TYPE(ob);
             var cls = (ClassBase)GetManagedObject(tp)!;
 
+            // CPython routes `del ob[key]` through this same slot with a null value. None of the
+            // assignment code below can take a null, so deletion must be handled before anything else.
+            if (v.IsNull)
+            {
+                return DeleteItemImpl(cls, ob, idx);
+            }
+
             if (cls.indexer == null || !cls.indexer.CanSet)
             {
                 Exceptions.SetError(Exceptions.TypeError, "object doesn't support item assignment");
@@ -560,6 +567,44 @@ static int mp_ass_subscript_impl(BorrowedReference ob, BorrowedReference idx, Bo
             return 0;
         }
 
+        /// <summary>
+        /// Implements __delitem__ (del ob[key]) for reflected classes: IDictionary&lt;K,V&gt;.Remove or
+        /// IList&lt;T&gt;.RemoveAt through the binder, TypeError for everything else.
+        /// </summary>
+        static int DeleteItemImpl(ClassBase cls, BorrowedReference ob, BorrowedReference idx)
+        {
+            if (cls.indexer == null || !cls.type.Valid || !cls.indexer.CanDelete(cls.type.Value))
+            {
+                Exceptions.SetError(Exceptions.TypeError, "object doesn't support item deletion");
+                return -1;
+            }
+
+            if (Runtime.PyTuple_Check(idx))
+            {
+                Exceptions.SetError(Exceptions.TypeError, "object doesn't support multi-index item deletion");
+                return -1;
+            }
+
+            using var args = Runtime.PyTuple_New(1);
+            Runtime.PyTuple_SetItem(args.Borrow(), 0, idx);
+
+            // The binder converts the key and turns a managed exception into a Python error.
+            using var result = cls.indexer.DeleteItem(ob, args.Borrow());
+            if (result.IsNull() || Exceptions.ErrorOccurred())
+            {
+                return -1;
+            }
+
+            // IDictionary<K,V>.Remove reports a missing key by returning false; match dict semantics.
+            if (result.Borrow() == Runtime.PyFalse)
+            {
+                Exceptions.SetError(Exceptions.KeyError, idx);
+                return -1;
+            }
+
+            return 0;
+        }
+
         static NewReference tp_call_impl(BorrowedReference ob, BorrowedReference args, BorrowedReference kw)
         {
             BorrowedReference tp = Runtime.PyObject_TYPE(ob);

src/runtime/Types/Indexer.cs

@@ -1,4 +1,6 @@
 using System;
+using System.Collections.Generic;
+using System.Linq;
 using System.Reflection;
 
 namespace Python.Runtime
@@ -11,11 +13,13 @@ internal class Indexer
     {
         public MethodBinder GetterBinder;
         public MethodBinder SetterBinder;
+        public MethodBinder DeleterBinder;
 
         public Indexer()
         {
             GetterBinder = new MethodBinder();
             SetterBinder = new MethodBinder();
+            DeleterBinder = new MethodBinder();
         }
 
 
@@ -29,6 +33,24 @@ public bool CanSet
             get { return SetterBinder.Count > 0; }
         }
 
+        // The deleter is resolved on the first `del`: most types are never deleted from, so the
+        // interface walk only runs for the ones that are. Called under the GIL, like the slot itself.
+        [NonSerialized] private bool _deleterResolved;
+
+        public bool CanDelete(Type type)
+        {
+            if (!_deleterResolved)
+            {
+                _deleterResolved = true;
+                DeleterBinder ??= new MethodBinder();
+                if (DeleterBinder.Count == 0)
+                {
+                    ResolveDeleter(type);
+                }
+            }
+            return DeleterBinder.Count > 0;
+        }
+
 
         public void AddProperty(PropertyInfo pi)
         {
@@ -55,6 +77,54 @@ internal void SetItem(BorrowedReference inst, BorrowedReference args)
             SetterBinder.Invoke(inst, args, null);
         }
 
+        /// <summary>
+        /// Resolves the method behind <c>del ob[key]</c>: IDictionary&lt;K,V&gt;.Remove(K), else
+        /// IList&lt;T&gt;.RemoveAt(int). Types with neither don't support item deletion.
+        /// </summary>
+        private void ResolveDeleter(Type type)
+        {
+            // Bind the interface method itself, not a member looked up by name: explicit implementations
+            // (e.g. ConcurrentDictionary.Remove, which only exposes TryRemove publicly) are reached this way.
+            var interfaces = type.GetInterfaces().AsEnumerable();
+            if (type.IsInterface)
+            {
+                interfaces = interfaces.Prepend(type);
+            }
+
+            foreach (var iface in interfaces)
+            {
+                if (iface.IsConstructedGenericType && iface.GetGenericTypeDefinition() == typeof(IDictionary<,>))
+                {
+                    var remove = iface.GetMethod(nameof(IDictionary<int, int>.Remove), new[] { iface.GetGenericArguments()[0] });
+                    if (remove != null)
+                    {
+                        DeleterBinder.AddMethod(remove, true);
+                    }
+                }
+            }
+            if (DeleterBinder.Count > 0)
+            {
+                return;
+            }
+
+            foreach (var iface in interfaces)
+            {
+                if (iface.IsConstructedGenericType && iface.GetGenericTypeDefinition() == typeof(IList<>))
+                {
+                    var removeAt = iface.GetMethod(nameof(IList<int>.RemoveAt), new[] { typeof(int) });
+                    if (removeAt != null)
+                    {
+                        DeleterBinder.AddMethod(removeAt, true);
+                    }
+                }
+            }
+        }
+
+        internal NewReference DeleteItem(BorrowedReference inst, BorrowedReference args)
+        {
+            return DeleterBinder.Invoke(inst, args, null);
+        }
+
         internal bool NeedsDefaultArgs(BorrowedReference args)
         {
             var pynargs = Runtime.PyTuple_Size(args);

src/testing/indexertest.cs

@@ -1,4 +1,6 @@
+using System;
 using System.Collections;
+using System.Collections.Generic;
 
 namespace Python.Test
 {
@@ -412,6 +414,38 @@ public MultiDefaultKeyIndexerTest() : base()
         }
     }
 
+    /// <summary>
+    /// IDictionary whose Remove throws: `del ob[key]` must surface it as a catchable Python error.
+    /// </summary>
+    public class ThrowingRemoveDictionary : IDictionary<string, string>
+    {
+        private readonly Dictionary<string, string> _items = new Dictionary<string, string>();
+
+        public string Marker => "alive";
+
+        public string this[string key]
+        {
+            get { return _items[key]; }
+            set { _items[key] = value; }
+        }
+
+        public ICollection<string> Keys => _items.Keys;
+        public ICollection<string> Values => _items.Values;
+        public int Count => _items.Count;
+        public bool IsReadOnly => false;
+        public void Add(string key, string value) => _items.Add(key, value);
+        public void Add(KeyValuePair<string, string> item) => _items.Add(item.Key, item.Value);
+        public void Clear() => _items.Clear();
+        public bool Contains(KeyValuePair<string, string> item) => _items.ContainsKey(item.Key);
+        public bool ContainsKey(string key) => _items.ContainsKey(key);
+        public void CopyTo(KeyValuePair<string, string>[] array, int arrayIndex) { }
+        public IEnumerator<KeyValuePair<string, string>> GetEnumerator() => _items.GetEnumerator();
+        IEnumerator IEnumerable.GetEnumerator() => _items.GetEnumerator();
+        public bool Remove(string key) => throw new InvalidOperationException("remove failed");
+        public bool Remove(KeyValuePair<string, string> item) => throw new InvalidOperationException("remove failed");
+        public bool TryGetValue(string key, out string value) => _items.TryGetValue(key, out value);
+    }
+
     public class PublicInheritedIndexerTest : PublicIndexerTest { }
 
     public class ProtectedInheritedIndexerTest : ProtectedIndexerTest { }