Fix native memory leak in Keychain.GetKeychainPath error path#178
Merged
Fix native memory leak in Keychain.GetKeychainPath error path#178
Conversation
Wrap SecKeychainGetPath call and result handling in try/finally to ensure Marshal.FreeHGlobal(pathBuffer) executes even when an exception is thrown on non-OK status. Previously, the 1 KB native buffer leaked on every failed call. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes an unmanaged-memory leak in Xamarin.MacDev.Keychain.GetKeychainPath() by ensuring the native buffer allocated for SecKeychainGetPath is always released, including when the native call fails and an exception is thrown. This is a targeted reliability fix in the keychain interop layer.
Changes:
- Wrapped the
SecKeychainGetPathcall in atry/finally. - Moved
Marshal.FreeHGlobal(pathBuffer)into thefinallyblock so cleanup runs on success and failure. - Kept the existing success-path behavior unchanged while closing the native leak on error paths.
rolfbjarne
requested changes
May 5, 2026
…Path Replace unmanaged allocation with a pinned managed byte array as suggested by rolfbjarne. This eliminates the need for try/finally cleanup since the GC handles the buffer lifetime. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Comment on lines
431
to
+437
| uint bufferLength = 1024; | ||
| var pathBuffer = Marshal.AllocHGlobal ((int) bufferLength); | ||
| var status = SecKeychainGetPath (keychainPtr, out bufferLength, pathBuffer); | ||
| var pathBuffer = new byte [bufferLength]; | ||
| OSStatus status; | ||
| unsafe { | ||
| fixed (byte* pathPtr = pathBuffer) | ||
| status = SecKeychainGetPath (keychainPtr, out bufferLength, (IntPtr) pathPtr); | ||
| } |
rolfbjarne
approved these changes
May 6, 2026
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.
Summary
In
Keychain.GetKeychainPath,Marshal.AllocHGlobalallocates a 1 KB native buffer for the keychain path. WhenSecKeychainGetPathreturns a non-OK status, the exception thrown at line 436 bypasses theMarshal.FreeHGlobalcall, leaking the buffer. In long-lived processes (e.g., IDE plugins) that retry keychain access after transient failures, each failure leaks 1 KB of unmanaged memory.Changes
Wrapped the
SecKeychainGetPathcall and result handling in atry/finallyblock, movingMarshal.FreeHGlobal(pathBuffer)into thefinallyclause. This ensures the native buffer is freed on both success and error paths.Verification
dotnet build Xamarin.MacDev.sln— 0 warnings, 0 errors (TreatWarningsAsErrors enabled)dotnet test tests/tests.csproj -f net10.0— all tests passdotnet format --verify-no-changes— formatting correct on changed file