Thank you for detailed insights. > 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? I tested osxkeychain with the modification at the end of this note and got the following log for "git fetch --all --prune --recurse-submodules". SecItemUpdate() sometimes returns errSecDuplicateItem but the keychain item seems okay -- its value is correct after the command finished -- perhaps because one of successful operations stores the correct value. Even if every store operation fails, perhaps the originally stored value is kept and no damage occurs. XXX: get XXXX: protocol=https XXXX: host=github.com XXXX: wwwauth[]=Basic realm="GitHub" XXX: store XXXX: protocol=https XXXX: host=github.com XXXX: username=jenkins XXXX: password=ghq_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx XXXX: -25299 XXXXX: 0 XXX: get XXXX: protocol=https XXXX: host=github.com XXXX: wwwauth[]=Basic realm="GitHub" XXX: store XXXX: protocol=https XXXX: host=github.com XXXX: username=jenkins XXXX: password=ghq_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx XXXX: -25299 XXXXX: 0 XXX: get XXXX: protocol=https XXXX: host=github.com XXXX: wwwauth[]=Basic realm="GitHub" XXX: store XXXX: protocol=https XXXX: host=github.com XXXX: username=jenkins XXXX: password=ghq_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx XXXX: -25299 XXXXX: 0 XXX: get XXXX: protocol=https XXXX: host=github.com XXXX: wwwauth[]=Basic realm="GitHub" XXX: store XXXX: protocol=https XXXX: host=github.com XXXX: username=jenkins XXXX: password=ghq_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx XXXX: -25299 XXXXX: -25299 failed to store: -25299 XXX: get XXXX: protocol=https XXXX: host=github.com XXXX: wwwauth[]=Basic realm="GitHub" XXX: store XXXX: protocol=https XXXX: host=github.com XXXX: username=jenkins XXXX: password=ghq_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx XXXX: -25299 XXXXX: 0 XXX: get XXXX: protocol=https XXXX: host=github.com XXXX: wwwauth[]=Basic realm="GitHub" XXX: store XXXX: protocol=https XXXX: host=github.com XXXX: username=jenkins XXXX: password=ghq_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx XXXX: -25299 XXXXX: -25299 failed to store: -25299 ... This issue however occurs only when fetch.parallel is configured. If fetch.parallel is not configured, we should not ignore any error (including errSecDuplicateItem). Also, the above unstable behaviour is essentially caused by running osxkeychain instances in parallel where some of them treat "get" and others treat "store". I've also considered treating errSecDuplicateItem of SecItemUpdate() as errSecSuccess, but ended up with the current patch for these reasons. > 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. I agree on this point and would like to know the reason. Koji Nakamaru diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c index 6a40917b1e..0373857731 100644 --- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c +++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c @@ -308,10 +308,12 @@ static OSStatus add_internet_password(void) NULL); result = SecItemAdd(attrs, NULL); + fprintf(stderr, "XXXX: %d\n", result); if (result == errSecDuplicateItem) { CFDictionaryRef query; query = CREATE_SEC_ATTRIBUTES(NULL); result = SecItemUpdate(query, attrs); + fprintf(stderr, "XXXXX: %d\n", result); CFRelease(query); } @@ -333,6 +335,7 @@ static void read_credential(void) if (!strcmp(buf, "\n")) break; buf[line_len-1] = '\0'; + fprintf(stderr, "XXXX: %s\n", buf); v = strchr(buf, '='); if (!v) @@ -414,6 +417,7 @@ int main(int argc, const char **argv) if (!argv[1]) die("%s", usage); + fprintf(stderr, "XXX: %s\n", argv[1]); read_credential(); if (!strcmp(argv[1], "get")) 2024年5月10日(金) 23:58 Bo Anderson <mail@xxxxxxxxxxxxx>: > > 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 > > On 10 May 2024, at 09:07, Koji Nakamaru via GitGitGadget <gitgitgadget@xxxxxxxxx> wrote: > > From: Koji Nakamaru <koji.nakamaru@xxxxxxxx> > > 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 <koji.nakamaru@xxxxxxxx> > --- > 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 > >
