From e8d8dc19676b429ee1059d1079c47a67e9ece35f Mon Sep 17 00:00:00 2001 From: Jhonathan Abreu Date: Wed, 2 Sep 2026 11:14:33 -0400 Subject: [PATCH 1/3] 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.Remove / IList.RemoveAt through the binder (KeyError on a missing dictionary key), TypeError for every other type and for arrays. --- src/embed_tests/TestIndexerDelete.cs | 101 ++++++++++++++++++++++ src/runtime/ClassManager.cs | 2 + src/runtime/Types/ArrayObject.cs | 7 ++ src/runtime/Types/ClassBase.cs | 45 ++++++++++ src/runtime/Types/Indexer.cs | 57 ++++++++++++ src/testing/indexertest.cs | 34 ++++++++ tests/test_indexer.py | 125 +++++++++++++++++++++++++++ 7 files changed, 371 insertions(+) create mode 100644 src/embed_tests/TestIndexerDelete.cs diff --git a/src/embed_tests/TestIndexerDelete.cs b/src/embed_tests/TestIndexerDelete.cs new file mode 100644 index 000000000..9a10009d2 --- /dev/null +++ b/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 +{ + /// + /// `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. + /// + [TestFixture] + public class TestIndexerDelete + { + [OneTimeSetUp] + public void SetUp() + { + PythonEngine.Initialize(); + } + + [OneTimeTearDown] + public void Dispose() + { + PythonEngine.Shutdown(); + } + + public class SettableIndexer + { + private readonly Dictionary _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()); + Assert.AreEqual("one", scope.Eval("ob[1]").As()); + } + } + + [Test] + public void DelOnConcurrentDictionaryRemovesKey() + { + using (Py.GIL()) + { + using var scope = Py.CreateScope(); + var dict = new ConcurrentDictionary(); + 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()); + } + } + + [Test] + public void DelOnDictionaryMissingKeyRaisesKeyError() + { + using (Py.GIL()) + { + using var scope = Py.CreateScope(); + scope.Set("d", new Dictionary { ["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()); + } + } + } +} diff --git a/src/runtime/ClassManager.cs b/src/runtime/ClassManager.cs index b88a6a6b6..5b2460108 100644 --- a/src/runtime/ClassManager.cs +++ b/src/runtime/ClassManager.cs @@ -681,6 +681,8 @@ void AddMember(string name, string snakeCasedName, bool isStaticReadonlyCallable } } + ci.indexer?.ResolveDeleter(type); + return ci; } diff --git a/src/runtime/Types/ArrayObject.cs b/src/runtime/Types/ArrayObject.cs index 3ca09ddce..0a03cf22b 100644 --- a/src/runtime/Types/ArrayObject.cs +++ b/src/runtime/Types/ArrayObject.cs @@ -245,6 +245,13 @@ public static NewReference mp_subscript(BorrowedReference ob, BorrowedReference /// 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(); diff --git a/src/runtime/Types/ClassBase.cs b/src/runtime/Types/ClassBase.cs index ed1659789..de0503db8 100644 --- a/src/runtime/Types/ClassBase.cs +++ b/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; } + /// + /// Implements __delitem__ (del ob[key]) for reflected classes: IDictionary<K,V>.Remove or + /// IList<T>.RemoveAt through the binder, TypeError for everything else. + /// + static int DeleteItemImpl(ClassBase cls, BorrowedReference ob, BorrowedReference idx) + { + if (cls.indexer == null || !cls.indexer.CanDelete) + { + 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.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); diff --git a/src/runtime/Types/Indexer.cs b/src/runtime/Types/Indexer.cs index 2ef079710..3d7dacc60 100644 --- a/src/runtime/Types/Indexer.cs +++ b/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,11 @@ public bool CanSet get { return SetterBinder.Count > 0; } } + public bool CanDelete + { + get { return DeleterBinder?.Count > 0; } + } + public void AddProperty(PropertyInfo pi) { @@ -55,6 +64,54 @@ internal void SetItem(BorrowedReference inst, BorrowedReference args) SetterBinder.Invoke(inst, args, null); } + /// + /// Resolves the method behind del ob[key]: IDictionary<K,V>.Remove(K), else + /// IList<T>.RemoveAt(int). Types with neither don't support item deletion. + /// + internal 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.Remove), new[] { iface.GetGenericArguments()[0] }); + if (remove != null) + { + DeleterBinder.AddMethod(remove, true); + } + } + } + if (CanDelete) + { + return; + } + + foreach (var iface in interfaces) + { + if (iface.IsConstructedGenericType && iface.GetGenericTypeDefinition() == typeof(IList<>)) + { + var removeAt = iface.GetMethod(nameof(IList.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); diff --git a/src/testing/indexertest.cs b/src/testing/indexertest.cs index 08e6ad053..2088a5ad8 100644 --- a/src/testing/indexertest.cs +++ b/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() } } + /// + /// IDictionary whose Remove throws: `del ob[key]` must surface it as a catchable Python error. + /// + public class ThrowingRemoveDictionary : IDictionary + { + private readonly Dictionary _items = new Dictionary(); + + public string Marker => "alive"; + + public string this[string key] + { + get { return _items[key]; } + set { _items[key] = value; } + } + + public ICollection Keys => _items.Keys; + public ICollection 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 item) => _items.Add(item.Key, item.Value); + public void Clear() => _items.Clear(); + public bool Contains(KeyValuePair item) => _items.ContainsKey(item.Key); + public bool ContainsKey(string key) => _items.ContainsKey(key); + public void CopyTo(KeyValuePair[] array, int arrayIndex) { } + public IEnumerator> GetEnumerator() => _items.GetEnumerator(); + IEnumerator IEnumerable.GetEnumerator() => _items.GetEnumerator(); + public bool Remove(string key) => throw new InvalidOperationException("remove failed"); + public bool Remove(KeyValuePair 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 { } diff --git a/tests/test_indexer.py b/tests/test_indexer.py index 7db68df3e..aac6fbfe0 100644 --- a/tests/test_indexer.py +++ b/tests/test_indexer.py @@ -642,3 +642,128 @@ def test_public_inherited_overloaded_indexer(): with pytest.raises(TypeError): ob[[]] + + +def test_del_settable_indexer_raises_type_error(): + """`del ob[key]` on a type with a settable indexer but no delete support must raise + a catchable TypeError, not crash the process (PythonnetEnterprise GH #167).""" + ob = Test.PublicIndexerTest() + ob[0] = "zero" + + with pytest.raises(TypeError): + del ob[0] + + # The interpreter is alive and the object is untouched and still usable. + assert ob[0] == "zero" + ob[0] = "one" + assert ob[0] == "one" + + +def test_del_multi_arg_indexer_raises_type_error(): + """Tuple-key counterpart: the multi-parameter setter path must not see the null value.""" + ob = Test.MultiArgIndexerTest() + ob[0, 1] = "zero-one" + + with pytest.raises(TypeError): + del ob[0, 1] + + assert ob[0, 1] == "zero-one" + + +def test_del_dictionary_item(): + """`del d[key]` removes the key via IDictionary.Remove; a missing key is a KeyError.""" + from System.Collections.Generic import Dictionary + + d = Dictionary[str, str]() + d["MyKey"] = "MyValue" + + with pytest.raises(KeyError): + del d["missing"] + assert d.Count == 1 + + del d["MyKey"] + assert d.Count == 0 + assert not d.ContainsKey("MyKey") + + with pytest.raises(KeyError): + del d["MyKey"] + + +def test_del_dictionary_wrong_key_type(): + from System.Collections.Generic import Dictionary + + d = Dictionary[str, str]() + d["a"] = "b" + + with pytest.raises(TypeError): + del d[1] + + assert d.Count == 1 + + +def test_del_concurrent_dictionary_item(): + """ConcurrentDictionary implements IDictionary.Remove explicitly (only TryRemove is a + public member). It is the type behind QCAlgorithm.RuntimeStatistics in GH #167.""" + from System.Collections.Concurrent import ConcurrentDictionary + + d = ConcurrentDictionary[str, str]() + d["MyKey"] = "MyValue" + assert d["MyKey"] == "MyValue" + + del d["MyKey"] + + assert d.Count == 0 + assert not d.ContainsKey("MyKey") + + with pytest.raises(KeyError): + del d["MyKey"] + + +def test_del_list_item(): + """`del l[i]` removes the element via IList.RemoveAt; out of range surfaces the .NET error.""" + from System import ArgumentOutOfRangeException + from System.Collections.Generic import List + + l = List[str]() + l.Add("a") + l.Add("b") + + with pytest.raises(ArgumentOutOfRangeException): + del l[5] + assert l.Count == 2 + + del l[0] + assert l.Count == 1 + assert l[0] == "b" + + +def test_del_array_item_raises_type_error(): + from System import Array + + a = Array[int]([1, 2, 3]) + + with pytest.raises(TypeError): + del a[0] + + assert a[0] == 1 + + +def test_del_on_object_without_indexer_raises_type_error(): + from System import Uri + + with pytest.raises(TypeError): + del Uri("http://www.example.com")[0] + + +def test_throwing_remove_does_not_crash(): + """A managed Remove that throws must raise a catchable Python exception and leave the + interpreter and the object usable.""" + ob = Test.ThrowingRemoveDictionary() + ob["k"] = "v" + + with pytest.raises(Exception) as excinfo: + del ob["k"] + assert "InvalidOperationException" in type(excinfo.value).__name__ + + assert ob.Marker == "alive" + assert ob["k"] == "v" From 543a0a49ef790c2a72293c371177d3221a095a45 Mon Sep 17 00:00:00 2001 From: Jhonathan Abreu Date: Thu, 3 Sep 2026 12:03:07 -0400 Subject: [PATCH 2/3] Resolve the item deleter lazily on the first del Most types are never deleted from, so the interface walk that finds IDictionary.Remove or IList.RemoveAt now runs on the first del instead of at class creation. --- src/runtime/ClassManager.cs | 2 -- src/runtime/Types/ClassBase.cs | 2 +- src/runtime/Types/Indexer.cs | 21 +++++++++++++++++---- 3 files changed, 18 insertions(+), 7 deletions(-) diff --git a/src/runtime/ClassManager.cs b/src/runtime/ClassManager.cs index 5b2460108..b88a6a6b6 100644 --- a/src/runtime/ClassManager.cs +++ b/src/runtime/ClassManager.cs @@ -681,8 +681,6 @@ void AddMember(string name, string snakeCasedName, bool isStaticReadonlyCallable } } - ci.indexer?.ResolveDeleter(type); - return ci; } diff --git a/src/runtime/Types/ClassBase.cs b/src/runtime/Types/ClassBase.cs index de0503db8..cdb00c548 100644 --- a/src/runtime/Types/ClassBase.cs +++ b/src/runtime/Types/ClassBase.cs @@ -573,7 +573,7 @@ static int mp_ass_subscript_impl(BorrowedReference ob, BorrowedReference idx, Bo /// static int DeleteItemImpl(ClassBase cls, BorrowedReference ob, BorrowedReference idx) { - if (cls.indexer == null || !cls.indexer.CanDelete) + 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; diff --git a/src/runtime/Types/Indexer.cs b/src/runtime/Types/Indexer.cs index 3d7dacc60..a2f19b0f9 100644 --- a/src/runtime/Types/Indexer.cs +++ b/src/runtime/Types/Indexer.cs @@ -33,9 +33,22 @@ public bool CanSet get { return SetterBinder.Count > 0; } } - public bool CanDelete + // 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) { - get { return DeleterBinder?.Count > 0; } + if (!_deleterResolved) + { + _deleterResolved = true; + DeleterBinder ??= new MethodBinder(); + if (DeleterBinder.Count == 0) + { + ResolveDeleter(type); + } + } + return DeleterBinder.Count > 0; } @@ -68,7 +81,7 @@ internal void SetItem(BorrowedReference inst, BorrowedReference args) /// Resolves the method behind del ob[key]: IDictionary<K,V>.Remove(K), else /// IList<T>.RemoveAt(int). Types with neither don't support item deletion. /// - internal void ResolveDeleter(Type type) + 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. @@ -89,7 +102,7 @@ internal void ResolveDeleter(Type type) } } } - if (CanDelete) + if (DeleterBinder.Count > 0) { return; } From 0115d6de5e614649e9a8933fe2d120a0c96ff839 Mon Sep 17 00:00:00 2001 From: Jhonathan Abreu Date: Thu, 3 Sep 2026 14:31:10 -0400 Subject: [PATCH 3/3] Bump version to 2.0.66 --- src/perf_tests/Python.PerformanceTests.csproj | 4 ++-- src/runtime/Properties/AssemblyInfo.cs | 4 ++-- src/runtime/Python.Runtime.csproj | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/perf_tests/Python.PerformanceTests.csproj b/src/perf_tests/Python.PerformanceTests.csproj index 2c18d49cd..dd7061c2b 100644 --- a/src/perf_tests/Python.PerformanceTests.csproj +++ b/src/perf_tests/Python.PerformanceTests.csproj @@ -13,7 +13,7 @@ runtime; build; native; contentfiles; analyzers; buildtransitive - + compile @@ -25,7 +25,7 @@ - + diff --git a/src/runtime/Properties/AssemblyInfo.cs b/src/runtime/Properties/AssemblyInfo.cs index 875a1286d..0a24781b8 100644 --- a/src/runtime/Properties/AssemblyInfo.cs +++ b/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")] diff --git a/src/runtime/Python.Runtime.csproj b/src/runtime/Python.Runtime.csproj index d8e720ef4..765a0dd2c 100644 --- a/src/runtime/Python.Runtime.csproj +++ b/src/runtime/Python.Runtime.csproj @@ -5,7 +5,7 @@ Python.Runtime Python.Runtime QuantConnect.pythonnet - 2.0.65 + 2.0.66 false LICENSE https://github.com/pythonnet/pythonnet