[PATCH v3 0/2] osxkeychain: lock for exclusive execution

["Koji Nakamaru via GitGitGadget" <gitgitgadget@gmail.com>]

Koji Nakamaru (2):
  osxkeychain: exclusive lock to serialize execution of operations
  osxkeychain: state to skip unnecessary store operations

 .../osxkeychain/git-credential-osxkeychain.c       | 14 ++++++++++++++
 1 file changed, 14 insertions(+)


base-commit: 83f1add914c6b4682de1e944ec0d1ac043d53d78
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1729%2FKojiNakamaru%2Ffeature%2Fosxkeychian_exlock-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1729/KojiNakamaru/feature/osxkeychian_exlock-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1729

Range-diff vs v2:

 1:  309c17c78f3 ! 1:  3341346c5e6 osxkeychain: lock for exclusive execution
     @@ Metadata
      Author: Koji Nakamaru <koji.nakamaru@gree.net>
      
       ## Commit message ##
     -    osxkeychain: lock for exclusive execution
     +    osxkeychain: exclusive lock to serialize execution of operations
      
     -    Resolves "failed to store: -25299" when "fetch.parallel 0" is configured
     -    and there are many submodules.
     +    git passes a credential that has been used successfully to the helpers
     +    to record. If "git-credential-osxkeychain store" commands run in
     +    parallel (with fetch.parallel configuration and/or by running multiple
     +    git commands simultaneously), some of them may exit with the error
     +    "failed to store: -25299". This is because SecItemUpdate() in
     +    add_internet_password() may return errSecDuplicateItem (-25299) in this
     +    situation. Apple's documentation [1] also states as below:
      
     -    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.
     +      In macOS, some of the functions of this API block while waiting for
     +      input from the user (for example, when the user is asked to unlock a
     +      keychain or give permission to change trust settings). In general, it
     +      is safe to use this API in threads other than your main thread, but
     +      avoid calling the functions from multiple operations, work queues, or
     +      threads concurrently. Instead, serialize function calls or confine
     +      them to a single thread.
     +
     +    The error has not been noticed before, because the former implementation
     +    ignored the error.
     +
     +    Introduce an exclusive lock to serialize execution of operations.
     +
     +    [1] https://developer.apple.com/documentation/security/certificate_key_and_trust_services/working_with_concurrency
      
          Signed-off-by: Koji Nakamaru <koji.nakamaru@gree.net>
      
 2:  1f57718abff ! 2:  146b0ae9146 osxkeychain: state[] seen=1 to skip unnecessary store operations
     @@ Metadata
      Author: Koji Nakamaru <koji.nakamaru@gree.net>
      
       ## Commit message ##
     -    osxkeychain: state[] seen=1 to skip unnecessary store operations
     +    osxkeychain: state to skip unnecessary store operations
      
     -    Records whether credentials come from get operations and skips
     -    unnecessary store operations by utilizing the state[] feature, as
     -    suggested by brian m. carlson.
     +    git passes a credential that has been used successfully to the helpers
     +    to record. If a credential is already stored,
     +    "git-credential-osxkeychain store" just records the credential returned
     +    by "git-credential-osxkeychain get", and unnecessary (sometimes
     +    problematic) SecItemAdd() and/or SecItemUpdate() are performed.
      
     +    We can skip such unnecessary operations by marking a credential returned
     +    by "git-credential-osxkeychain get". This marking can be done by
     +    utilizing the "state[]" feature:
     +
     +    - The "get" command sets the field "state[]=osxkeychain:seen=1".
     +
     +    - The "store" command skips its actual operation if the field
     +      "state[]=osxkeychain:seen=1" exists.
     +
     +    Introduce a new state "state[]=osxkeychain:seen=1".
     +
     +    Suggested-by: brian m. carlson <sandals@crustytoothpaste.net>
          Signed-off-by: Koji Nakamaru <koji.nakamaru@gree.net>
      
       ## contrib/credential/osxkeychain/git-credential-osxkeychain.c ##

-- 
gitgitgadget