Re: [PATCH 1/4] osxkeychain: replace deprecated SecKeychain API

[Bo Anderson <mail@boanderson.me>]

> On 18 Feb 2024, at 06:08, Eric Sunshine <sunshine@sunshineco.com> wrote:
> 
> 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...

Thanks for taking a look!

>> 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.

Acknowledged. It is indeed a bit of a nothing change that doesn’t really do much on its own, but when paired with the port variable reorder could potentially make a “minor code cleanup” commit.

>> +       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.
> 

In this case it’s by design to accept and check for NULL values as it greatly simplifies the code. Inputs to the credential helpers have various optional fields, such as port and path. It is programmer error to pass NULL to the SecItem API (runtime crash) so in order to simplify having to check each individual field in all of the callers (and probably ditch varargs since you can’t really do dynamic varargs), I check the value here instead. That means you can do something like:

 create_dictionary(kCFAllocatorDefault,
     kSecAttrServer, host,
     kSecAttrPath, path, \
     kSecAttrPort, port,
     NULL)

And it will only include the key-value pairs that have non-NULL values.

It would indeed be programmer error to not pass key-value pairs, though it is equally programmer error to not have a terminating NULL.

>> +       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.

Would it make sense to just have a comment paired with the CFStringGetCStringPtr return explaining why it doesn’t need to be freed there? I’m OK with the to_free variable however if that’s clearer. Idea in my mind was pairing it based on `xmalloc` but I can see why pairing based on variable is clearer.