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

[Fabian Stelzer <fs@gigacodes.de>]

On 09.01.2022 16:37, Eric Sunshine wrote:
>On Fri, Jan 07, 2022 at 10:07:35AM +0100, Fabian Stelzer 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>
>
>Should this also have a "Helped-by: Junio" since this code was heavily
>inspired by his suggestion[1]?

Yeah, this should have a "Written-by: Junio" ^^
I'm never sure when to add these headers (except the signed-off).

>
>[1]: https://lore.kernel.org/git/xmqqo84rcn3j.fsf@gitster.g/
>
>> ---
>> 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.

I agree that this is the most readable of the variants (and it works just as 
well). Since in most cases there even will only ever be a single line of 
output the extra work/allocation we might be doing with it is quite minimal.

I have done something quite similar in get_default_ssh_signing_key() and got 
a bit of negative feedback for it (being overkill for retrieving a single 
line) but ended up using it anyway.