On Sat, Feb 17, 2024 at 6:35 PM Bo Anderson via GitGitGadget
[off-list ref] wrote:

The SecKeychain API was deprecated in macOS 10.10, nearly 10 years ago.
The replacement SecItem API however is available as far back as macOS
10.6.

While supporting older macOS was perhaps prevously a concern,
git-credential-osxkeychain already requires a minimum of macOS 10.7
since 5747c8072b (contrib/credential: avoid fixed-size buffer in
osxkeychain, 2023-05-01) so using the newer API should not regress the
range of macOS versions supported.

Adapting to use the newer SecItem API also happens to fix two test
failures in osxkeychain:

    8 - helper (osxkeychain) overwrites on store
    9 - helper (osxkeychain) can forget host

The new API is compatible with credentials saved with the older API.

Signed-off-by: Bo Anderson <redacted>

I haven't studied the SecItem API, so I can't comment on the meat of
the patch, but I can make a few generic observations...

The addition of `__noreturn__` to the `__attribute__` seems unrelated
to the stated purpose of this patch. As such, it typically would be
placed in its own patch. If it really is too minor for a separate
patch, mentioning it in the commit message as a "While at it..." would
be helpful.

Style: one blank line is preferred over two

+       va_start(args, allocator);
+       while ((key = va_arg(args, const void *)) != NULL) {
+               const void *value;
+               value = va_arg(args, const void *);
+               if (value)
+                       CFDictionarySetValue(result, key, value);
+       }
+       va_end(args);

A couple related comments...

If va_arg() ever returns NULL for `value`, the next iteration of the
loop will call va_arg() again, but calling va_arg() again after it has
already returned NULL is likely undefined behavior. At minimum, I
would have expected this to be written as:

    while (...) {
        ...
        if (!value)
            break;
        CFDictionarySetValue(...);
    }

However, isn't it a programmer error if va_arg() returns NULL for
`value`? If so, I'd think we'd want to scream loudly about that rather
than silently ignoring it. So, perhaps something like this:

    while (...) {
        ...
        if (!value) {
            fprintf(stderr, "BUG: ...");
            abort();
        }
        CFDictionarySetValue(...);
   }

Or, perhaps just call the existing die() function in this file with a
suitable "BUG ..." message.

+static void find_username_in_item(CFDictionaryRef item)
 {
+       CFStringRef account_ref;
+       char *username_buf;
+       CFIndex buffer_len;

+       account_ref = CFDictionaryGetValue(item, kSecAttrAccount);
+       if (!account_ref)
+       {
+               write_item("username", "", 0);
+               return;
+       }

Style: opening brace sticks to the `if` line:

    if !(account_ref) {
        ...
    }

Same comment applies to the `if` below.

+       username_buf = (char *)CFStringGetCStringPtr(account_ref, ENCODING);
+       if (username_buf)
+       {
+               write_item("username", username_buf, strlen(username_buf));
                return;
+       }

According to the documentation for CFStringGetCStringPtr(), the
returned C-string is not newly-allocated, so the caller does not have
to free it. Therefore, can `username_buf` be declared `const char *`
rather than `char *` to make it clear to readers that nothing is being
leaked here? Same comment about the `(char *)` cast.

+       /* If we can't get a CString pointer then
+        * we need to allocate our own buffer */

Style:

    /*
     * Multi-line comments
     * are formatted like this.
     */

+       buffer_len = CFStringGetMaximumSizeForEncoding(
+                       CFStringGetLength(account_ref), ENCODING) + 1;
+       username_buf = xmalloc(buffer_len);
+       if (CFStringGetCString(account_ref,
+                               username_buf,
+                               buffer_len,
+                               ENCODING)) {
+               write_item("username", username_buf, buffer_len - 1);
+       }
+       free(username_buf);

Okay, this explains why `username_buf` is declared `char *` rather
than `const char *`. Typically, when we have a situation in which a
value may or may not need freeing, we use a `to_free` variable like
this:

    const char *username_buf;
    char *to_free = NULL;
    ...
    username_buf = (const char *)CFStringGetCStringPtr(...);
    if (username_buf) {
        ...
        return;
    }
    ...
    username_buf = to_free = xmalloc(buffer_len);
    if (CFStringGetCString(...))
        ...
    free(to_free);

But that may be overkill for this simple case, and what you have here
may be "good enough" for anyone already familiar with the API and who
knows that the `return` after CFStringGetCStringPtr() isn't leaking.

+static OSStatus find_internet_password(void)
 {
+       CFDictionaryRef attrs;
+       [...]

+       attrs = CREATE_SEC_ATTRIBUTES(kSecMatchLimit, kSecMatchLimitOne,
+                                     kSecReturnAttributes, kCFBooleanTrue,
+                                     kSecReturnData, kCFBooleanTrue,
+                                     NULL);
+       result = SecItemCopyMatching(attrs, (CFTypeRef *)&item);
+       if (result) {
+               goto out;
+       }

We omit braces when the body is a single statement:

    if (result)
        goto out;

(Same comment applies to other code in this patch.)

+       data = CFDictionaryGetValue(item, kSecValueData);
+       [...]
+
+out:
+       CFRelease(attrs);

Good, `attrs` is released in all cases.

+static OSStatus add_internet_password(void)
 {
+       [...]
+       attrs = CREATE_SEC_ATTRIBUTES(kSecValueData, password,
+                                     NULL);
+       result = SecItemAdd(attrs, NULL);
+       if (result == errSecDuplicateItem) {
+               CFDictionaryRef query;
+               query = CREATE_SEC_ATTRIBUTES(NULL);
+               result = SecItemUpdate(query, attrs);
+               CFRelease(query);
+       }
+       CFRelease(attrs);
+       return result;
 }

Good, `attrs` and `query` are released in all cases.