From: Fabian Stelzer Date: Thu, 09 Dec 2021 17:20:32 GMT Subject: Re: [PATCH] gpg-interface: trim CR from ssh-keygen -Y find-principals Message-ID: <20211209172032.2iyda3rv4zsjry3s@fs> In-Reply-To: On 09.12.2021 17:58, Pedro Martelletto wrote: >On Thu, Dec 9, 2021 at 5:33 PM Fabian Stelzer wrote: > >> On 06.12.2021 10:06, Pedro Martelletto wrote: >> >On Sun, Dec 5, 2021 at 6:50 AM Junio C Hamano 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