From cc3bea83705559c203f2ceab8d4b23b9b5957c62 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 7 Oct 2026 21:41:03 -0400 Subject: [PATCH] =?UTF-8?q?fix(ab):=20final=20review=20M2=20=E2=80=94=20a?= =?UTF-8?q?=20retyped=20libplctag=20handle=20field=20degrades=20to=20keep-?= =?UTF-8?q?alive?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit HandleOf cast the reflected nativeTagHandle with (int) and was called inside the runtimes' catch blocks, so a libplctag bump that retyped the field would replace every create failure with an InvalidCastException. The field is now accepted only if it is still an int (otherwise CanReadHandles is false and orphans are rooted), and HandleOf never throws. Both projects pin the field type explicitly. Claude-Session: https://claude.ai/code/session_01Ef2ozxS7QPanGT8uoATbTe --- .../LibplctagOrphanRelease.cs | 23 +++++++++++++++---- .../LibplctagOrphanRelease.cs | 23 +++++++++++++++---- .../LibplctagOrphanReleaseTests.cs | 22 ++++++++++++++++++ .../LibplctagLegacyOrphanReleaseTests.cs | 22 ++++++++++++++++++ 4 files changed, 82 insertions(+), 8 deletions(-) diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagOrphanRelease.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagOrphanRelease.cs index 11b756a9..f0defa6e 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagOrphanRelease.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagOrphanRelease.cs @@ -31,7 +31,11 @@ internal static class LibplctagOrphanRelease private const BindingFlags Private = BindingFlags.NonPublic | BindingFlags.Instance; private static readonly FieldInfo? WrapperField = typeof(Tag).GetField("_tag", Private); - private static readonly FieldInfo? HandleField = WrapperField?.FieldType.GetField("nativeTagHandle", Private); + // Final review M2: the field must also still be an int. A wrapper that retyped it (IntPtr, long) would make + // the cast in HandleOf throw; treating it as unreadable instead degrades to the rooting fallback ("keep + // alive"), which the pin test reports. + private static readonly FieldInfo? HandleField = + WrapperField?.FieldType.GetField("nativeTagHandle", Private) is { } f && f.FieldType == typeof(int) ? f : null; /// Tags whose handle could not be read: kept reachable so their callback delegate is never collected. private static readonly ConcurrentBag Rooted = []; @@ -50,9 +54,20 @@ internal static class LibplctagOrphanRelease /// Orphaned tags rooted for the process lifetime because their handle could not be read. internal static int RootedCount => Rooted.Count; - /// The native handle the wrapper holds (0 when none was ever created, or unreadable). - internal static int HandleOf(Tag tag) => - HandleField is not null && WrapperField!.GetValue(tag) is { } wrapper ? (int)HandleField.GetValue(wrapper)! : 0; + /// The native handle the wrapper holds (0 when none was ever created, or unreadable). Never throws: + /// callers run it inside a catch block, where an exception would replace the failure being handled. + internal static int HandleOf(Tag tag) + { + try + { + return HandleField is not null && WrapperField!.GetValue(tag) is { } wrapper + && HandleField.GetValue(wrapper) is int handle ? handle : 0; + } + catch (Exception) + { + return 0; + } + } /// Clears the wrapper's handle after a destroy, so a repeat release is a no-op (review r1 M2). private static void ClearHandle(Tag tag) diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/LibplctagOrphanRelease.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/LibplctagOrphanRelease.cs index ffaf5274..7b4ec909 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/LibplctagOrphanRelease.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/LibplctagOrphanRelease.cs @@ -31,7 +31,11 @@ internal static class LibplctagOrphanRelease private const BindingFlags Private = BindingFlags.NonPublic | BindingFlags.Instance; private static readonly FieldInfo? WrapperField = typeof(Tag).GetField("_tag", Private); - private static readonly FieldInfo? HandleField = WrapperField?.FieldType.GetField("nativeTagHandle", Private); + // Final review M2: the field must also still be an int. A wrapper that retyped it (IntPtr, long) would make + // the cast in HandleOf throw; treating it as unreadable instead degrades to the rooting fallback ("keep + // alive"), which the pin test reports. + private static readonly FieldInfo? HandleField = + WrapperField?.FieldType.GetField("nativeTagHandle", Private) is { } f && f.FieldType == typeof(int) ? f : null; /// Tags whose handle could not be read: kept reachable so their callback delegate is never collected. private static readonly ConcurrentBag Rooted = []; @@ -50,9 +54,20 @@ internal static class LibplctagOrphanRelease /// Orphaned tags rooted for the process lifetime because their handle could not be read. internal static int RootedCount => Rooted.Count; - /// The native handle the wrapper holds (0 when none was ever created, or unreadable). - internal static int HandleOf(Tag tag) => - HandleField is not null && WrapperField!.GetValue(tag) is { } wrapper ? (int)HandleField.GetValue(wrapper)! : 0; + /// The native handle the wrapper holds (0 when none was ever created, or unreadable). Never throws: + /// callers run it inside a catch block, where an exception would replace the failure being handled. + internal static int HandleOf(Tag tag) + { + try + { + return HandleField is not null && WrapperField!.GetValue(tag) is { } wrapper + && HandleField.GetValue(wrapper) is int handle ? handle : 0; + } + catch (Exception) + { + return 0; + } + } /// Clears the wrapper's handle after a destroy, so a repeat release is a no-op (review r1 M2). private static void ClearHandle(Tag tag) diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/LibplctagOrphanReleaseTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/LibplctagOrphanReleaseTests.cs index 55dd52ad..3d1b3a62 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/LibplctagOrphanReleaseTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/LibplctagOrphanReleaseTests.cs @@ -36,6 +36,28 @@ public sealed class LibplctagOrphanReleaseTests LibplctagOrphanRelease.CanReadHandles.ShouldBeTrue(); } + /// + /// Final review M2: the handle field's TYPE is pinned explicitly, not just its presence. A retype (IntPtr, + /// long) now degrades to the rooting fallback instead of an InvalidCastException inside a catch block, + /// and this names the reason when it happens. + /// + [Fact] + public void The_pinned_wrapper_handle_field_is_still_an_int() + { + const System.Reflection.BindingFlags priv = + System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance; + var wrapper = typeof(libplctag.Tag).GetField("_tag", priv).ShouldNotBeNull(); + wrapper.FieldType.GetField("nativeTagHandle", priv).ShouldNotBeNull().FieldType.ShouldBe(typeof(int)); + } + + /// HandleOf never throws (it runs inside the runtimes' catch blocks); a never-created tag reads 0. + [Fact] + public void HandleOf_a_fresh_tag_is_zero_and_does_not_throw() + { + using var tag = new libplctag.Tag(); + LibplctagOrphanRelease.HandleOf(tag).ShouldBe(0); + } + [Fact] public async Task A_failed_initialisation_destroys_the_native_tag_it_created() { diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/LibplctagLegacyOrphanReleaseTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/LibplctagLegacyOrphanReleaseTests.cs index 8b34d284..913eff67 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/LibplctagLegacyOrphanReleaseTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/LibplctagLegacyOrphanReleaseTests.cs @@ -16,6 +16,28 @@ public sealed class LibplctagLegacyOrphanReleaseTests public void The_pinned_libplctag_wrapper_still_exposes_the_native_handle() => LibplctagOrphanRelease.CanReadHandles.ShouldBeTrue(); + /// + /// Final review M2: the handle field's TYPE is pinned explicitly, not just its presence. A retype (IntPtr, + /// long) now degrades to the rooting fallback instead of an InvalidCastException inside a catch block, + /// and this names the reason when it happens. + /// + [Fact] + public void The_pinned_wrapper_handle_field_is_still_an_int() + { + const System.Reflection.BindingFlags priv = + System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance; + var wrapper = typeof(libplctag.Tag).GetField("_tag", priv).ShouldNotBeNull(); + wrapper.FieldType.GetField("nativeTagHandle", priv).ShouldNotBeNull().FieldType.ShouldBe(typeof(int)); + } + + /// HandleOf never throws (it runs inside the runtimes' catch blocks); a never-created tag reads 0. + [Fact] + public void HandleOf_a_fresh_tag_is_zero_and_does_not_throw() + { + using var tag = new libplctag.Tag(); + LibplctagOrphanRelease.HandleOf(tag).ShouldBe(0); + } + [Fact] public async Task A_failed_initialisation_destroys_the_native_tag_it_created() {