[src] Fix GC-safety issues in a number of APIs.#26147
Conversation
…-written bindings. Fix a batch of genuinely GC-unsafe hand-written binding methods flagged by the HandleSafety test: they fetched a native handle from a managed object but didn't keep that object alive across the native use of the handle, so the object could be collected (and finalized, invalidating the handle) while the native call was still in progress. The fix is to keep the object alive until after the native call: * Most methods just add a GC.KeepAlive (obj) after the native call (capturing the return value in a local first where needed). * CoreFoundation.CFArray.Create captures the native return value in a local declared outside the 'fixed' block and calls GC.KeepAlive after it. * Darwin.SystemLog.Search is split so the native asl_search call runs in a normal method: in the original iterator the compiler hoists 'this' and 'msg' into the generated state machine, defeating the keep-alive analysis. Methods fixed span AddressBook, AudioToolbox, AudioUnit, CoreFoundation, CoreGraphics, CoreMidi, CoreServices, CoreText, CoreVideo, Darwin, Foundation, ImageIO, NearbyInteraction, QuickLook, Security and VideoToolbox. The now- resolved entries are removed from tests/cecil-tests/HandleSafety.KnownFailures.cs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: dd86cf03-297a-43e8-ae1f-3c0b22bb67c4
There was a problem hiding this comment.
Pull request overview
This PR addresses GC-safety in multiple hand-written bindings by ensuring managed objects aren’t collected/finalized while their native handles are in use (typically by adding GC.KeepAlive after the P/Invoke/native call). This aligns with the HandleSafety test’s goal of preventing use-after-free of native handles derived from managed wrappers.
Changes:
- Adds
GC.KeepAlivepatterns across a broad set of bindings that pass managed-derived native handles into native APIs. - Refactors
Darwin.SystemLog.Searchto avoid iterator state-machine hoisting interfering with keep-alive analysis. - Removes now-resolved entries from
tests/cecil-tests/HandleSafety.KnownFailures.cs.
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/cecil-tests/HandleSafety.KnownFailures.cs | Removes entries for APIs that are now GC-safe per HandleSafety expectations. |
| src/VideoToolbox/VTPixelTransferSession.cs | Keeps the properties dictionary alive across VTSessionSetProperties. |
| src/VideoToolbox/VTPixelRotationSession.cs | Keeps the properties dictionary alive across VTSessionSetProperties. |
| src/VideoToolbox/VTDecompressionSession.cs | Keeps the properties dictionary alive across VTSessionSetProperties. |
| src/VideoToolbox/VTCompressionSession.cs | Keeps the properties dictionary alive across VTSessionSetProperties. |
| src/Security/Items.cs | Keeps keychain query dictionaries alive across SecItem* calls. |
| src/Security/Certificate.cs | Keeps algorithm constants alive across SecKey* native calls. |
| src/QuickLook/Thumbnail.cs | Keeps option keys alive across dictionary low-level set calls. |
| src/NearbyInteraction/NIAlgorithmConvergenceStatusReasonValues.cs | Keeps the returned NSString constant alive across native description lookup. |
| src/ImageIO/CGImageSource.cs | Keeps auxiliary-data type constant alive across native copy call. |
| src/ImageIO/CGImageDestination.cs | Keeps auxiliary-data type constant alive across native add call. |
| src/Foundation/NSFastEnumerator.cs | Keeps the enumerated collection alive across the objc_msgSend fast-enum call. |
| src/Foundation/NSDecimal.cs | Keeps the current locale alive across NSDecimalString native call. |
| src/Darwin/SystemLog.cs | Moves asl_search out of the iterator to ensure msg stays alive during the native call. |
| src/CoreVideo/CVPixelBufferPool.cs | Keeps allocation settings alive across pixel buffer creation. |
| src/CoreVideo/CVPixelBuffer.cs | Keeps pixel buffer attributes alive across native buffer creation calls. |
| src/CoreVideo/CVImageBuffer.cs | Keeps strongly-typed string constants alive across code-point native lookups. |
| src/CoreText/CTTypesetter.cs | Keeps options alive across native typesetter creation. |
| src/CoreText/CTTextTab.cs | Keeps options alive across native text-tab creation. |
| src/CoreText/CTFramesetter.cs | Keeps frame attributes alive across native frame-size suggestion call. |
| src/CoreText/CTFontCollection.cs | Keeps options alive across native font-collection creation/matching calls. |
| src/CoreServices/FSEvents.cs | Keeps allocator object alive across FSEventStreamCreate calls. |
| src/CoreMidi/MidiServices.cs | Keeps MIDI objects alive across native add calls. |
| src/CoreGraphics/CGPDFDocument.cs | Keeps outline options alive across native outline-setting call. |
| src/CoreFoundation/CFPreferences.cs | Keeps key/application/value objects alive across CFPreferencesSetAppValue native calls. |
| src/CoreFoundation/CFDictionary.cs | Keeps key/value arrays alive across CFDictionaryCreate. |
| src/CoreFoundation/CFArray.cs | Keeps the values array alive across CFArrayCreate. |
| src/AudioUnit/AudioUnit.cs | Keeps the preset dictionary alive across AudioUnitSetProperty. |
| src/AudioToolbox/MusicTrack.cs | Keeps the owning sequence alive across MusicSequenceDisposeTrack. |
| src/AddressBook/ABMultiValue.cs | Keeps the backing ABMultiValue alive across native calls for entry value/label/id access. |
Comments suppressed due to low confidence (1)
src/Darwin/SystemLog.cs:256
aslresponse_free (search)is currently executed only if the iterator runs to completion. If the caller stops enumerating early (or the enumerator is disposed), the nativeaslresponsehandle can leak because the cleanup code after theyield returnloop will never run. Wrap the enumeration in atry/finallyso the native response is freed even on early-exit, and guard againstasl_searchreturningIntPtr.Zero.
static IEnumerable<Message> EnumerateSearchResults (IntPtr search)
{
IntPtr mh;
while ((mh = aslresponse_next (search)) != IntPtr.Zero)
yield return new Message (mh, true);
aslresponse_free (search);
}
This comment has been minimized.
This comment has been minimized.
✅ API diff for current PR / commitNET (empty diffs)✅ API diff vs stableNET (empty diffs)ℹ️ Generator diffGenerator Diff: vsdrops (html) vsdrops (raw diff) gist (raw diff) - Please review changes) Pipeline on Agent |
This comment has been minimized.
This comment has been minimized.
🚀 [CI Build #2000d63] Test results 🚀Test results✅ All tests passed on VSTS: test results. 🎉 All 203 tests passed 🎉 Tests counts✅ assembly-processing: All 1 tests passed. Html Report (VSDrops) Download macOS tests✅ Tests on macOS Monterey (12): All 5 tests passed. Html Report (VSDrops) Download Linux Build VerificationPipeline on Agent |
Fix a batch of genuinely GC-unsafe hand-written binding methods flagged by the HandleSafety test: they fetched a native handle from a managed object but didn't keep that object alive across the native use of the handle, so the object could be collected (and finalized, invalidating the handle) while the native call was still in progress.
The fix is to keep the object alive until after the native call:
Methods fixed span AddressBook, AudioToolbox, AudioUnit, CoreFoundation, CoreGraphics, CoreMidi, CoreServices, CoreText, CoreVideo, Darwin, Foundation, ImageIO, NearbyInteraction, QuickLook, Security and VideoToolbox. The now-resolved entries are removed from tests/cecil-tests/HandleSafety.KnownFailures.cs.
Contributes towards #10146.
🤖 Pull request created by Copilot