git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH] osxkeychain: lock for exclusive execution

From
Koji Nakamaru <koji.nakamaru@gree.net>
Date
May 10, 2024, 18:26 UTC
Message-ID
<CAOTNsDzveWCr4wx2vqJF_YfRkF5QyhHpopqfw-CiG2xcNduC2Q@mail.gmail.com>
In-Reply-To
<C0C8F71D-2A01-4C31-9EB6-AB31FA17C3AB@boanderson.me>
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@boanderson.me>:
>
> 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@gmail.com> wrote:
>
> From: Koji Nakamaru <koji.nakamaru@gree.net>
>
> 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@gree.net>
> ---
>    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
>
>
Previous: Koji Nakamaru
Message 21 of 21 in “osxkeychain: lock for exclusive execution”
  1. osxkeychain: lock for exclusive executionKoji Nakamaru via GitGitGadget, May 10, 2024
  2. Bo AndersonMay 10, 2024
  3. Jeff KingMay 10, 2024
  4. brian m. carlsonMay 10, 2024
  5. Jeff KingMay 10, 2024
  6. brian m. carlsonMay 10, 2024
  7. Junio C HamanoMay 10, 2024
  8. Jeff KingMay 10, 2024
  9. Junio C HamanoMay 10, 2024
  10. 0/2 osxkeychain: lock for exclusive executionKoji Nakamaru via GitGitGadget, May 11, 2024
  11. 1/2 osxkeychain: lock for exclusive executionKoji Nakamaru via GitGitGadget, May 11, 2024
  12. Junio C HamanoMay 12, 2024
  13. Koji NakamaruMay 12, 2024
  14. 2/2 osxkeychain: state[] seen=1 to skip unnecessary store operationsKoji Nakamaru via GitGitGadget, May 11, 2024
  15. Junio C HamanoMay 12, 2024
  16. Koji NakamaruMay 12, 2024
  17. 0/2 osxkeychain: lock for exclusive executionKoji Nakamaru via GitGitGadget, May 15, 2024
  18. 1/2 osxkeychain: exclusive lock to serialize execution of operationsKoji Nakamaru via GitGitGadget, May 15, 2024
  19. 2/2 osxkeychain: state to skip unnecessary store operationsKoji Nakamaru via GitGitGadget, May 15, 2024
  20. Koji NakamaruMay 15, 2024
  21. Koji NakamaruMay 10, 2024

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.