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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jan 9, 2022, 21:37 UTC
Message-ID: <YdtVrT4gBvnXfNr6@flurp.local>
In-Reply-To: <20220107090735.580225-1-fs@gigacodes.de>

On Fri, Jan 07, 2022 at 10:07:35AM +0100, Fabian Stelzer wrote:

Show 7 quoted lines

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

Should this also have a "Helped-by: Junio" since this code was heavily inspired by his suggestion[1]?

[1]: https://lore.kernel.org/git/xmqqo84rcn3j.fsf@gitster.g/

Show 27 quoted lines

> ---
> diff --git a/gpg-interface.c b/gpg-interface.c
> @@ -502,15 +501,30 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc,
> +		const char *next;
> +		for (line = ssh_principals_out.buf;
> +		     *line;
> +		     line = next) {
> +			const char *end_of_text;
> +
> +			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--;
> +
> +			/* 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)
> +				continue;
> +
> +			/* We now know we have an non-empty line. Process it */
> +			principal = xmemdupz(line, end_of_text - line);

Considering that this code makes a copy of the line _anyhow_ which it assigns to `principal`, it still seems like it would be simpler and far easier to understand at-a-glance to instead take advantage of one of the existing string-splitting functions. For instance, something like this:

    struct strbuf **line, **to_free;
    line = to_free = strbuf_split(&ssh_principals_out, '\n');
    for (; *line; line++) {
        strbuf_trim_trailing_newline(*line);
        if (!(*line)->len)
            continue;
        principal = (*line)->buf;

keeping in mind that strbuf_trim_trailing_newline() takes care of CR/LF, and with appropriate cleanup at the end of the loop:

        strbuf_list_free(to_free);

(and removal of `FREE_AND_NULL(principal)` which is no longer needed).

Something similar can be done with string_list_split(), as well.