Re: [PATCH v2] gpg-interface: trim CR from ssh-keygen

[Junio C Hamano <gitster@pobox.com>]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Mon, Jan 3, 2022 at 9:24 AM Fabian Stelzer <fs@gigacodes.de> wrote:
>> We need to trim \r from the output of 'ssh-keygen -Y find-principals' on
>> Windows, or we end up calling 'ssh-keygen -Y verify' with a bogus signer
>> identity. ssh-keygen.c:2841 contains a call to puts(3), which confirms
>> this hypothesis. Signature verification passes with the fix.
>>
>> Helped-by: Pedro Martelletto <pedro@yubico.com>
>> Signed-off-by: Fabian Stelzer <fs@gigacodes.de>
>> ---
>> diff --git a/gpg-interface.c b/gpg-interface.c
>> @@ -509,7 +509,10 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc,
>> -                       trust_size = strcspn(line, "\n");
>> +                       trust_size = strcspn(line, "\n"); /* truncate at LF */
>> +                       if (trust_size && trust_size != strlen(line) &&
>> +                           line[trust_size - 1] == '\r')
>> +                               trust_size--; /* the LF was part of CRLF at the end */
>
> I may be misunderstanding, but isn't the strlen() unnecessary?
>
>     if (trust_size && line[trust_size] &&
>         line[trust_size - 1] == '\r')
>             trust_size--;

That changes behaviour when "line" has more than one lines in it.
strcspn() finds the first LF, and the posted patch ignores CRLF not
at the end of line[].  Your variant feels more correct if the
objective is to find the end of the first line (regardless of the
choice of the end-of-line convention, either LF or CRLF) and omit
the line terminator.