> On 31 Jul 2024, at 08:42, Jeff King <peff@xxxxxxxx> wrote:
>
> Hrm. Just looking at the code, here's a wild hypothesis: the problem
> could be not that the buffer is not NUL-terminated, but that after the
> NUL it contains junk, and we print that junk. That is, the code looks
> like this:
>
> /* If we can't get a CString pointer then
> * we need to allocate our own buffer */
> 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);
> }
>
> So we asked the system for the _maximum_ size that the string could be
> (and added one for the NUL). Then we got the string, and we printed out
> the _whole_ buffer, not just the string up to the NUL. And your fix
> "works" because NULs end up getting ignored on the read side (or at
> least cause ruby not to complain about bogus utf8).
>
> If that hypothesis is true, then the fix is more like:
>
> diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
> index 6ce22a28ed..1c8310d7fe 100644
> --- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c
> +++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
> @@ -141,7 +141,7 @@ static void find_username_in_item(CFDictionaryRef item)
> username_buf,
> buffer_len,
> ENCODING)) {
> - write_item("username", username_buf, buffer_len - 1);
> + write_item("username", username_buf, strlen(username_buf));
> }
> free(username_buf);
> }
This is correct.
The reason I couldn’t reproduce the problem and how few will have noticed up to
now is that for most users the CFStringGetCStringPtr call, which correctly uses
strlen, does what is necessary and we return early. I don't entirely know the
precise criteria where the fallback is used but I imagine it depends on certain
system encodings/locales.
The patch changing this to strlen looks good to me to apply to master & maint.
Bo
