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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jan 4, 2022, 00:41 UTC
Message-ID: <CAPig+cTM3wZz4NXjxYeBuFv0CVNS-T+pBFeVkfMQ-25pL1kBzw@mail.gmail.com>
In-Reply-To: <xmqqee5oieb2.fsf@gitster.g>

On Mon, Jan 3, 2022 at 6:34 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 25 quoted lines

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

Okay, that makes sense if that's the intention of the patch. Perhaps the commit message should mention that `line` might contain multiple lines and that it's only interested in the very last LF (unless it's already obvious to everyone else, even though it wasn't to me). I think it can still be done without strlen(), but it gets uglier and less obvious[*], so strlen() is probably the way to go, and I presume this isn't a hot path, so no big reason to avoid strlen().

[*] Like this, for instance, which is safe because there must be at least one character after the '\n' since this is a NUL-terminated string:

    if (trust_size && line[trust_size] == '\n'
        line[true_size + 1] == '\0' &&
        line[trust_size - 1] == '\r')