secret service: fix error creating credential #996
Merged
Conversation
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
Multiple Linux users have reported that they are unable to use Secret Service as their credential store, as GCM throws the following error: sec_free: Assertion `cell->requested > 0' failed. The root cause is that we're using the libsecret secret_value_get() function to obtain secret data, then attempting to free the string with secret_password_free(). It appears that secret_password_free() is only meant to be used to free nonpageable memory [1], however. Removing this call fixes the issue, as verified with a successful git-credential-manager diagnose (which was previously failing with the above error). [1] secret_password_free manpage https://www.manpagez.com/html/libsecret-1/libsecret-1-0.18.6/libsecret-Password-storage.php#secret-password-free
dscho
approved these changes
Dec 16, 2022
mjcheetham
reviewed
Dec 16, 2022
| if (accountKeyPtr != IntPtr.Zero) Marshal.FreeHGlobal(accountKeyPtr); | ||
| if (serviceKeyPtr != IntPtr.Zero) Marshal.FreeHGlobal(serviceKeyPtr); | ||
| if (value != null) secret_value_unref(value); | ||
| if (passwordPtr != IntPtr.Zero) secret_password_free(passwordPtr); |
There was a problem hiding this comment.
Oh wow! How did I miss this? Great job
You have to free pageable memory too, its just that GCM doesn't use |
|
Lol indeed I can't take credit for identifying the fix. I just followed @js6pak's suggestion. |
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.
Multiple Linux users have reported that they are unable to use Secret Service as their credential store, as GCM throws the following error:
sec_free: Assertion `cell->requested > 0' failed.
The root cause is that we're using the libsecret secret_value_get function to obtain secret data, then attempting to free the string with secret_password_free. It appears that secret_password_free is only meant to be used to free nonpageable memory [1], however. Removing this call fixes the issue, as verified with a successful git-credential-manager diagnose (which was previously failing with the above error).
[1] secret_password_free manpage
https://www.manpagez.com/html/libsecret-1/libsecret-1-0.18.6/libsecret-Password-storage.php#secret-password-free
Fixes #793