On Sat, Feb 17, 2024 at 6:35 PM Bo Anderson via GitGitGadget
<gitgitgadget@xxxxxxxxx> 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 <mail@xxxxxxxxxxxxx>
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...
> diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
> @@ -3,14 +3,39 @@
> -__attribute__((format (printf, 1, 2)))
> +#define ENCODING kCFStringEncodingUTF8
> +static CFStringRef protocol; /* Stores constant strings - not memory managed */
> +static CFStringRef host;
> [...]
> +
> +static void clear_credential(void)
> +{
> + if (host) {
> + CFRelease(host);
> + host = NULL;
> + }
> + [...]
> +}
> +
> +__attribute__((format (printf, 1, 2), __noreturn__))
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.
> @@ -19,70 +44,135 @@ static void die(const char *err, ...)
> +static CFDictionaryRef create_dictionary(CFAllocatorRef allocator, ...)
> +{
> + va_list args;
> + const void *key;
> + CFMutableDictionaryRef result;
> +
> + result = CFDictionaryCreateMutable(allocator,
> + 0,
> + &kCFTypeDictionaryKeyCallBacks,
> + &kCFTypeDictionaryValueCallBacks);
> +
> +
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.
