* [PATCH] gpg-interface: trim CR from ssh-keygen -Y find-principals @ 2021-12-03 13:31 Johannes Schindelin via GitGitGadget 2021-12-03 14:18 ` Fabian Stelzer ` (2 more replies) 0 siblings, 3 replies; 30+ messages in thread From: Johannes Schindelin via GitGitGadget @ 2021-12-03 13:31 UTC (permalink / raw) To: git; +Cc: Johannes Schindelin, pedro martelletto From: pedro martelletto <pedro@yubico.com> 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. Signed-off-by: pedro martelletto <pedro@yubico.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> --- Allow for CR in the output of ssh-keygen This came in via https://github.com/git-for-windows/git/pull/3561. It affects current Windows versions of OpenSSH (but apparently not the MSYS2 version included in Git for Windows). Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1090%2Fdscho%2Fallow-cr-from-ssh-keygen-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1090/dscho/allow-cr-from-ssh-keygen-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/1090 gpg-interface.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gpg-interface.c b/gpg-interface.c index 3e7255a2a91..85e26882782 100644 --- a/gpg-interface.c +++ b/gpg-interface.c @@ -497,7 +497,7 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc, if (!*line) break; - trust_size = strcspn(line, "\n"); + trust_size = strcspn(line, "\r\n"); principal = xmemdupz(line, trust_size); child_process_init(&ssh_keygen); base-commit: abe6bb3905392d5eb6b01fa6e54d7e784e0522aa -- gitgitgadget ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH] gpg-interface: trim CR from ssh-keygen -Y find-principals 2021-12-03 13:31 [PATCH] gpg-interface: trim CR from ssh-keygen -Y find-principals Johannes Schindelin via GitGitGadget @ 2021-12-03 14:18 ` Fabian Stelzer 2021-12-03 15:58 ` Jeff King 2022-01-03 9:53 ` [PATCH v2] gpg-interface: trim CR from ssh-keygen Fabian Stelzer 2 siblings, 0 replies; 30+ messages in thread From: Fabian Stelzer @ 2021-12-03 14:18 UTC (permalink / raw) To: Johannes Schindelin via GitGitGadget Cc: git, Johannes Schindelin, pedro martelletto On 03.12.2021 13:31, Johannes Schindelin via GitGitGadget wrote: >From: pedro martelletto <pedro@yubico.com> > >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. This fix is obviously fine. But I'm a unsure if this is the only place where we would need to account for windows line endings. There are at least two similar uses in gpg-interface.c. parse_ssh_output() might include a trailing \r in the fingerprint. So I think we should do the same thing here: diff --git a/gpg-interface.c b/gpg-interface.c index 330cfc5845..92cd0f0ebd 100644 --- a/gpg-interface.c +++ b/gpg-interface.c @@ -383,7 +383,7 @@ static void parse_ssh_output(struct signature_check *sigc) sigc->result = 'B'; sigc->trust_level = TRUST_NEVER; - line = to_free = xmemdupz(sigc->output, strcspn(sigc->output, "\n")); + line = to_free = xmemdupz(sigc->output, strcspn(sigc->output, "\r\n")); if (skip_prefix(line, "Good \"git\" signature for ", &line)) { /* Search for the last "with" to get the full principal */ get_default_ssh_signing_key() also splits the defaultKeyCommand output by \n but only puts the result into a file for ssh to use which should be able to deal with it. However the whole parse_gpg_output() also assumes "\n" everywhere. So either GPG behaves differently under windows than ssh or a similar bug could be in there (and if, then probably is for a long time). I'm not familiar with the windows details (like what MSYS2 is / whats different here) and don't really have the means to test it. > >Signed-off-by: pedro martelletto <pedro@yubico.com> >Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> >--- > Allow for CR in the output of ssh-keygen > > This came in via https://github.com/git-for-windows/git/pull/3561. It > affects current Windows versions of OpenSSH (but apparently not the > MSYS2 version included in Git for Windows). > >Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1090%2Fdscho%2Fallow-cr-from-ssh-keygen-v1 >Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1090/dscho/allow-cr-from-ssh-keygen-v1 >Pull-Request: https://github.com/gitgitgadget/git/pull/1090 > > gpg-interface.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > >diff --git a/gpg-interface.c b/gpg-interface.c >index 3e7255a2a91..85e26882782 100644 >--- a/gpg-interface.c >+++ b/gpg-interface.c >@@ -497,7 +497,7 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc, > if (!*line) > break; > >- trust_size = strcspn(line, "\n"); >+ trust_size = strcspn(line, "\r\n"); > principal = xmemdupz(line, trust_size); > > child_process_init(&ssh_keygen); > >base-commit: abe6bb3905392d5eb6b01fa6e54d7e784e0522aa >-- >gitgitgadget ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH] gpg-interface: trim CR from ssh-keygen -Y find-principals 2021-12-03 13:31 [PATCH] gpg-interface: trim CR from ssh-keygen -Y find-principals Johannes Schindelin via GitGitGadget 2021-12-03 14:18 ` Fabian Stelzer @ 2021-12-03 15:58 ` Jeff King 2021-12-04 13:11 ` Fabian Stelzer 2022-01-03 9:53 ` [PATCH v2] gpg-interface: trim CR from ssh-keygen Fabian Stelzer 2 siblings, 1 reply; 30+ messages in thread From: Jeff King @ 2021-12-03 15:58 UTC (permalink / raw) To: Johannes Schindelin via GitGitGadget Cc: Fabian Stelzer, git, Johannes Schindelin, pedro martelletto On Fri, Dec 03, 2021 at 01:31:16PM +0000, Johannes Schindelin via GitGitGadget 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. > [...] > @@ -497,7 +497,7 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc, > if (!*line) > break; > > - trust_size = strcspn(line, "\n"); > + trust_size = strcspn(line, "\r\n"); > principal = xmemdupz(line, trust_size); Just playing devil's advocate for a moment: this parsing is kind of loose. Is there any chance that I could smuggle a CR into my principal name, and make "a principal\rthat is fake" now get parsed as "a principal"? Our strcspn() here would cut off at the first CR. I'm guessing probably not, but when it comes to something with security implications like this, it pays to be extra careful. I'm hoping somebody familiar with the ssh-keygen side and how the rest of the parsing works (like Fabian) can verify that this is OK. -Peff ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] gpg-interface: trim CR from ssh-keygen -Y find-principals 2021-12-03 15:58 ` Jeff King @ 2021-12-04 13:11 ` Fabian Stelzer 2021-12-05 5:50 ` Junio C Hamano 2021-12-05 23:06 ` Damien Miller 0 siblings, 2 replies; 30+ messages in thread From: Fabian Stelzer @ 2021-12-04 13:11 UTC (permalink / raw) To: Jeff King Cc: Johannes Schindelin via GitGitGadget, git, Johannes Schindelin, pedro martelletto, Damien Miller On 03.12.2021 10:58, Jeff King wrote: >On Fri, Dec 03, 2021 at 01:31:16PM +0000, Johannes Schindelin via GitGitGadget 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. >> [...] >> @@ -497,7 +497,7 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc, >> if (!*line) >> break; >> >> - trust_size = strcspn(line, "\n"); >> + trust_size = strcspn(line, "\r\n"); >> principal = xmemdupz(line, trust_size); > >Just playing devil's advocate for a moment: this parsing is kind of >loose. Is there any chance that I could smuggle a CR into my principal >name, and make "a principal\rthat is fake" now get parsed as "a >principal"? Our strcspn() here would cut off at the first CR. > >I'm guessing probably not, but when it comes to something with security >implications like this, it pays to be extra careful. I'm hoping somebody >familiar with the ssh-keygen side and how the rest of the parsing works >(like Fabian) can verify that this is OK. > A good point. I just tested this and CR is a valid character to use in a principal name in the allowed signers file and as of now the principal will be passed to the verify call `as is` and everything works just fine. When we introduce the patch above a principal with a CR in it will fail to verify. I've added Damien Miller to this thread. He knows more about what the expected behaviour for the principal would/should be. I think at the moment almost everything except \n or \0 goes. Maybe restricting \r as well would make life easier for other uses too? From a security perspective I don't think this is problem. The principal does not come from any user input but is actually looked up in the allowed signers file using the signatures public key (with ssh-keygen -Y find-principals). If I could manipulate this file I could change the key as well. If we add `trust on first use` in a future series I would assume we use the email address from the commit/tag author ident when adding a new principal to the file. Can the ident contain a CR? Even if it did, I would only allow a list of allowed alphanumeric chars to be added anyway since a principal can contain wildcards which we obviously don't want to trust on first use ;). Thanks ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] gpg-interface: trim CR from ssh-keygen -Y find-principals 2021-12-04 13:11 ` Fabian Stelzer @ 2021-12-05 5:50 ` Junio C Hamano [not found] ` <CABPYr=y+sDDko9zPxQTOM6Tz4E7CafH7hJc6oB1zv7XYA9KH1A@mail.gmail.com> 2021-12-05 23:06 ` Damien Miller 1 sibling, 1 reply; 30+ messages in thread From: Junio C Hamano @ 2021-12-05 5:50 UTC (permalink / raw) To: Fabian Stelzer Cc: Jeff King, Johannes Schindelin via GitGitGadget, git, Johannes Schindelin, pedro martelletto, Damien Miller Fabian Stelzer <fs@gigacodes.de> writes: > On 03.12.2021 10:58, Jeff King wrote: >>On Fri, Dec 03, 2021 at 01:31:16PM +0000, Johannes Schindelin via GitGitGadget 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. >>> [...] >>> @@ -497,7 +497,7 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc, >>> if (!*line) >>> break; >>> >>> - trust_size = strcspn(line, "\n"); >>> + trust_size = strcspn(line, "\r\n"); >>> principal = xmemdupz(line, trust_size); >> >>Just playing devil's advocate for a moment: this parsing is kind of >>loose. Is there any chance that I could smuggle a CR into my principal >>name, and make "a principal\rthat is fake" now get parsed as "a >>principal"? Our strcspn() here would cut off at the first CR. >> >>I'm guessing probably not, but when it comes to something with security >>implications like this, it pays to be extra careful. I'm hoping somebody >>familiar with the ssh-keygen side and how the rest of the parsing works >>(like Fabian) can verify that this is OK. >> > > A good point. I just tested this and CR is a valid character to use in > a principal name in the allowed signers file and as of now the > principal will be passed to the verify call `as is` and everything > works just fine. When we introduce the patch above a principal with a > CR in it will fail to verify. > > I've added Damien Miller to this thread. He knows more about what the > expected behaviour for the principal would/should be. I think at the > moment almost everything except \n or \0 goes. Maybe restricting \r as > well would make life easier for other uses too? > > From a security perspective I don't think this is problem. The > principal does not come from any user input but is actually looked up > in the allowed signers file using the signatures public key (with > ssh-keygen -Y find-principals). If I could manipulate this file I > could change the key as well. > > If we add `trust on first use` in a future series I would assume we > use the email address from the commit/tag author ident when adding a > new principal to the file. Can the ident contain a CR? > Even if it did, I would only allow a list of allowed alphanumeric > chars to be added anyway since a principal can contain wildcards which > we obviously don't want to trust on first use ;). 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 */ ^ permalink raw reply [flat|nested] 30+ messages in thread
[parent not found: <CABPYr=y+sDDko9zPxQTOM6Tz4E7CafH7hJc6oB1zv7XYA9KH1A@mail.gmail.com>]
* Re: [PATCH] gpg-interface: trim CR from ssh-keygen -Y find-principals [not found] ` <CABPYr=y+sDDko9zPxQTOM6Tz4E7CafH7hJc6oB1zv7XYA9KH1A@mail.gmail.com> @ 2021-12-09 16:33 ` Fabian Stelzer [not found] ` <CABPYr=xfotWvTQK9k1eKHa0kP4SsB=TKKuM0d8cpMb5BtuUZLA@mail.gmail.com> 0 siblings, 1 reply; 30+ messages in thread From: Fabian Stelzer @ 2021-12-09 16:33 UTC (permalink / raw) To: Pedro Martelletto Cc: Junio C Hamano, Jeff King, Johannes Schindelin via GitGitGadget, git, Johannes Schindelin, Damien Miller 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); - Fabian >trust_size = strcspn(line, "\n"); /* truncate at LF */ >if (trust_size && trust_size != strlen(line) && line[trust_size - 1] == >'\r') > trust_size--; /* the LF was part of CRLF at the end */ > >-p. ^ permalink raw reply [flat|nested] 30+ messages in thread
[parent not found: <CABPYr=xfotWvTQK9k1eKHa0kP4SsB=TKKuM0d8cpMb5BtuUZLA@mail.gmail.com>]
* Re: [PATCH] gpg-interface: trim CR from ssh-keygen -Y find-principals [not found] ` <CABPYr=xfotWvTQK9k1eKHa0kP4SsB=TKKuM0d8cpMb5BtuUZLA@mail.gmail.com> @ 2021-12-09 17:20 ` Fabian Stelzer 2021-12-30 10:25 ` Fabian Stelzer 1 sibling, 0 replies; 30+ messages in thread From: Fabian Stelzer @ 2021-12-09 17:20 UTC (permalink / raw) To: Pedro Martelletto Cc: Junio C Hamano, Jeff King, Johannes Schindelin via GitGitGadget, git, Johannes Schindelin, Damien Miller On 09.12.2021 17:58, Pedro Martelletto wrote: >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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] gpg-interface: trim CR from ssh-keygen -Y find-principals [not found] ` <CABPYr=xfotWvTQK9k1eKHa0kP4SsB=TKKuM0d8cpMb5BtuUZLA@mail.gmail.com> 2021-12-09 17:20 ` Fabian Stelzer @ 2021-12-30 10:25 ` Fabian Stelzer 1 sibling, 0 replies; 30+ messages in thread From: Fabian Stelzer @ 2021-12-30 10:25 UTC (permalink / raw) To: Pedro Martelletto Cc: Junio C Hamano, Jeff King, Johannes Schindelin via GitGitGadget, git, Johannes Schindelin On 09.12.2021 17:58, Pedro Martelletto wrote: >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". > Just saw that this is still open. @pedro: do you want to send an updated version of your patch or would you like me to pick this up and send one? ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] gpg-interface: trim CR from ssh-keygen -Y find-principals 2021-12-04 13:11 ` Fabian Stelzer 2021-12-05 5:50 ` Junio C Hamano @ 2021-12-05 23:06 ` Damien Miller 2021-12-06 8:39 ` Fabian Stelzer 1 sibling, 1 reply; 30+ messages in thread From: Damien Miller @ 2021-12-05 23:06 UTC (permalink / raw) To: Fabian Stelzer Cc: Jeff King, Johannes Schindelin via GitGitGadget, git, Johannes Schindelin, pedro martelletto On Sat, 4 Dec 2021, Fabian Stelzer wrote: > > I'm guessing probably not, but when it comes to something with security > > implications like this, it pays to be extra careful. I'm hoping somebody > > familiar with the ssh-keygen side and how the rest of the parsing works > > (like Fabian) can verify that this is OK. > > > > A good point. I just tested this and CR is a valid character to use in a > principal name in the allowed signers file and as of now the principal will be > passed to the verify call `as is` and everything works just fine. When we > introduce the patch above a principal with a CR in it will fail to verify. Are you sure? I thought that we split principals in allowed_signers on most whitespace, including \r. Follow: https://github.com/openssh/openssh-portable/blob/e9c7149/sshsig.c#L742 https://github.com/openssh/openssh-portable/blob/e9c7149/misc.c#L452 https://github.com/openssh/openssh-portable/blob/e9c7149/misc.c#L408 > I've added Damien Miller to this thread. He knows more about what the expected > behaviour for the principal would/should be. I think at the moment almost > everything except \n or \0 goes. Maybe restricting \r as well would make life > easier for other uses too? IMO sensible content for the principals section would be printable, non- whitespace characters, excluding wildcards ('*', '?'). ssh-keygen mostly assumes that the file is in good order, but maybe it could be stricter. > If we add `trust on first use` in a future series I would assume we use the > email address from the commit/tag author ident when adding a new principal to > the file. Can the ident contain a CR? > Even if it did, I would only allow a list of allowed alphanumeric chars to be > added anyway since a principal can contain wildcards which we obviously don't > want to trust on first use ;). Yeah. my mental model for the allowed_signers file is that it's similar to ~/.ssh/authorized_keys in that it directly controls authn/authz decisions, and if you put bad stuff in there then you're going to have a bad day... -d ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] gpg-interface: trim CR from ssh-keygen -Y find-principals 2021-12-05 23:06 ` Damien Miller @ 2021-12-06 8:39 ` Fabian Stelzer 0 siblings, 0 replies; 30+ messages in thread From: Fabian Stelzer @ 2021-12-06 8:39 UTC (permalink / raw) To: Damien Miller Cc: Jeff King, Johannes Schindelin via GitGitGadget, git, Johannes Schindelin, pedro martelletto, Junio C Hamano On 06.12.2021 10:06, Damien Miller wrote: >On Sat, 4 Dec 2021, Fabian Stelzer wrote: > >> > I'm guessing probably not, but when it comes to something with security >> > implications like this, it pays to be extra careful. I'm hoping somebody >> > familiar with the ssh-keygen side and how the rest of the parsing works >> > (like Fabian) can verify that this is OK. >> > >> >> A good point. I just tested this and CR is a valid character to use in a >> principal name in the allowed signers file and as of now the principal will be >> passed to the verify call `as is` and everything works just fine. When we >> introduce the patch above a principal with a CR in it will fail to verify. > >Are you sure? I thought that we split principals in allowed_signers on >most whitespace, including \r. Follow: > >https://github.com/openssh/openssh-portable/blob/e9c7149/sshsig.c#L742 >https://github.com/openssh/openssh-portable/blob/e9c7149/misc.c#L452 >https://github.com/openssh/openssh-portable/blob/e9c7149/misc.c#L408 > Sorry, I should have mentioned that I quoted the principal. Within the quotes whitespace (and \r) works. Since find-principals then returns one principal per line the line ending issue can come up. >> I've added Damien Miller to this thread. He knows more about what the expected >> behaviour for the principal would/should be. I think at the moment almost >> everything except \n or \0 goes. Maybe restricting \r as well would make life >> easier for other uses too? > >IMO sensible content for the principals section would be printable, non- >whitespace characters, excluding wildcards ('*', '?'). ssh-keygen mostly >assumes that the file is in good order, but maybe it could be stricter. > Ok, I think we can make sure of that when adding principals and use Junios suggested patch for trimming the \r at the line ending. >> If we add `trust on first use` in a future series I would assume we use the >> email address from the commit/tag author ident when adding a new principal to >> the file. Can the ident contain a CR? >> Even if it did, I would only allow a list of allowed alphanumeric chars to be >> added anyway since a principal can contain wildcards which we obviously don't >> want to trust on first use ;). > >Yeah. my mental model for the allowed_signers file is that it's similar >to ~/.ssh/authorized_keys in that it directly controls authn/authz >decisions, and if you put bad stuff in there then you're going to have >a bad day... > Thanks for your input. ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2] gpg-interface: trim CR from ssh-keygen 2021-12-03 13:31 [PATCH] gpg-interface: trim CR from ssh-keygen -Y find-principals Johannes Schindelin via GitGitGadget 2021-12-03 14:18 ` Fabian Stelzer 2021-12-03 15:58 ` Jeff King @ 2022-01-03 9:53 ` Fabian Stelzer 2022-01-03 17:17 ` Eric Sunshine 2022-01-07 9:07 ` [PATCH v3] " Fabian Stelzer 2 siblings, 2 replies; 30+ messages in thread From: Fabian Stelzer @ 2022-01-03 9:53 UTC (permalink / raw) To: git Cc: Fabian Stelzer, Pedro Martelletto, Junio C Hamano, Jeff King, Johannes Schindelin 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> --- gpg-interface.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/gpg-interface.c b/gpg-interface.c index b52eb0e2e0..d5eca417e8 100644 --- a/gpg-interface.c +++ b/gpg-interface.c @@ -509,7 +509,10 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc, if (!*line) break; - trust_size = strcspn(line, "\n"); + trust_size = strcspn(line, "\n"); /* truncate at LF */ + if (trust_size && trust_size != strlen(line) && + line[trust_size - 1] == '\r') + trust_size--; /* the LF was part of CRLF at the end */ principal = xmemdupz(line, trust_size); child_process_init(&ssh_keygen); -- 2.33.1 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH v2] gpg-interface: trim CR from ssh-keygen 2022-01-03 9:53 ` [PATCH v2] gpg-interface: trim CR from ssh-keygen Fabian Stelzer @ 2022-01-03 17:17 ` Eric Sunshine 2022-01-03 23:34 ` Junio C Hamano 2022-01-07 9:07 ` [PATCH v3] " Fabian Stelzer 1 sibling, 1 reply; 30+ messages in thread From: Eric Sunshine @ 2022-01-03 17:17 UTC (permalink / raw) To: Fabian Stelzer Cc: Git List, Pedro Martelletto, Junio C Hamano, Jeff King, Johannes Schindelin On Mon, Jan 3, 2022 at 9:24 AM Fabian Stelzer <fs@gigacodes.de> 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> > --- > diff --git a/gpg-interface.c b/gpg-interface.c > @@ -509,7 +509,10 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc, > - trust_size = strcspn(line, "\n"); > + trust_size = strcspn(line, "\n"); /* truncate at LF */ > + if (trust_size && trust_size != strlen(line) && > + line[trust_size - 1] == '\r') > + trust_size--; /* the LF was part of CRLF at the end */ I may be misunderstanding, but isn't the strlen() unnecessary? if (trust_size && line[trust_size] && line[trust_size - 1] == '\r') trust_size--; ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2] gpg-interface: trim CR from ssh-keygen 2022-01-03 17:17 ` Eric Sunshine @ 2022-01-03 23:34 ` Junio C Hamano 2022-01-04 0:41 ` Eric Sunshine 0 siblings, 1 reply; 30+ messages in thread From: Junio C Hamano @ 2022-01-03 23:34 UTC (permalink / raw) To: Eric Sunshine Cc: Fabian Stelzer, Git List, Pedro Martelletto, Jeff King, Johannes Schindelin Eric Sunshine <sunshine@sunshineco.com> writes: > On Mon, Jan 3, 2022 at 9:24 AM Fabian Stelzer <fs@gigacodes.de> 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> >> --- >> diff --git a/gpg-interface.c b/gpg-interface.c >> @@ -509,7 +509,10 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc, >> - trust_size = strcspn(line, "\n"); >> + trust_size = strcspn(line, "\n"); /* truncate at LF */ >> + if (trust_size && trust_size != strlen(line) && >> + line[trust_size - 1] == '\r') >> + trust_size--; /* the LF was part of CRLF at the end */ > > I may be misunderstanding, but isn't the strlen() unnecessary? > > if (trust_size && line[trust_size] && > line[trust_size - 1] == '\r') > trust_size--; That changes behaviour when "line" has more than one lines in it. strcspn() finds the first LF, and the posted patch ignores CRLF not at the end of line[]. Your variant feels more correct if the objective is to find the end of the first line (regardless of the choice of the end-of-line convention, either LF or CRLF) and omit the line terminator. ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2] gpg-interface: trim CR from ssh-keygen 2022-01-03 23:34 ` Junio C Hamano @ 2022-01-04 0:41 ` Eric Sunshine 2022-01-04 1:19 ` Junio C Hamano 0 siblings, 1 reply; 30+ messages in thread From: Eric Sunshine @ 2022-01-04 0:41 UTC (permalink / raw) To: Junio C Hamano Cc: Fabian Stelzer, Git List, Pedro Martelletto, Jeff King, Johannes Schindelin On Mon, Jan 3, 2022 at 6:34 PM Junio C Hamano <gitster@pobox.com> wrote: > Eric Sunshine <sunshine@sunshineco.com> writes: > > On Mon, Jan 3, 2022 at 9:24 AM Fabian Stelzer <fs@gigacodes.de> 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. > >> --- > >> - trust_size = strcspn(line, "\n"); > >> + trust_size = strcspn(line, "\n"); /* truncate at LF */ > >> + if (trust_size && trust_size != strlen(line) && > >> + line[trust_size - 1] == '\r') > >> + trust_size--; /* the LF was part of CRLF at the end */ > > > > I may be misunderstanding, but isn't the strlen() unnecessary? > > > > if (trust_size && line[trust_size] && > > line[trust_size - 1] == '\r') > > trust_size--; > > That changes behaviour when "line" has more than one lines in it. > strcspn() finds the first LF, and the posted patch ignores CRLF not > at the end of line[]. Your variant feels more correct if the > objective is to find the end of the first line (regardless of the > choice of the end-of-line convention, either LF or CRLF) and omit > the line terminator. Okay, that makes sense if that's the intention of the patch. Perhaps the commit message should mention that `line` might contain multiple lines and that it's only interested in the very last LF (unless it's already obvious to everyone else, even though it wasn't to me). I think it can still be done without strlen(), but it gets uglier and less obvious[*], so strlen() is probably the way to go, and I presume this isn't a hot path, so no big reason to avoid strlen(). [*] Like this, for instance, which is safe because there must be at least one character after the '\n' since this is a NUL-terminated string: if (trust_size && line[trust_size] == '\n' line[true_size + 1] == '\0' && line[trust_size - 1] == '\r') ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2] gpg-interface: trim CR from ssh-keygen 2022-01-04 0:41 ` Eric Sunshine @ 2022-01-04 1:19 ` Junio C Hamano 2022-01-04 3:06 ` Eric Sunshine 0 siblings, 1 reply; 30+ messages in thread From: Junio C Hamano @ 2022-01-04 1:19 UTC (permalink / raw) To: Eric Sunshine Cc: Fabian Stelzer, Git List, Pedro Martelletto, Jeff King, Johannes Schindelin Eric Sunshine <sunshine@sunshineco.com> writes: > On Mon, Jan 3, 2022 at 6:34 PM Junio C Hamano <gitster@pobox.com> wrote: >> Eric Sunshine <sunshine@sunshineco.com> writes: >> > On Mon, Jan 3, 2022 at 9:24 AM Fabian Stelzer <fs@gigacodes.de> 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. >> >> --- >> >> - trust_size = strcspn(line, "\n"); >> >> + trust_size = strcspn(line, "\n"); /* truncate at LF */ >> >> + if (trust_size && trust_size != strlen(line) && >> >> + line[trust_size - 1] == '\r') >> >> + trust_size--; /* the LF was part of CRLF at the end */ >> > >> > I may be misunderstanding, but isn't the strlen() unnecessary? >> > >> > if (trust_size && line[trust_size] && >> > line[trust_size - 1] == '\r') >> > trust_size--; >> >> That changes behaviour when "line" has more than one lines in it. >> strcspn() finds the first LF, and the posted patch ignores CRLF not >> at the end of line[]. Your variant feels more correct if the >> objective is to find the end of the first line (regardless of the >> choice of the end-of-line convention, either LF or CRLF) and omit >> the line terminator. > > Okay, that makes sense if that's the intention of the patch. Perhaps > the commit message should mention that `line` might contain multiple > lines and that it's only interested in the very last LF (unless it's > already obvious to everyone else, even though it wasn't to me). I do not think that is the case. strcspn(line, "\n") will stop at the first one, so unless it is guaranteed that "line" has only one line in it, the patch as posted is not correct. Your variant without strlen() feels more correct, as I said. ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2] gpg-interface: trim CR from ssh-keygen 2022-01-04 1:19 ` Junio C Hamano @ 2022-01-04 3:06 ` Eric Sunshine 2022-01-04 12:55 ` Fabian Stelzer 0 siblings, 1 reply; 30+ messages in thread From: Eric Sunshine @ 2022-01-04 3:06 UTC (permalink / raw) To: Junio C Hamano Cc: Fabian Stelzer, Git List, Pedro Martelletto, Jeff King, Johannes Schindelin On Mon, Jan 3, 2022 at 8:19 PM Junio C Hamano <gitster@pobox.com> wrote: > Eric Sunshine <sunshine@sunshineco.com> writes: > > On Mon, Jan 3, 2022 at 6:34 PM Junio C Hamano <gitster@pobox.com> wrote: > >> Eric Sunshine <sunshine@sunshineco.com> writes: > >> > On Mon, Jan 3, 2022 at 9:24 AM Fabian Stelzer <fs@gigacodes.de> wrote: > >> >> - trust_size = strcspn(line, "\n"); > >> >> + trust_size = strcspn(line, "\n"); /* truncate at LF */ > >> >> + if (trust_size && trust_size != strlen(line) && > >> >> + line[trust_size - 1] == '\r') > >> >> + trust_size--; /* the LF was part of CRLF at the end */ > >> > > >> > I may be misunderstanding, but isn't the strlen() unnecessary? > >> > > >> > if (trust_size && line[trust_size] && > >> > line[trust_size - 1] == '\r') > >> > trust_size--; > >> > >> That changes behaviour when "line" has more than one lines in it. > >> strcspn() finds the first LF, and the posted patch ignores CRLF not > >> at the end of line[]. Your variant feels more correct if the > >> objective is to find the end of the first line (regardless of the > >> choice of the end-of-line convention, either LF or CRLF) and omit > >> the line terminator. > > > > Okay, that makes sense if that's the intention of the patch. Perhaps > > the commit message should mention that `line` might contain multiple > > lines and that it's only interested in the very last LF (unless it's > > already obvious to everyone else, even though it wasn't to me). > > I do not think that is the case. strcspn(line, "\n") will stop at > the first one, so unless it is guaranteed that "line" has only one > line in it, the patch as posted is not correct. Your variant > without strlen() feels more correct, as I said. Okay, sorry for my unclear thinking. The existing code (before this patch) does indeed seem to be interested only in the first line of `line`, in which case I agree that the patch's use of strlen() does not appear to be correct if `line` could ever contain more than one line. ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2] gpg-interface: trim CR from ssh-keygen 2022-01-04 3:06 ` Eric Sunshine @ 2022-01-04 12:55 ` Fabian Stelzer 2022-01-04 19:33 ` Junio C Hamano 0 siblings, 1 reply; 30+ messages in thread From: Fabian Stelzer @ 2022-01-04 12:55 UTC (permalink / raw) To: Eric Sunshine Cc: Junio C Hamano, Git List, Pedro Martelletto, Jeff King, Johannes Schindelin On 03.01.2022 22:06, Eric Sunshine wrote: >On Mon, Jan 3, 2022 at 8:19 PM Junio C Hamano <gitster@pobox.com> wrote: >> Eric Sunshine <sunshine@sunshineco.com> writes: >> > On Mon, Jan 3, 2022 at 6:34 PM Junio C Hamano <gitster@pobox.com> wrote: >> >> Eric Sunshine <sunshine@sunshineco.com> writes: >> >> > On Mon, Jan 3, 2022 at 9:24 AM Fabian Stelzer <fs@gigacodes.de> wrote: >> >> >> - trust_size = strcspn(line, "\n"); >> >> >> + trust_size = strcspn(line, "\n"); /* truncate at LF */ >> >> >> + if (trust_size && trust_size != strlen(line) && >> >> >> + line[trust_size - 1] == '\r') >> >> >> + trust_size--; /* the LF was part of CRLF at the end */ >> >> > >> >> > I may be misunderstanding, but isn't the strlen() unnecessary? >> >> > >> >> > if (trust_size && line[trust_size] && >> >> > line[trust_size - 1] == '\r') >> >> > trust_size--; >> >> >> >> That changes behaviour when "line" has more than one lines in it. >> >> strcspn() finds the first LF, and the posted patch ignores CRLF not >> >> at the end of line[]. Your variant feels more correct if the >> >> objective is to find the end of the first line (regardless of the >> >> choice of the end-of-line convention, either LF or CRLF) and omit >> >> the line terminator. >> > >> > Okay, that makes sense if that's the intention of the patch. Perhaps >> > the commit message should mention that `line` might contain multiple >> > lines and that it's only interested in the very last LF (unless it's >> > already obvious to everyone else, even though it wasn't to me). >> >> I do not think that is the case. strcspn(line, "\n") will stop at >> the first one, so unless it is guaranteed that "line" has only one >> line in it, the patch as posted is not correct. Your variant >> without strlen() feels more correct, as I said. > >Okay, sorry for my unclear thinking. The existing code (before this >patch) does indeed seem to be interested only in the first line of >`line`, in which case I agree that the patch's use of strlen() does >not appear to be correct if `line` could ever contain more than one >line. I guess we need a bit more context for this patch to make sense: for (line = ssh_principals_out.buf; *line; line = strchrnul(line + 1, '\n')) { while (*line == '\n') line++; if (!*line) break; trust_size = strcspn(line, "\n"); /* truncate at LF */ if (trust_size && trust_size != strlen(line) && line[trust_size - 1] == '\r') trust_size--; /* the LF was part of CRLF at the end */ principal = xmemdupz(line, trust_size); ssh_principals_out contains the result of the find-principals call which contains one found principal per line (normally LF, CRLF in some cygwin setup). A principal can contain CR as a valid character. This is problematic if CR is the last char of the principal since we have no way of knowing then if we are in cygwin with CRLF line endings or another platform using LF and the CR is the last character of the principal. Lets leave this rather weird edge case aside for now. So what we want to do is split the buffer by line, no matter which line endings are used, and copy the principal without any line ending characters. The `trust_size != strlen(line)` check was supposed to guard against `line` having no LF at all and ending with a CR. I think a `line[trust_size + 1] != '\0'` would work as well. But since this whole thing is already hard enough to follow i guess it's better we simply remove it instead of adding checks for the unlikely case we encounter a broken ssh-keygen. Especially since the effect would only be a failed signature validation. We could even remove the `if (trust_size)` condition since this only happens when `line` begins with LF which is already skipped over a few lines before. But it's probably better to leave this in just in case the code changes. Generally I think this is a common enough problem that there should be a function to split a strbuf by line no matter if LF or CRLF is used. Similar to strbuf_getline() but to read from a strbuf or maybe even handle this within pipe_command() when filling the strbuf. Maybe git even has better code to handle this but i haven't found it yet? ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2] gpg-interface: trim CR from ssh-keygen 2022-01-04 12:55 ` Fabian Stelzer @ 2022-01-04 19:33 ` Junio C Hamano 2022-01-05 7:09 ` Eric Sunshine 0 siblings, 1 reply; 30+ messages in thread From: Junio C Hamano @ 2022-01-04 19:33 UTC (permalink / raw) To: Fabian Stelzer Cc: Eric Sunshine, Git List, Pedro Martelletto, Jeff King, Johannes Schindelin Fabian Stelzer <fs@gigacodes.de> writes: > I guess we need a bit more context for this patch to make sense: > > for (line = ssh_principals_out.buf; *line; > line = strchrnul(line + 1, '\n')) { > while (*line == '\n') > line++; > if (!*line) > break; > > trust_size = strcspn(line, "\n"); /* truncate at LF */ > if (trust_size && trust_size != strlen(line) && > line[trust_size - 1] == '\r') > trust_size--; /* the LF was part of CRLF at the end */ > principal = xmemdupz(line, trust_size); > > ssh_principals_out contains the result of the find-principals call > which contains one found principal per line (normally LF, CRLF in some > cygwin setup). Ahh, OK. Sorry for being ultra lazy for not visiting the actual source but just responding after reading only somebody else's comments. So, the code skips over one or more LFs (but users of platforms that use CRLF line termination are screwed here already) to find the beginning of a non-empty line. Then it wants to find the end of that non-empty line (if there is still LF there in the buffer). Since strcspn() may not find any LF (i.e. it is an incomplete line), strlen(line) is used to see if we found a LF or if we hit the terminating NUL. If the line ended with CR, we do not want to strip it. OK, so I was completely missing the idea. And I agree that it may be a good idea to check how strcspn() returned to deal with an incomplete line, although as you hint later in the message I am responding to, checking line[trust_size] would be a more obvious implementation. In any case, I think the earlier part of the loop is more confusing, and I think fixing that would naturally fix the trust_size computation. For example, wouldn't this easier to grok? const char *next; for (line = ssh_principals_out.buf; *line; line = next) { const char *end_of_text; /* Find the terminating LF */ 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) /* * You may want to allow skipping more than just * lines with 0-byte on them (e.g. comments?) * depending on the format you are reading. */ continue; /* We now know we have an non-empty line. Process it */ principal = xmemdupz(line, end_of_text - line); ... } The idea is to make sure that the place where the line ending convention is taken care of is very isolated at the beginning of the loop. Hmm? ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2] gpg-interface: trim CR from ssh-keygen 2022-01-04 19:33 ` Junio C Hamano @ 2022-01-05 7:09 ` Eric Sunshine 2022-01-05 10:36 ` Fabian Stelzer 0 siblings, 1 reply; 30+ messages in thread From: Eric Sunshine @ 2022-01-05 7:09 UTC (permalink / raw) To: Junio C Hamano Cc: Fabian Stelzer, Git List, Pedro Martelletto, Jeff King, Johannes Schindelin On Tue, Jan 4, 2022 at 2:33 PM Junio C Hamano <gitster@pobox.com> wrote: > Fabian Stelzer <fs@gigacodes.de> writes: > > I guess we need a bit more context for this patch to make sense: > > > > for (line = ssh_principals_out.buf; *line; > > line = strchrnul(line + 1, '\n')) { > > while (*line == '\n') > > line++; > > if (!*line) > > break; > > > > trust_size = strcspn(line, "\n"); /* truncate at LF */ > > if (trust_size && trust_size != strlen(line) && > > line[trust_size - 1] == '\r') > > trust_size--; /* the LF was part of CRLF at the end */ > > principal = xmemdupz(line, trust_size); > > Ahh, OK. Sorry for being ultra lazy for not visiting the actual > source but just responding after reading only somebody else's > comments. I'm also guilty of being lazy and not consulting the actual source. Sorry. Fabian, thanks for all the extra context information. > OK, so I was completely missing the idea. And I agree that it may > be a good idea to check how strcspn() returned to deal with an > incomplete line, although as you hint later in the message I am > responding to, checking line[trust_size] would be a more obvious > implementation. > > In any case, I think the earlier part of the loop is more confusing, > and I think fixing that would naturally fix the trust_size > computation. For example, wouldn't this easier to grok? Indeed, the existing code is confusing me. I've been staring at it for several minutes and I think I'm still failing to understand the purpose of the +1 in the strchrnul() call. Perhaps I'm missing something obvious(?). > const char *next; > > for (line = ssh_principals_out.buf; > *line; > line = next) { > const char *end_of_text; > > /* Find the terminating LF */ > 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--; It took several seconds for me to convince myself that the -1 array index was safe. Had the `line < end_of_text` condition been written `end_of_text > line`, I think it would have been immediately obvious, but it's subjective, of course. > /* 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) > /* > * You may want to allow skipping more than just > * lines with 0-byte on them (e.g. comments?) > * depending on the format you are reading. > */ > continue; > > /* We now know we have an non-empty line. Process it */ > principal = xmemdupz(line, end_of_text - line); > ... > } > > The idea is to make sure that the place where the line ending > convention is taken care of is very isolated at the beginning of the > loop. Yes, this may be an improvement, though the cognitive load is still somewhat high. Using one of the `split` functions from strbuf.h or string-list.h might reduce the cognitive load significantly, even if this code still needs to handle CR removal manually since none of the `split` functions are LF/CRLF agnostic. (Adding such a function might be useful but could be outside the scope of this bug fix patch.) ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2] gpg-interface: trim CR from ssh-keygen 2022-01-05 7:09 ` Eric Sunshine @ 2022-01-05 10:36 ` Fabian Stelzer 2022-01-05 20:40 ` Junio C Hamano 2022-01-09 20:49 ` Eric Sunshine 0 siblings, 2 replies; 30+ messages in thread From: Fabian Stelzer @ 2022-01-05 10:36 UTC (permalink / raw) To: Eric Sunshine Cc: Junio C Hamano, Git List, Pedro Martelletto, Jeff King, Johannes Schindelin On 05.01.2022 02:09, Eric Sunshine wrote: >On Tue, Jan 4, 2022 at 2:33 PM Junio C Hamano <gitster@pobox.com> wrote: >> Fabian Stelzer <fs@gigacodes.de> writes: >> > I guess we need a bit more context for this patch to make sense: >> > >> > for (line = ssh_principals_out.buf; *line; >> > line = strchrnul(line + 1, '\n')) { >> > while (*line == '\n') >> > line++; >> > if (!*line) >> > break; >> > >> > trust_size = strcspn(line, "\n"); /* truncate at LF */ >> > if (trust_size && trust_size != strlen(line) && >> > line[trust_size - 1] == '\r') >> > trust_size--; /* the LF was part of CRLF at the end */ >> > principal = xmemdupz(line, trust_size); >> >> Ahh, OK. Sorry for being ultra lazy for not visiting the actual >> source but just responding after reading only somebody else's >> comments. > >I'm also guilty of being lazy and not consulting the actual source. Sorry. > >Fabian, thanks for all the extra context information. > >> OK, so I was completely missing the idea. And I agree that it may >> be a good idea to check how strcspn() returned to deal with an >> incomplete line, although as you hint later in the message I am >> responding to, checking line[trust_size] would be a more obvious >> implementation. >> >> In any case, I think the earlier part of the loop is more confusing, >> and I think fixing that would naturally fix the trust_size >> computation. For example, wouldn't this easier to grok? > >Indeed, the existing code is confusing me. I've been staring at it for >several minutes and I think I'm still failing to understand the >purpose of the +1 in the strchrnul() call. Perhaps I'm missing >something obvious(?). This whole loop was basically copied from parse_gpg_output() above. Without the +1 this would always find the same line in the buffer. The +1 skips over the previously found LF. > >> const char *next; >> >> for (line = ssh_principals_out.buf; >> *line; >> line = next) { >> const char *end_of_text; >> >> /* Find the terminating LF */ >> 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--; > >It took several seconds for me to convince myself that the -1 array >index was safe. Had the `line < end_of_text` condition been written >`end_of_text > line`, I think it would have been immediately obvious, >but it's subjective, of course. > >> /* 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) >> /* >> * You may want to allow skipping more than just >> * lines with 0-byte on them (e.g. comments?) >> * depending on the format you are reading. >> */ >> continue; >> >> /* We now know we have an non-empty line. Process it */ >> principal = xmemdupz(line, end_of_text - line); >> ... >> } >> >> The idea is to make sure that the place where the line ending >> convention is taken care of is very isolated at the beginning of the >> loop. > >Yes, this may be an improvement, though the cognitive load is still >somewhat high. Using one of the `split` functions from strbuf.h or >string-list.h might reduce the cognitive load significantly, even if >this code still needs to handle CR removal manually since none of the >`split` functions are LF/CRLF agnostic. (Adding such a function might >be useful but could be outside the scope of this bug fix patch.) How about something like this: int string_find_line(char **line, size_t *len) { const char *eol = NULL; if (*len > 0) { *line = *line + *len; if (**line && **line == '\r') (*line)++; if (**line && **line == '\n') (*line)++; } if (!**line) return 0; eol = strchrnul(*line, '\n'); /* Trim trailing CR from length */ if (eol > *line && eol[-1] == '\r') eol--; *len = eol - *line; return 1; } Its use would then simply be: char *line = strbuf.buf; size_t len = 0; while(string_find_line(&line,&len)) { if (!len) continue; /* Skip over empty lines */ principal = xmemdupz(line, len); } Not sure about the name though. Maybe string_find_line() / _iterate_line / foreach_line ? ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2] gpg-interface: trim CR from ssh-keygen 2022-01-05 10:36 ` Fabian Stelzer @ 2022-01-05 20:40 ` Junio C Hamano 2022-01-06 10:26 ` Fabian Stelzer 2022-01-09 20:49 ` Eric Sunshine 1 sibling, 1 reply; 30+ messages in thread From: Junio C Hamano @ 2022-01-05 20:40 UTC (permalink / raw) To: Fabian Stelzer Cc: Eric Sunshine, Git List, Pedro Martelletto, Jeff King, Johannes Schindelin Fabian Stelzer <fs@gigacodes.de> writes: > How about something like this: > > int string_find_line(char **line, size_t *len) { > const char *eol = NULL; > > if (*len > 0) { > *line = *line + *len; > if (**line && **line == '\r') > (*line)++; > if (**line && **line == '\n') > (*line)++; > } > > if (!**line) > return 0; > > eol = strchrnul(*line, '\n'); > > /* Trim trailing CR from length */ > if (eol > *line && eol[-1] == '\r') > eol--; > > *len = eol - *line; > return 1; > } It is a confusing piece of "we handle one line at a time" helper. It is not obvious what the loop invariants are. It would be most natural to readers if *line points at the very beginning of the buffer, i.e. the beginning of the first line, and *len points at the very first character of that line, i.e. 0. But then the first thing this function worries about is a case where *len is not 0. I obviously am biased, but sorry, I find what I gave you 100 times simpler to understand. > > Its use would then simply be: > > char *line = strbuf.buf; > size_t len = 0; > while(string_find_line(&line,&len)) { > if (!len) > continue; /* Skip over empty lines */ > principal = xmemdupz(line, len); > } > > Not sure about the name though. > Maybe string_find_line() / _iterate_line / foreach_line ? ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2] gpg-interface: trim CR from ssh-keygen 2022-01-05 20:40 ` Junio C Hamano @ 2022-01-06 10:26 ` Fabian Stelzer 2022-01-06 17:50 ` Junio C Hamano 0 siblings, 1 reply; 30+ messages in thread From: Fabian Stelzer @ 2022-01-06 10:26 UTC (permalink / raw) To: Junio C Hamano Cc: Eric Sunshine, Git List, Pedro Martelletto, Jeff King, Johannes Schindelin On 05.01.2022 12:40, Junio C Hamano wrote: >Fabian Stelzer <fs@gigacodes.de> writes: > >> How about something like this: >> >> int string_find_line(char **line, size_t *len) { >> const char *eol = NULL; >> >> if (*len > 0) { >> *line = *line + *len; >> if (**line && **line == '\r') >> (*line)++; >> if (**line && **line == '\n') >> (*line)++; >> } >> >> if (!**line) >> return 0; >> >> eol = strchrnul(*line, '\n'); >> >> /* Trim trailing CR from length */ >> if (eol > *line && eol[-1] == '\r') >> eol--; >> >> *len = eol - *line; >> return 1; >> } > >It is a confusing piece of "we handle one line at a time" helper. >It is not obvious what the loop invariants are. > >It would be most natural to readers if *line points at the very >beginning of the buffer, i.e. the beginning of the first line, >and *len points at the very first character of that line, i.e. 0. > >But then the first thing this function worries about is a case where >*len is not 0. I obviously am biased, but sorry, I find what I gave >you 100 times simpler to understand. > There are a few more places where the same thing happens and text is just split by LF, ignoring CR. The gpg parsing where this code originated being the most prominent example. However those just parse some parts from the output and the worst that seems to happen is a trailing CR in some log outputs. If we are ok with this then your version is indeed the better one. If we want to correct the parsing at the other sites then I think a more generalized function would be better. Since the gpg stuff is in place for a long time and no one complained we can probably leave it as is. I'll prepare a new patch. Thanks ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2] gpg-interface: trim CR from ssh-keygen 2022-01-06 10:26 ` Fabian Stelzer @ 2022-01-06 17:50 ` Junio C Hamano 0 siblings, 0 replies; 30+ messages in thread From: Junio C Hamano @ 2022-01-06 17:50 UTC (permalink / raw) To: Fabian Stelzer Cc: Eric Sunshine, Git List, Pedro Martelletto, Jeff King, Johannes Schindelin Fabian Stelzer <fs@gigacodes.de> writes: > There are a few more places where the same thing happens and text > is just split by LF, ignoring CR. The gpg parsing where this code > originated being the most prominent example. ... ... Since the > gpg stuff is in place for a long time and no one complained we can > probably leave it as is. Yeah, that is the conclusion I was hoping for. Thanks. ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2] gpg-interface: trim CR from ssh-keygen 2022-01-05 10:36 ` Fabian Stelzer 2022-01-05 20:40 ` Junio C Hamano @ 2022-01-09 20:49 ` Eric Sunshine 2022-01-10 12:28 ` Fabian Stelzer 1 sibling, 1 reply; 30+ messages in thread From: Eric Sunshine @ 2022-01-09 20:49 UTC (permalink / raw) To: Fabian Stelzer Cc: Junio C Hamano, Git List, Pedro Martelletto, Jeff King, Johannes Schindelin On Wed, Jan 5, 2022 at 5:36 AM Fabian Stelzer <fs@gigacodes.de> wrote: > On 05.01.2022 02:09, Eric Sunshine wrote: > >> > line = strchrnul(line + 1, '\n')) { > >> > while (*line == '\n') > >> > line++; > >> > if (!*line) > >> > break; > > > >Indeed, the existing code is confusing me. I've been staring at it for > >several minutes and I think I'm still failing to understand the > >purpose of the +1 in the strchrnul() call. Perhaps I'm missing > >something obvious(?). > > This whole loop was basically copied from parse_gpg_output() above. Without > the +1 this would always find the same line in the buffer. The +1 skips over > the previously found LF. I still don't see the point of +1 in the strchrnul() call. After: line = strchrnul(line + 1, '\n')) `line` is going to point either at '\n' or at NUL. Then: while (*line == '\n') line++; skips over the '\n' if present. So, by the time the next loop iteration starts, `line` will already be pointing past the '\n' we just found, thus the +1 seems pointless (and maybe even buggy). But perhaps I have a blind spot and am missing something obvious... ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2] gpg-interface: trim CR from ssh-keygen 2022-01-09 20:49 ` Eric Sunshine @ 2022-01-10 12:28 ` Fabian Stelzer 0 siblings, 0 replies; 30+ messages in thread From: Fabian Stelzer @ 2022-01-10 12:28 UTC (permalink / raw) To: Eric Sunshine Cc: Junio C Hamano, Git List, Pedro Martelletto, Jeff King, Johannes Schindelin On 09.01.2022 15:49, Eric Sunshine wrote: >On Wed, Jan 5, 2022 at 5:36 AM Fabian Stelzer <fs@gigacodes.de> wrote: >> On 05.01.2022 02:09, Eric Sunshine wrote: >> >> > line = strchrnul(line + 1, '\n')) { >> >> > while (*line == '\n') >> >> > line++; >> >> > if (!*line) >> >> > break; >> > >> >Indeed, the existing code is confusing me. I've been staring at it for >> >several minutes and I think I'm still failing to understand the >> >purpose of the +1 in the strchrnul() call. Perhaps I'm missing >> >something obvious(?). >> >> This whole loop was basically copied from parse_gpg_output() above. Without >> the +1 this would always find the same line in the buffer. The +1 skips over >> the previously found LF. > >I still don't see the point of +1 in the strchrnul() call. After: > > line = strchrnul(line + 1, '\n')) > >`line` is going to point either at '\n' or at NUL. Then: > > while (*line == '\n') > line++; > >skips over the '\n' if present. So, by the time the next loop >iteration starts, `line` will already be pointing past the '\n' we >just found, thus the +1 seems pointless (and maybe even buggy). > >But perhaps I have a blind spot and am missing something obvious... Hm, yeah. I think you are correct. The while below should make the +1 unnecessary. I think this never mattered to parse_gpg_output() since it is only looking for the [GNUPG:] status line which probably comes first anyway. If it does not then I think the loop will skip over it. Same thing with ssh (but we are changing this whole loop anyway) ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v3] gpg-interface: trim CR from ssh-keygen 2022-01-03 9:53 ` [PATCH v2] gpg-interface: trim CR from ssh-keygen Fabian Stelzer 2022-01-03 17:17 ` Eric Sunshine @ 2022-01-07 9:07 ` Fabian Stelzer 2022-01-09 21:37 ` Eric Sunshine 1 sibling, 1 reply; 30+ messages in thread From: Fabian Stelzer @ 2022-01-07 9:07 UTC (permalink / raw) To: git Cc: Fabian Stelzer, Pedro Martelletto, Junio C Hamano, Jeff King, Johannes Schindelin, Eric Sunshine 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> --- gpg-interface.c | 34 ++++++++++++++++++++++++---------- 1 file changed, 24 insertions(+), 10 deletions(-) diff --git a/gpg-interface.c b/gpg-interface.c index b52eb0e2e0..17b1e44baa 100644 --- a/gpg-interface.c +++ b/gpg-interface.c @@ -433,7 +433,6 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc, struct tempfile *buffer_file; int ret = -1; const char *line; - size_t trust_size; char *principal; struct strbuf ssh_principals_out = STRBUF_INIT; struct strbuf ssh_principals_err = STRBUF_INIT; @@ -502,15 +501,30 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc, ret = -1; } else { /* Check every principal we found (one per line) */ - 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); + 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); child_process_init(&ssh_keygen); strbuf_release(&ssh_keygen_out); -- 2.33.1 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH v3] gpg-interface: trim CR from ssh-keygen 2022-01-07 9:07 ` [PATCH v3] " Fabian Stelzer @ 2022-01-09 21:37 ` Eric Sunshine 2022-01-10 12:59 ` Fabian Stelzer 2022-01-10 17:03 ` Junio C Hamano 0 siblings, 2 replies; 30+ messages in thread From: Eric Sunshine @ 2022-01-09 21:37 UTC (permalink / raw) To: Fabian Stelzer Cc: git, Pedro Martelletto, Junio C Hamano, Jeff King, Johannes Schindelin 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]? [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. ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3] gpg-interface: trim CR from ssh-keygen 2022-01-09 21:37 ` Eric Sunshine @ 2022-01-10 12:59 ` Fabian Stelzer 2022-01-10 17:51 ` Junio C Hamano 2022-01-10 17:03 ` Junio C Hamano 1 sibling, 1 reply; 30+ messages in thread From: Fabian Stelzer @ 2022-01-10 12:59 UTC (permalink / raw) To: Eric Sunshine Cc: git, Pedro Martelletto, Junio C Hamano, Jeff King, Johannes Schindelin 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. ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3] gpg-interface: trim CR from ssh-keygen 2022-01-10 12:59 ` Fabian Stelzer @ 2022-01-10 17:51 ` Junio C Hamano 0 siblings, 0 replies; 30+ messages in thread From: Junio C Hamano @ 2022-01-10 17:51 UTC (permalink / raw) To: Fabian Stelzer Cc: Eric Sunshine, git, Pedro Martelletto, Jeff King, Johannes Schindelin Fabian Stelzer <fs@gigacodes.de> writes: > 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). Heh, helped-by might be OK but I certainly didn't write it. I merely translated what you wrote without knowing exactly what's on these lines (and what I knew, like the lines are the unit of processing and empty lines are to be skipped, I all learned from your original without knowing why) ;-) So if anything, Helped-by: is good enough, but I do not need more credit or blame ;-) ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3] gpg-interface: trim CR from ssh-keygen 2022-01-09 21:37 ` Eric Sunshine 2022-01-10 12:59 ` Fabian Stelzer @ 2022-01-10 17:03 ` Junio C Hamano 1 sibling, 0 replies; 30+ messages in thread From: Junio C Hamano @ 2022-01-10 17:03 UTC (permalink / raw) To: Eric Sunshine Cc: Fabian Stelzer, git, Pedro Martelletto, Jeff King, Johannes Schindelin Eric Sunshine <sunshine@sunshineco.com> writes: > 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. Unless you are writing an interactive text editor, an array of lines, each of which can individually be manupulated cheaply when inserting or deleting a span of chars, is a way too ugly and overly expensive data structure to keep your data in the long haul. In short, strbuf_split() was a mistaken piece of API that does not belong to this project ;-) The cycles spent by crypto before getting to this point in the code is expensive enough that the extra cycles to separately scan to split them into lines and another scan from the end of the each line to trim may not matter, so I'd stop at saying "I'd rather not to see the above code" instead of my usual "Please don't", from performance perspective in this case. But from code cleanliness perspective, well, let me just say that this is not Python or Java but a C project. ^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2022-01-10 17:51 UTC | newest] Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-12-03 13:31 [PATCH] gpg-interface: trim CR from ssh-keygen -Y find-principals Johannes Schindelin via GitGitGadget 2021-12-03 14:18 ` Fabian Stelzer 2021-12-03 15:58 ` Jeff King 2021-12-04 13:11 ` Fabian Stelzer 2021-12-05 5:50 ` Junio C Hamano [not found] ` <CABPYr=y+sDDko9zPxQTOM6Tz4E7CafH7hJc6oB1zv7XYA9KH1A@mail.gmail.com> 2021-12-09 16:33 ` Fabian Stelzer [not found] ` <CABPYr=xfotWvTQK9k1eKHa0kP4SsB=TKKuM0d8cpMb5BtuUZLA@mail.gmail.com> 2021-12-09 17:20 ` Fabian Stelzer 2021-12-30 10:25 ` Fabian Stelzer 2021-12-05 23:06 ` Damien Miller 2021-12-06 8:39 ` Fabian Stelzer 2022-01-03 9:53 ` [PATCH v2] gpg-interface: trim CR from ssh-keygen Fabian Stelzer 2022-01-03 17:17 ` Eric Sunshine 2022-01-03 23:34 ` Junio C Hamano 2022-01-04 0:41 ` Eric Sunshine 2022-01-04 1:19 ` Junio C Hamano 2022-01-04 3:06 ` Eric Sunshine 2022-01-04 12:55 ` Fabian Stelzer 2022-01-04 19:33 ` Junio C Hamano 2022-01-05 7:09 ` Eric Sunshine 2022-01-05 10:36 ` Fabian Stelzer 2022-01-05 20:40 ` Junio C Hamano 2022-01-06 10:26 ` Fabian Stelzer 2022-01-06 17:50 ` Junio C Hamano 2022-01-09 20:49 ` Eric Sunshine 2022-01-10 12:28 ` Fabian Stelzer 2022-01-07 9:07 ` [PATCH v3] " Fabian Stelzer 2022-01-09 21:37 ` Eric Sunshine 2022-01-10 12:59 ` Fabian Stelzer 2022-01-10 17:51 ` Junio C Hamano 2022-01-10 17:03 ` Junio C Hamano
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).