Visitar URL original
Fix: del obj[key] on a reflected indexer crashed the process (#149) · QuantConnect/pythonnet@56b3b6e · GitHub
Skip to content

Commit 56b3b6e

Browse files
jhonabreuljhonabreul
authored
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
1 parent 9fc7ff1 commit 56b3b6e

9 files changed

Lines changed: 387 additions & 5 deletions

File tree

‎src/embed_tests/TestIndexerDelete.cs‎

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
using System.Collections.Concurrent;
2+
using System.Collections.Generic;
3+
4+
using NUnit.Framework;
5+
6+
using Python.Runtime;
7+
8+
namespace Python.EmbeddingTest
9+
{
10+
/// <summary>
11+
/// `del ob[key]` reaches mp_ass_subscript with a null value. It must raise a catchable Python
12+
/// exception (or delete, for IDictionary/IList types) instead of aborting the process.
13+
/// </summary>
14+
[TestFixture]
15+
public class TestIndexerDelete
16+
{
17+
[OneTimeSetUp]
18+
public void SetUp()
19+
{
20+
PythonEngine.Initialize();
21+
}
22+
23+
[OneTimeTearDown]
24+
public void Dispose()
25+
{
26+
PythonEngine.Shutdown();
27+
}
28+
29+
public class SettableIndexer
30+
{
31+
private readonly Dictionary<int, string> _items = new();
32+
33+
public string this[int key]
34+
{
35+
get => _items[key];
36+
set => _items[key] = value;
37+
}
38+
39+
public string Marker => "alive";
40+
}
41+
42+
[Test]
43+
public void DelOnSettableIndexerRaisesTypeError()
44+
{
45+
using (Py.GIL())
46+
{
47+
using var scope = Py.CreateScope();
48+
scope.Set("ob", new SettableIndexer().ToPython());
49+
scope.Exec(@"
50+
ob[1] = 'one'
51+
raised = None
52+
try:
53+
del ob[1]
54+
except TypeError as e:
55+
raised = e
56+
");
57+
using var raised = scope.Get("raised");
58+
Assert.IsFalse(raised.IsNone(), "del must raise TypeError");
59+
Assert.AreEqual("alive", scope.Eval("ob.Marker").As<string>());
60+
Assert.AreEqual("one", scope.Eval("ob[1]").As<string>());
61+
}
62+
}
63+
64+
[Test]
65+
public void DelOnConcurrentDictionaryRemovesKey()
66+
{
67+
using (Py.GIL())
68+
{
69+
using var scope = Py.CreateScope();
70+
var dict = new ConcurrentDictionary<string, string>();
71+
dict["MyKey"] = "MyValue";
72+
scope.Set("d", dict.ToPython());
73+
74+
scope.Exec("del d['MyKey']");
75+
76+
Assert.IsFalse(dict.ContainsKey("MyKey"));
77+
Assert.AreEqual(0, scope.Eval("d.Count").As<int>());
78+
}
79+
}
80+
81+
[Test]
82+
public void DelOnDictionaryMissingKeyRaisesKeyError()
83+
{
84+
using (Py.GIL())
85+
{
86+
using var scope = Py.CreateScope();
87+
scope.Set("d", new Dictionary<string, int> { ["a"] = 1 }.ToPython());
88+
scope.Exec(@"
89+
raised = None
90+
try:
91+
del d['missing']
92+
except KeyError as e:
93+
raised = e
94+
");
95+
using var raised = scope.Get("raised");
96+
Assert.IsFalse(raised.IsNone(), "del of a missing key must raise KeyError");
97+
Assert.AreEqual(1, scope.Eval("d.Count").As<int>());
98+
}
99+
}
100+
}
101+
}

‎src/perf_tests/Python.PerformanceTests.csproj‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@
1313
<IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
1414
</PackageReference>
1515
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="16.*" />
16-
<PackageReference Include="quantconnect.pythonnet" Version="2.0.65" GeneratePathProperty="true">
16+
<PackageReference Include="quantconnect.pythonnet" Version="2.0.66" GeneratePathProperty="true">
1717
<IncludeAssets>compile</IncludeAssets>
1818
</PackageReference>
1919
</ItemGroup>
@@ -25,7 +25,7 @@
2525
</Target>
2626

2727
<Target Name="CopyBaseline" AfterTargets="Build">
28-
<Copy SourceFiles="$(NuGetPackageRoot)quantconnect.pythonnet\2.0.65\lib\net10.0\Python.Runtime.dll" DestinationFolder="$(OutDir)baseline" />
28+
<Copy SourceFiles="$(NuGetPackageRoot)quantconnect.pythonnet\2.0.66\lib\net10.0\Python.Runtime.dll" DestinationFolder="$(OutDir)baseline" />
2929
</Target>
3030

3131
<Target Name="CopyNewBuild" AfterTargets="Build">

‎src/runtime/Properties/AssemblyInfo.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,5 +4,5 @@
44
[assembly: InternalsVisibleTo("Python.EmbeddingTest, PublicKey=00240000048000009400000006020000002400005253413100040000110000005ffd8f49fb44ab0641b3fd8d55e749f716e6dd901032295db641eb98ee46063cbe0d4a1d121ef0bc2af95f8a7438d7a80a3531316e6b75c2dae92fb05a99f03bf7e0c03980e1c3cfb74ba690aca2f3339ef329313bcc5dccced125a4ffdc4531dcef914602cd5878dc5fbb4d4c73ddfbc133f840231343e013762884d6143189")]
55
[assembly: InternalsVisibleTo("Python.Test, PublicKey=00240000048000009400000006020000002400005253413100040000110000005ffd8f49fb44ab0641b3fd8d55e749f716e6dd901032295db641eb98ee46063cbe0d4a1d121ef0bc2af95f8a7438d7a80a3531316e6b75c2dae92fb05a99f03bf7e0c03980e1c3cfb74ba690aca2f3339ef329313bcc5dccced125a4ffdc4531dcef914602cd5878dc5fbb4d4c73ddfbc133f840231343e013762884d6143189")]
66

7-
[assembly: AssemblyVersion("2.0.65")]
8-
[assembly: AssemblyFileVersion("2.0.65")]
7+
[assembly: AssemblyVersion("2.0.66")]
8+
[assembly: AssemblyFileVersion("2.0.66")]

‎src/runtime/Python.Runtime.csproj‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
<RootNamespace>Python.Runtime</RootNamespace>
66
<AssemblyName>Python.Runtime</AssemblyName>
77
<PackageId>QuantConnect.pythonnet</PackageId>
8-
<Version>2.0.65</Version>
8+
<Version>2.0.66</Version>
99
<GenerateAssemblyInfo>false</GenerateAssemblyInfo>
1010
<PackageLicenseFile>LICENSE</PackageLicenseFile>
1111
<RepositoryUrl>https://github.com/pythonnet/pythonnet</RepositoryUrl>

‎src/runtime/Types/ArrayObject.cs‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -245,6 +245,13 @@ public static NewReference mp_subscript(BorrowedReference ob, BorrowedReference
245245
/// </summary>
246246
public static int mp_ass_subscript(BorrowedReference ob, BorrowedReference idx, BorrowedReference v)
247247
{
248+
// `del arr[i]` arrives here with a null value; arrays are fixed-size, so refuse it up front.
249+
if (v.IsNull)
250+
{
251+
Exceptions.RaiseTypeError("array does not support item deletion");
252+
return -1;
253+
}
254+
248255
var obj = (CLRObject)GetManagedObject(ob)!;
249256
var items = (Array)obj.inst;
250257
Type itemType = obj.inst.GetType().GetElementType();

‎src/runtime/Types/ClassBase.cs‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -507,6 +507,13 @@ static int mp_ass_subscript_impl(BorrowedReference ob, BorrowedReference idx, Bo
507507
BorrowedReference tp = Runtime.PyObject_TYPE(ob);
508508
var cls = (ClassBase)GetManagedObject(tp)!;
509509

510+
// CPython routes `del ob[key]` through this same slot with a null value. None of the
511+
// assignment code below can take a null, so deletion must be handled before anything else.
512+
if (v.IsNull)
513+
{
514+
return DeleteItemImpl(cls, ob, idx);
515+
}
516+
510517
if (cls.indexer == null || !cls.indexer.CanSet)
511518
{
512519
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
560567
return 0;
561568
}
562569

570+
/// <summary>
571+
/// Implements __delitem__ (del ob[key]) for reflected classes: IDictionary&lt;K,V&gt;.Remove or
572+
/// IList&lt;T&gt;.RemoveAt through the binder, TypeError for everything else.
573+
/// </summary>
574+
static int DeleteItemImpl(ClassBase cls, BorrowedReference ob, BorrowedReference idx)
575+
{
576+
if (cls.indexer == null || !cls.type.Valid || !cls.indexer.CanDelete(cls.type.Value))
577+
{
578+
Exceptions.SetError(Exceptions.TypeError, "object doesn't support item deletion");
579+
return -1;
580+
}
581+
582+
if (Runtime.PyTuple_Check(idx))
583+
{
584+
Exceptions.SetError(Exceptions.TypeError, "object doesn't support multi-index item deletion");
585+
return -1;
586+
}
587+
588+
using var args = Runtime.PyTuple_New(1);
589+
Runtime.PyTuple_SetItem(args.Borrow(), 0, idx);
590+
591+
// The binder converts the key and turns a managed exception into a Python error.
592+
using var result = cls.indexer.DeleteItem(ob, args.Borrow());
593+
if (result.IsNull() || Exceptions.ErrorOccurred())
594+
{
595+
return -1;
596+
}
597+
598+
// IDictionary<K,V>.Remove reports a missing key by returning false; match dict semantics.
599+
if (result.Borrow() == Runtime.PyFalse)
600+
{
601+
Exceptions.SetError(Exceptions.KeyError, idx);
602+
return -1;
603+
}
604+
605+
return 0;
606+
}
607+
563608
static NewReference tp_call_impl(BorrowedReference ob, BorrowedReference args, BorrowedReference kw)
564609
{
565610
BorrowedReference tp = Runtime.PyObject_TYPE(ob);

‎src/runtime/Types/Indexer.cs‎

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,6 @@
11
using System;
2+
using System.Collections.Generic;
3+
using System.Linq;
24
using System.Reflection;
35

46
namespace Python.Runtime
@@ -11,11 +13,13 @@ internal class Indexer
1113
{
1214
public MethodBinder GetterBinder;
1315
public MethodBinder SetterBinder;
16+
public MethodBinder DeleterBinder;
1417

1518
public Indexer()
1619
{
1720
GetterBinder = new MethodBinder();
1821
SetterBinder = new MethodBinder();
22+
DeleterBinder = new MethodBinder();
1923
}
2024

2125

@@ -29,6 +33,24 @@ public bool CanSet
2933
get { return SetterBinder.Count > 0; }
3034
}
3135

36+
// The deleter is resolved on the first `del`: most types are never deleted from, so the
37+
// interface walk only runs for the ones that are. Called under the GIL, like the slot itself.
38+
[NonSerialized] private bool _deleterResolved;
39+
40+
public bool CanDelete(Type type)
41+
{
42+
if (!_deleterResolved)
43+
{
44+
_deleterResolved = true;
45+
DeleterBinder ??= new MethodBinder();
46+
if (DeleterBinder.Count == 0)
47+
{
48+
ResolveDeleter(type);
49+
}
50+
}
51+
return DeleterBinder.Count > 0;
52+
}
53+
3254

3355
public void AddProperty(PropertyInfo pi)
3456
{
@@ -55,6 +77,54 @@ internal void SetItem(BorrowedReference inst, BorrowedReference args)
5577
SetterBinder.Invoke(inst, args, null);
5678
}
5779

80+
/// <summary>
81+
/// Resolves the method behind <c>del ob[key]</c>: IDictionary&lt;K,V&gt;.Remove(K), else
82+
/// IList&lt;T&gt;.RemoveAt(int). Types with neither don't support item deletion.
83+
/// </summary>
84+
private void ResolveDeleter(Type type)
85+
{
86+
// Bind the interface method itself, not a member looked up by name: explicit implementations
87+
// (e.g. ConcurrentDictionary.Remove, which only exposes TryRemove publicly) are reached this way.
88+
var interfaces = type.GetInterfaces().AsEnumerable();
89+
if (type.IsInterface)
90+
{
91+
interfaces = interfaces.Prepend(type);
92+
}
93+
94+
foreach (var iface in interfaces)
95+
{
96+
if (iface.IsConstructedGenericType && iface.GetGenericTypeDefinition() == typeof(IDictionary<,>))
97+
{
98+
var remove = iface.GetMethod(nameof(IDictionary<int, int>.Remove), new[] { iface.GetGenericArguments()[0] });
99+
if (remove != null)
100+
{
101+
DeleterBinder.AddMethod(remove, true);
102+
}
103+
}
104+
}
105+
if (DeleterBinder.Count > 0)
106+
{
107+
return;
108+
}
109+
110+
foreach (var iface in interfaces)
111+
{
112+
if (iface.IsConstructedGenericType && iface.GetGenericTypeDefinition() == typeof(IList<>))
113+
{
114+
var removeAt = iface.GetMethod(nameof(IList<int>.RemoveAt), new[] { typeof(int) });
115+
if (removeAt != null)
116+
{
117+
DeleterBinder.AddMethod(removeAt, true);
118+
}
119+
}
120+
}
121+
}
122+
123+
internal NewReference DeleteItem(BorrowedReference inst, BorrowedReference args)
124+
{
125+
return DeleterBinder.Invoke(inst, args, null);
126+
}
127+
58128
internal bool NeedsDefaultArgs(BorrowedReference args)
59129
{
60130
var pynargs = Runtime.PyTuple_Size(args);

‎src/testing/indexertest.cs‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,6 @@
1+
using System;
12
using System.Collections;
3+
using System.Collections.Generic;
24

35
namespace Python.Test
46
{
@@ -412,6 +414,38 @@ public MultiDefaultKeyIndexerTest() : base()
412414
}
413415
}
414416

417+
/// <summary>
418+
/// IDictionary whose Remove throws: `del ob[key]` must surface it as a catchable Python error.
419+
/// </summary>
420+
public class ThrowingRemoveDictionary : IDictionary<string, string>
421+
{
422+
private readonly Dictionary<string, string> _items = new Dictionary<string, string>();
423+
424+
public string Marker => "alive";
425+
426+
public string this[string key]
427+
{
428+
get { return _items[key]; }
429+
set { _items[key] = value; }
430+
}
431+
432+
public ICollection<string> Keys => _items.Keys;
433+
public ICollection<string> Values => _items.Values;
434+
public int Count => _items.Count;
435+
public bool IsReadOnly => false;
436+
public void Add(string key, string value) => _items.Add(key, value);
437+
public void Add(KeyValuePair<string, string> item) => _items.Add(item.Key, item.Value);
438+
public void Clear() => _items.Clear();
439+
public bool Contains(KeyValuePair<string, string> item) => _items.ContainsKey(item.Key);
440+
public bool ContainsKey(string key) => _items.ContainsKey(key);
441+
public void CopyTo(KeyValuePair<string, string>[] array, int arrayIndex) { }
442+
public IEnumerator<KeyValuePair<string, string>> GetEnumerator() => _items.GetEnumerator();
443+
IEnumerator IEnumerable.GetEnumerator() => _items.GetEnumerator();
444+
public bool Remove(string key) => throw new InvalidOperationException("remove failed");
445+
public bool Remove(KeyValuePair<string, string> item) => throw new InvalidOperationException("remove failed");
446+
public bool TryGetValue(string key, out string value) => _items.TryGetValue(key, out value);
447+
}
448+
415449
public class PublicInheritedIndexerTest : PublicIndexerTest { }
416450

417451
public class ProtectedInheritedIndexerTest : ProtectedIndexerTest { }

0 commit comments

Comments
 (0)