On 09.12.2021 17:58, Pedro Martelletto wrote:
On Thu, Dec 9, 2021 at 5:33 PM Fabian Stelzer <fs@xxxxxxxxxxxx> wrote:
On 06.12.2021 10:06, Pedro Martelletto wrote:
>On Sun, Dec 5, 2021 at 6:50 AM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
>> So instead of the posted patch, we should do something along this
>> line instead?
>>
>> trust_size = strcspn(line, "\n"); /* truncate at LF */
>> if (trust_size && line[trust_size - 1] == '\r')
>> trust_size--; /* the LF was part of CRLF at the end */
>>
>>
>>
>I agree that's a more consistent fix. A minor nit: if the intention is to
>only trim CR as part of a CRLF sequence, we need to ensure a LF is found:
>
This shouldn't be necessary as we split/loop by LF just above.
for (line = ssh_principals_out.buf; *line;
line = strchrnul(line + 1, '\n')) {
while (*line == '\n')
line++;
if (!*line)
break;
trust_size = strcspn(line, "\n");
principal = xmemdupz(line, trust_size);
The loop ensures that 'line' points to the first character of
ssh_principals_out.buf or to a non-NUL character after a '\n'. It does not
ensure that that 'line' contains a '\n', e.g:
"principalA\nprincipalB\nprincipalC\r" or just "principalA\r".
Oops, yep. You are of course right.
I still dislike how we have to consider this in various places and i guess
there might be more bugs hidden on the windows platform with things like
this. I kinda wish we could strip \r\n -> \n within pipe_command and then
have the rest of the code not have to deal with it :/
Especially since writing a test for this would involve mirroring at leas
parts oft the ssh-keygen api.
But that would be a much bigger/riskier change so for this i think your
latest version is fine.
-
Fabian
