Re: [PATCH] osxkeychain: lock for exclusive execution
From: Bo Anderson <hidden>
Date: 2024-05-10 15:02:19
Interesting. SecItemUpdate returning errSecDuplicateItem didn’t make sense to make sense to me so I had a check to see what scenario this happens and it appears to be a scenario where updating in-place fails but replacing it entirely succeeds. However it seems the item might have ultimately still been updated: https://github.com/apple-oss-distributions/Security/blob/0600e7bab30fbac3adcafcb6c57d3981dc682304/OSX/libsecurity_keychain/lib/SecItem.cpp#L2398 The behaviour is a bit odd and the associated code comment referencing an Apple bug number is perhaps is indicative of that. I guess it perhaps makes sense if you are holding references, but that doesn’t apply to us. I wonder if a fix here could be to treat errSecDuplicateItem as a successful operation for SecItemUpdate. Can you confirm the keychain item is successfully updated in that scenario? A broader Git-wide question that you perhaps don’t know the answer to but someone else here might do is: why are we spamming updates to the credential helper? Every parallel fetch instance performing a store operation on the same host seems unexpected to me, particularly if there’s no actual changes. Bo
quoted hunk ↗ jump to hunk
On 10 May 2024, at 09:07, Koji Nakamaru via GitGitGadget [off-list ref] wrote: From: Koji Nakamaru <redacted> Resolves "failed to store: -25299" when "fetch.parallel 0" is configured and there are many submodules. The error code -25299 (errSecDuplicateItem) may be returned by SecItemUpdate() in add_internet_password() if multiple instances of git-credential-osxkeychain run in parallel. This patch introduces an exclusive lock to serialize execution for avoiding this and other potential issues. Signed-off-by: Koji Nakamaru <redacted> --- osxkeychain: lock for exclusive execution Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1729%2FKojiNakamaru%2Ffeature%2Fosxkeychian_exlock-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1729/KojiNakamaru/feature/osxkeychian_exlock-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/1729 contrib/credential/osxkeychain/git-credential-osxkeychain.c | 3 +++ 1 file changed, 3 insertions(+)diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c index 6a40917b1ef..0884db48d0a 100644 --- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c +++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c@@ -414,6 +414,9 @@ int main(int argc, const char **argv)if (!argv[1]) die("%s", usage); + if (open(argv[0], O_RDONLY | O_EXLOCK) == -1) + die("failed to lock %s", argv[0]); + read_credential(); if (!strcmp(argv[1], "get")) base-commit: 0f3415f1f8478b05e64db11eb8aaa2915e48fef6 -- gitgitgadget