fix: root registered-callback delegates and fix metadata handle ownership#15
Merged
Merged
Conversation
…ship
Round-19 audit (1:1 vs official C++/Python bindings) found two classes of
lifetime bugs where the GC could reclaim a delegate (or strand a native
ref) while the core still held the function pointer / handle.
1. Callback delegate lifetime (CallbackOnCollectedDelegate). Every site that
hands a function pointer to a core API storing it long-term built the
adapter delegate inline, leaving no rooted reference. Once the GC
collected the temporary the native pointer dangled and the next callback
crashed (AccessViolation). Fixed across:
- 5 Custom* ToNative structs (CustomBinaryView, CustomBinaryViewType,
CustomTransform, FileAccessor, LogListener): per-instance thunk fields.
- BNLogListener.FromNative copy-paste (close/getLogLevel pointed at log).
- 11 Custom*Command instance Register*() + 14 static RegisterPluginCommand*:
thunk fields / shared UnsafeUtils.PinCallback root.
- Core.RegisterBinaryViewEvent, BinaryViewType.RegisterPlatformRecognizer,
FlowGraph.StartLayout (async completion): PinCallback process root.
- Activity.Create/CreateWithEligibility: per-instance delegate fields.
- 22 UnsafeUtils.WrapProgressDelegate call sites: local + GC.KeepAlive
across the synchronous native call.
UnsafeUtils.PinCallback<T> centralizes the process-lifetime rooting so
every registration site routes through one obviously-correct helper.
2. Metadata native-handle ownership. Several paths addref'd a transferred-
owned handle (MustNewFromHandle) or leaked a caller-owned BNCreate* handle:
- Metadata.ToArray + MetadataValueStore.FromNative: MustTakeHandle (no
addref on a transferred-owned ref; container-only free).
- 12 scalar/array Metadata.SetValue overloads: wrap the BNCreate* handle
in an owning Metadata (using) and pass a borrowed handle; the core
addrefs internally so the value survives the wrapper's disposal.
e2e (outside repo, per project constraint): +2 modules (CallbackLifetime,
MetadataSetValueOwnership). 1470 passed / 0 failed / 3 skipped (pre-existing
absent-export skip). CallbackLifetime.logListenerSurvivesGc would crash the
process pre-fix; it now passes on all three samples.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Round-19 audit: delegate lifetime + metadata handle ownership
1:1 comparison against the official C++/Python
binaryninja-apifound two classes of lifetime bugs where the GC could reclaim a delegate (or strand a native ref) while the core still held the function pointer / handle.1. Callback delegate lifetime (CallbackOnCollectedDelegate → AccessViolation)
Every site handing a function pointer to a core API that stores it long-term built the adapter delegate inline, leaving no rooted reference. Once the GC collected the temporary, the native pointer dangled and the next callback crashed. Fixed across:
Custom*ToNative structs (CustomBinaryView,CustomBinaryViewType,CustomTransform,FileAccessor,LogListener): per-instance thunk fields.BNLogListener.FromNative: copy-paste fix (close/getLogLevelhad pointed atlog).Custom*CommandinstanceRegister*()+ 14 staticRegisterPluginCommand*: thunk fields / sharedUnsafeUtils.PinCallback.Core.RegisterBinaryViewEvent,BinaryViewType.RegisterPlatformRecognizer,FlowGraph.StartLayout(async completion):PinCallbackprocess-lifetime root.Activity.Create/CreateWithEligibility: per-instance delegate fields.UnsafeUtils.WrapProgressDelegatecall sites: local +GC.KeepAliveacross the synchronous native call.New
UnsafeUtils.PinCallback<T>centralizes the process-lifetime rooting so every registration site routes through one obviously-correct helper.2. Metadata native-handle ownership
Metadata.ToArray+MetadataValueStore.FromNative:MustTakeHandle(no addref on a transferred-owned ref; the core frees only the container).Metadata.SetValueoverloads: wrap theBNCreate*handle in an owningMetadata(using) and pass a borrowed handle; the core addrefs internally so the value survives the wrapper's disposal. (Previously leaked oneBNMetadataref per call.)Validation
CallbackLifetime(thelogListenerSurvivesGcprobe would crash the process pre-fix; now passes on all 3 samples) andMetadataSetValueOwnership(all 8 probes pass, incl.survivesGc).