git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH] gpg-interface: trim CR from ssh-keygen -Y find-principals

From
Fabian Stelzer <fs@gigacodes.de>
Date
Dec 9, 2021, 17:20 UTC
Message-ID
<20211209172032.2iyda3rv4zsjry3s@fs>
In-Reply-To
<CABPYr=xfotWvTQK9k1eKHa0kP4SsB=TKKuM0d8cpMb5BtuUZLA@mail.gmail.com>
On 09.12.2021 17:58, Pedro Martelletto wrote:
Show 36 quoted lines
>On Thu, Dec 9, 2021 at 5:33 PM Fabian Stelzer <fs@gigacodes.de> wrote:
>
>> On 06.12.2021 10:06, Pedro Martelletto wrote:
>> >On Sun, Dec 5, 2021 at 6:50 AM Junio C Hamano <gitster@pobox.com> 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

Previous: Fabian StelzerNext: Fabian Stelzer
Message 29 of 30 in “gpg-interface: trim CR from ssh-keygen -Y find-principals”
  1. gpg-interface: trim CR from ssh-keygen -Y find-principalsJohannes Schindelin via GitGitGadget, Dec 3, 2021
  2. Fabian StelzerDec 3, 2021
  3. Jeff KingDec 3, 2021
  4. Fabian StelzerDec 4, 2021
  5. Junio C HamanoDec 5, 2021
  6. Damien MillerDec 5, 2021
  7. Fabian StelzerDec 6, 2021
  8. gpg-interface: trim CR from ssh-keygenFabian Stelzer, Jan 3, 2022
  9. Eric SunshineJan 3, 2022
  10. Junio C HamanoJan 3, 2022
  11. Eric SunshineJan 4, 2022
  12. Junio C HamanoJan 4, 2022
  13. Eric SunshineJan 4, 2022
  14. Fabian StelzerJan 4, 2022
  15. Junio C HamanoJan 4, 2022
  16. Eric SunshineJan 5, 2022
  17. Fabian StelzerJan 5, 2022
  18. Junio C HamanoJan 5, 2022
  19. Fabian StelzerJan 6, 2022
  20. Junio C HamanoJan 6, 2022
  21. Eric SunshineJan 9, 2022
  22. Fabian StelzerJan 10, 2022
  23. gpg-interface: trim CR from ssh-keygenFabian Stelzer, Jan 7, 2022
  24. Eric SunshineJan 9, 2022
  25. Fabian StelzerJan 10, 2022
  26. Junio C HamanoJan 10, 2022
  27. Junio C HamanoJan 10, 2022
  28. Fabian StelzerDec 9, 2021
  29. Fabian StelzerDec 9, 2021
  30. Fabian StelzerDec 30, 2021

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.