From 4e5afdf973e29f1ae50413aa0cc092f2a03df68f Mon Sep 17 00:00:00 2001 From: Frank Witscher Date: Mon, 13 May 2024 16:25:11 +0200 Subject: [PATCH 1/4] Fix access violation exception on shutdown (#1977) When nulling the GC handles on shutdown the reference count of all objects pointed to by the IntPtr in the `CLRObject.reflectedObjects` are zero. This caused an exception in some scenarios because `Runtime.PyObject_TYPE(reflectedClrObject)` is called while the reference counter is at zero. After `TypeManager.RemoveTypes();` is called in the `Runtime.Shutdown()` method, reference count decrements to zero do not invoke `ClassBase.tp_clear` for managed objects anymore which normally is responsible for removing references from `CLRObject.reflectedObjects`. Collecting objects referenced in `CLRObject.reflectedObjects` only after leads to an unstable state in which the reference count for these object addresses is zero while still maintaining them to be used for further pseudo-cleanup. In that time, the memory could have been reclaimed already which leads to the exception. --- AUTHORS.md | 1 + CHANGELOG.md | 2 ++ src/runtime/Runtime.cs | 5 +++-- 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/AUTHORS.md b/AUTHORS.md index 18435671c..6aa4a6010 100644 --- a/AUTHORS.md +++ b/AUTHORS.md @@ -38,6 +38,7 @@ - Dmitriy Se ([@dmitriyse](https://github.com/dmitriyse)) - Félix Bourbonnais ([@BadSingleton](https://github.com/BadSingleton)) - Florian Treurniet ([@ftreurni](https://github.com/ftreurni)) +- Frank Witscher ([@Frawak](https://github.com/Frawak)) - He-chien Tsai ([@t3476](https://github.com/t3476)) - Inna Wiesel ([@inna-w](https://github.com/inna-w)) - Ivan Cronyn ([@cronan](https://github.com/cronan)) diff --git a/CHANGELOG.md b/CHANGELOG.md index 23184258d..7d2faa1b7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,7 @@ This document follows the conventions laid out in [Keep a CHANGELOG][]. - Fixed RecursionError for reverse operators on C# operable types from python. See #2240 - Fixed probing for assemblies in `sys.path` failing when a path in `sys.path` has invalid characters. See #2376 +- Fixed possible access violation exception on shutdown. See ([#1977][i1977]) ## [3.0.3](https://github.com/pythonnet/pythonnet/releases/tag/v3.0.3) - 2023-10-11 @@ -970,3 +971,4 @@ This version improves performance on benchmarks significantly compared to 2.3. [i1481]: https://github.com/pythonnet/pythonnet/issues/1481 [i1672]: https://github.com/pythonnet/pythonnet/pull/1672 [i2311]: https://github.com/pythonnet/pythonnet/issues/2311 +[i1977]: https://github.com/pythonnet/pythonnet/issues/1977 diff --git a/src/runtime/Runtime.cs b/src/runtime/Runtime.cs index 2f9e18f65..a65fea66f 100644 --- a/src/runtime/Runtime.cs +++ b/src/runtime/Runtime.cs @@ -278,6 +278,8 @@ internal static void Shutdown() ClearClrModules(); RemoveClrRootModule(); + TryCollectingGarbage(MaxCollectRetriesOnShutdown, forceBreakLoops: true); + NullGCHandles(ExtensionType.loadedExtensions); ClassManager.RemoveClasses(); TypeManager.RemoveTypes(); @@ -295,8 +297,7 @@ internal static void Shutdown() PyObjectConversions.Reset(); PyGC_Collect(); - bool everythingSeemsCollected = TryCollectingGarbage(MaxCollectRetriesOnShutdown, - forceBreakLoops: true); + bool everythingSeemsCollected = TryCollectingGarbage(MaxCollectRetriesOnShutdown); Debug.Assert(everythingSeemsCollected); Finalizer.Shutdown(); From 6f0f6713e8f55a24ea7803584c5490eca0518739 Mon Sep 17 00:00:00 2001 From: Frank Witscher Date: Mon, 13 May 2024 22:38:59 +0200 Subject: [PATCH 2/4] Restrict first garbage collection Otherwise, collecting all at this earlier point results in corrupt memory for derived types. --- src/runtime/Finalizer.cs | 8 ++++---- src/runtime/Runtime.cs | 10 +++++++--- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/src/runtime/Finalizer.cs b/src/runtime/Finalizer.cs index 713564f08..5b5ecfcfc 100644 --- a/src/runtime/Finalizer.cs +++ b/src/runtime/Finalizer.cs @@ -191,7 +191,7 @@ internal static void Shutdown() Instance.started = false; } - internal nint DisposeAll() + internal nint DisposeAll(bool disposeObj = true, bool disposeDerived = true, bool disposeBuffer = true) { if (_objQueue.IsEmpty && _derivedQueue.IsEmpty && _bufferQueue.IsEmpty) return 0; @@ -216,7 +216,7 @@ internal nint DisposeAll() try { - while (!_objQueue.IsEmpty) + if (disposeObj) while (!_objQueue.IsEmpty) { if (!_objQueue.TryDequeue(out var obj)) continue; @@ -240,7 +240,7 @@ internal nint DisposeAll() } } - while (!_derivedQueue.IsEmpty) + if (disposeDerived) while (!_derivedQueue.IsEmpty) { if (!_derivedQueue.TryDequeue(out var derived)) continue; @@ -258,7 +258,7 @@ internal nint DisposeAll() collected++; } - while (!_bufferQueue.IsEmpty) + if (disposeBuffer) while (!_bufferQueue.IsEmpty) { if (!_bufferQueue.TryDequeue(out var buffer)) continue; diff --git a/src/runtime/Runtime.cs b/src/runtime/Runtime.cs index a65fea66f..b3820270c 100644 --- a/src/runtime/Runtime.cs +++ b/src/runtime/Runtime.cs @@ -278,7 +278,8 @@ internal static void Shutdown() ClearClrModules(); RemoveClrRootModule(); - TryCollectingGarbage(MaxCollectRetriesOnShutdown, forceBreakLoops: true); + TryCollectingGarbage(MaxCollectRetriesOnShutdown, forceBreakLoops: true, + obj: true, derived: false, buffer: false); NullGCHandles(ExtensionType.loadedExtensions); ClassManager.RemoveClasses(); @@ -329,7 +330,8 @@ internal static void Shutdown() const int MaxCollectRetriesOnShutdown = 20; internal static int _collected; - static bool TryCollectingGarbage(int runs, bool forceBreakLoops) + static bool TryCollectingGarbage(int runs, bool forceBreakLoops, + bool obj = true, bool derived = true, bool buffer = true) { if (runs <= 0) throw new ArgumentOutOfRangeException(nameof(runs)); @@ -342,7 +344,9 @@ static bool TryCollectingGarbage(int runs, bool forceBreakLoops) GC.Collect(); GC.WaitForPendingFinalizers(); pyCollected += PyGC_Collect(); - pyCollected += Finalizer.Instance.DisposeAll(); + pyCollected += Finalizer.Instance.DisposeAll(disposeObj: obj, + disposeDerived: derived, + disposeBuffer: buffer); } if (Volatile.Read(ref _collected) == 0 && pyCollected == 0) { From c99cdf3efef20451b96417d9422e21b8bcbf2cf4 Mon Sep 17 00:00:00 2001 From: Frank Witscher Date: Mon, 8 Jul 2024 09:49:16 +0200 Subject: [PATCH 3/4] Throw exception trying to add a reflected object after the hashset is cleared --- src/runtime/Runtime.cs | 2 ++ src/runtime/Types/ClrObject.cs | 8 ++++++++ 2 files changed, 10 insertions(+) diff --git a/src/runtime/Runtime.cs b/src/runtime/Runtime.cs index b3820270c..3b9b0ce48 100644 --- a/src/runtime/Runtime.cs +++ b/src/runtime/Runtime.cs @@ -158,6 +158,7 @@ internal static void Initialize(bool initSigs = false) ClassManager.Reset(); ClassDerivedObject.Reset(); TypeManager.Initialize(); + CLRObject.creationBlocked = false; _typesInitialized = true; // Initialize modules that depend on the runtime class. @@ -356,6 +357,7 @@ static bool TryCollectingGarbage(int runs, bool forceBreakLoops, { NullGCHandles(CLRObject.reflectedObjects); CLRObject.reflectedObjects.Clear(); + CLRObject.creationBlocked = true; } } return false; diff --git a/src/runtime/Types/ClrObject.cs b/src/runtime/Types/ClrObject.cs index 4cf9062cb..afa136414 100644 --- a/src/runtime/Types/ClrObject.cs +++ b/src/runtime/Types/ClrObject.cs @@ -11,10 +11,15 @@ internal sealed class CLRObject : ManagedType { internal readonly object inst; + internal static bool creationBlocked = false; + // "borrowed" references internal static readonly HashSet reflectedObjects = new(); static NewReference Create(object ob, BorrowedReference tp) { + if (creationBlocked) + throw new InvalidOperationException("Reflected objects should not be created anymore."); + Debug.Assert(tp != null); var py = Runtime.PyType_GenericAlloc(tp, 0); @@ -61,6 +66,9 @@ internal static void Restore(object ob, BorrowedReference pyHandle, Dictionary? context) { + if (creationBlocked) + throw new InvalidOperationException("Reflected objects should not be loaded anymore."); + base.OnLoad(ob, context); GCHandle gc = GCHandle.Alloc(this); SetGCHandle(ob, gc); From 6cdd6d7d7b7c50781390c5e59978cdc90967ef97 Mon Sep 17 00:00:00 2001 From: Frank Witscher Date: Mon, 5 Aug 2024 09:46:07 +0200 Subject: [PATCH 4/4] Move reflected object creation block Otherwise if you have a Python object that needs to temporarily create a .NET object in its destructor (for instance write log summary to a file), its destructor will fail if it happens to be freed on the second iteration of loop breaking. --- src/runtime/Runtime.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/runtime/Runtime.cs b/src/runtime/Runtime.cs index 3b9b0ce48..4fa4f5957 100644 --- a/src/runtime/Runtime.cs +++ b/src/runtime/Runtime.cs @@ -281,6 +281,7 @@ internal static void Shutdown() TryCollectingGarbage(MaxCollectRetriesOnShutdown, forceBreakLoops: true, obj: true, derived: false, buffer: false); + CLRObject.creationBlocked = true; NullGCHandles(ExtensionType.loadedExtensions); ClassManager.RemoveClasses(); @@ -357,7 +358,6 @@ static bool TryCollectingGarbage(int runs, bool forceBreakLoops, { NullGCHandles(CLRObject.reflectedObjects); CLRObject.reflectedObjects.Clear(); - CLRObject.creationBlocked = true; } } return false;