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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jan 5, 2022, 07:09 UTC
Message-ID: <CAPig+cQinNZp_2=eo7nokMCZ9gc-tAKO1V_jejL2Ei9J63tSDQ@mail.gmail.com>
In-Reply-To: <xmqqo84rcn3j.fsf@gitster.g>

On Tue, Jan 4, 2022 at 2:33 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 19 quoted lines

> Fabian Stelzer <fs@gigacodes.de> writes:
> > I guess we need a bit more context for this patch to make sense:
> >
> > for (line = ssh_principals_out.buf; *line;
> >      line = strchrnul(line + 1, '\n')) {
> >       while (*line == '\n')
> >               line++;
> >       if (!*line)
> >               break;
> >
> >       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 */
> >       principal = xmemdupz(line, trust_size);
>
> Ahh, OK.  Sorry for being ultra lazy for not visiting the actual
> source but just responding after reading only somebody else's
> comments.

I'm also guilty of being lazy and not consulting the actual source. Sorry.

Fabian, thanks for all the extra context information.

Show 9 quoted lines

> OK, so I was completely missing the idea.  And I agree that it may
> be a good idea to check how strcspn() returned to deal with an
> incomplete line, although as you hint later in the message I am
> responding to, checking line[trust_size] would be a more obvious
> implementation.
>
> In any case, I think the earlier part of the loop is more confusing,
> and I think fixing that would naturally fix the trust_size
> computation.  For example, wouldn't this easier to grok?

Indeed, the existing code is confusing me. I've been staring at it for several minutes and I think I'm still failing to understand the purpose of the +1 in the strchrnul() call. Perhaps I'm missing something obvious(?).

Show 15 quoted lines

>         const char *next;
>
>         for (line = ssh_principals_out.buf;
>              *line;
>              line = next) {
>                 const char *end_of_text;
>
>                 /* Find the terminating LF */
>                 next = end_of_text = strchrnul(line, '\n');
>
>                 /* Did we find a LF, and did we have CR before it? */
>                 if (*end_of_text &&
>                     line < end_of_text &&
>                     end_of_text[-1] == '\r')
>                         end_of_text--;

It took several seconds for me to convince myself that the -1 array index was safe. Had the `line < end_of_text` condition been written `end_of_text > line`, I think it would have been immediately obvious, but it's subjective, of course.

Show 21 quoted lines

>                 /* Unless we hit NUL, skip over the LF we found */
>                 if (*next)
>                         next++;
>
>                 /* Not all lines are data.  Skip empty ones */
>                 if (line == end_of_text)
>                         /*
>                          * You may want to allow skipping more than just
>                          * lines with 0-byte on them (e.g. comments?)
>                          * depending on the format you are reading.
>                          */
>                         continue;
>
>                 /* We now know we have an non-empty line. Process it */
>                 principal = xmemdupz(line, end_of_text - line);
>                 ...
>         }
>
> The idea is to make sure that the place where the line ending
> convention is taken care of is very isolated at the beginning of the
> loop.

Yes, this may be an improvement, though the cognitive load is still somewhat high. Using one of the `split` functions from strbuf.h or string-list.h might reduce the cognitive load significantly, even if this code still needs to handle CR removal manually since none of the `split` functions are LF/CRLF agnostic. (Adding such a function might be useful but could be outside the scope of this bug fix patch.)