Re: [PATCH] osxkeychain: lock for exclusive execution

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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






[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux