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

[Eric Sunshine <sunshine@sunshineco.com>]

On Sun, Feb 18, 2024 at 9:48 AM Bo Anderson <mail@boanderson.me> wrote:
> > On 18 Feb 2024, at 06:08, Eric Sunshine <sunshine@sunshineco.com> wrote:
> >> +    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);
> >
> > 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: [...]
>
> 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.

Okay. I had thought that this check was merely protecting against
programmer error, but the described use-case to avoid passing NULL to
SecItem API makes perfect sense. It might be helpful to future readers
to explain this either as a function-level comment (explaining how to
call the function) or as an in-code comment.

> >> +    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.
> >
> >> +    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: [...]
> >
> > 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.

Most likely, anyone working on this code is already familiar with the
CoreFoundation API, thus would understand implicitly that this isn't
leaking. But, yes, a simple comment should be plenty sufficient for
everyone else if you are re-rolling anyhow.