{"thread":{"id":"57022","subject":"[PATCH] gpg-interface: trim CR from ssh-keygen -Y find-principals","startedAt":"2021-12-03T13:31:20Z","lastAt":"2022-01-10T17:51:30Z","messageCount":30,"participants":["Johannes Schindelin via GitGitGadget","Fabian Stelzer","Jeff King","Junio C Hamano","Damien Miller","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"442975","messageId":"pull.1090.git.1638538276608.gitgitgadget@gmail.com","threadId":"57022","inReplyTo":null,"subject":"[PATCH] gpg-interface: trim CR from ssh-keygen -Y find-principals","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-12-03T13:31:16Z","receivedAt":"2021-12-03T13:31:20Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: pedro martelletto \n\nWe need to trim \\r from the output of 'ssh-keygen -Y find-principals' on\nWindows, or we end up calling 'ssh-keygen -Y verify' with a bogus signer\nidentity. ssh-keygen.c:2841 contains a call to puts(3), which confirms this\nhypothesis. Signature verification passes with the fix.\n\nSigned-off-by: pedro martelletto \nSigned-off-by: Johannes Schindelin \n---\n Allow for CR in the output of ssh-keygen\n \n This came in via https://github.com/git-for-windows/git/pull/3561. It\n affects current Windows versions of OpenSSH (but apparently not the\n MSYS2 version included in Git for Windows).\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1090%2Fdscho%2Fallow-cr-from-ssh-keygen-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1090/dscho/allow-cr-from-ssh-keygen-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1090\n\n gpg-interface.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex 3e7255a2a91..85e26882782 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -497,7 +497,7 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc,\n \t\t\tif (!*line)\n \t\t\t\tbreak;\n \n-\t\t\ttrust_size = strcspn(line, \"\\n\");\n+\t\t\ttrust_size = strcspn(line, \"\\r\\n\");\n \t\t\tprincipal = xmemdupz(line, trust_size);\n \n \t\t\tchild_process_init(&ssh_keygen);\n\nbase-commit: abe6bb3905392d5eb6b01fa6e54d7e784e0522aa\n-- \ngitgitgadget\n"},{"id":"442998","messageId":"20211203141822.w3d2poeiylm6zpf6@fs","threadId":"57022","inReplyTo":"pull.1090.git.1638538276608.gitgitgadget@gmail.com","subject":"Re: [PATCH] gpg-interface: trim CR from ssh-keygen -Y find-principals","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2021-12-03T14:18:22Z","receivedAt":"2021-12-03T14:18:28Z","isPatch":true,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"On 03.12.2021 13:31, Johannes Schindelin via GitGitGadget wrote:\n>From: pedro martelletto \n>\n>We need to trim \\r from the output of 'ssh-keygen -Y find-principals' on\n>Windows, or we end up calling 'ssh-keygen -Y verify' with a bogus signer\n>identity. ssh-keygen.c:2841 contains a call to puts(3), which confirms this\n>hypothesis. Signature verification passes with the fix.\n\nThis fix is obviously fine. But I'm a unsure if this is the only place where \nwe would need to account for windows line endings. There are at least two \nsimilar uses in gpg-interface.c. parse_ssh_output() might include a trailing \n\\r in the fingerprint. So I think we should do the same thing here:\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex 330cfc5845..92cd0f0ebd 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -383,7 +383,7 @@ static void parse_ssh_output(struct signature_check *sigc)\n \tsigc->result = 'B';\n \tsigc->trust_level = TRUST_NEVER;\n \n-\tline = to_free = xmemdupz(sigc->output, strcspn(sigc->output, \"\\n\"));\n+\tline = to_free = xmemdupz(sigc->output, strcspn(sigc->output, \"\\r\\n\"));\n \n \tif (skip_prefix(line, \"Good \\\"git\\\" signature for \", &line)) {\n \t\t/* Search for the last \"with\" to get the full principal */\n\nget_default_ssh_signing_key() also splits the defaultKeyCommand output by \\n \nbut only puts the result into a file for ssh to use which should be able to \ndeal with it.\n\nHowever the whole parse_gpg_output() also assumes \"\\n\" everywhere. So either \nGPG behaves differently under windows than ssh or a similar bug could be in \nthere (and if, then probably is for a long time).\nI'm not familiar with the windows details (like what MSYS2 is / whats \ndifferent here) and don't really have the means to test it.\n\n\n>\n>Signed-off-by: pedro martelletto \n>Signed-off-by: Johannes Schindelin \n>---\n> Allow for CR in the output of ssh-keygen\n>\n> This came in via https://github.com/git-for-windows/git/pull/3561. It\n> affects current Windows versions of OpenSSH (but apparently not the\n> MSYS2 version included in Git for Windows).\n>\n>Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1090%2Fdscho%2Fallow-cr-from-ssh-keygen-v1\n>Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1090/dscho/allow-cr-from-ssh-keygen-v1\n>Pull-Request: https://github.com/gitgitgadget/git/pull/1090\n>\n> gpg-interface.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n>diff --git a/gpg-interface.c b/gpg-interface.c\n>index 3e7255a2a91..85e26882782 100644\n>--- a/gpg-interface.c\n>+++ b/gpg-interface.c\n>@@ -497,7 +497,7 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc,\n> \t\t\tif (!*line)\n> \t\t\t\tbreak;\n>\n>-\t\t\ttrust_size = strcspn(line, \"\\n\");\n>+\t\t\ttrust_size = strcspn(line, \"\\r\\n\");\n> \t\t\tprincipal = xmemdupz(line, trust_size);\n>\n> \t\t\tchild_process_init(&ssh_keygen);\n>\n>base-commit: abe6bb3905392d5eb6b01fa6e54d7e784e0522aa\n>-- \n>gitgitgadget\n"},{"id":"443005","messageId":"Yao+l0ckDWZNf4AE@coredump.intra.peff.net","threadId":"57022","inReplyTo":"pull.1090.git.1638538276608.gitgitgadget@gmail.com","subject":"Re: [PATCH] gpg-interface: trim CR from ssh-keygen -Y find-principals","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-12-03T15:58:15Z","receivedAt":"2021-12-03T15:58:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 03, 2021 at 01:31:16PM +0000, Johannes Schindelin via GitGitGadget wrote:\n\n> We need to trim \\r from the output of 'ssh-keygen -Y find-principals' on\n> Windows, or we end up calling 'ssh-keygen -Y verify' with a bogus signer\n> identity. ssh-keygen.c:2841 contains a call to puts(3), which confirms this\n> hypothesis. Signature verification passes with the fix.\n> [...]\n> @@ -497,7 +497,7 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc,\n> \t\t\tif (!*line)\n> \t\t\t\tbreak;\n> \n> -\t\t\ttrust_size = strcspn(line, \"\\n\");\n> +\t\t\ttrust_size = strcspn(line, \"\\r\\n\");\n> \t\t\tprincipal = xmemdupz(line, trust_size);\n\nJust playing devil's advocate for a moment: this parsing is kind of\nloose. Is there any chance that I could smuggle a CR into my principal\nname, and make \"a principal\\rthat is fake\" now get parsed as \"a\nprincipal\"? Our strcspn() here would cut off at the first CR.\n\nI'm guessing probably not, but when it comes to something with security\nimplications like this, it pays to be extra careful. I'm hoping somebody\nfamiliar with the ssh-keygen side and how the rest of the parsing works\n(like Fabian) can verify that this is OK.\n\n-Peff\n"},{"id":"443076","messageId":"20211204131149.cvyu7dvf6p66dotq@fs","threadId":"57022","inReplyTo":"Yao+l0ckDWZNf4AE@coredump.intra.peff.net","subject":"Re: [PATCH] gpg-interface: trim CR from ssh-keygen -Y find-principals","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2021-12-04T13:11:49Z","receivedAt":"2021-12-04T13:11:56Z","isPatch":true,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"On 03.12.2021 10:58, Jeff King wrote:\n>On Fri, Dec 03, 2021 at 01:31:16PM +0000, Johannes Schindelin via GitGitGadget wrote:\n>\n>> We need to trim \\r from the output of 'ssh-keygen -Y find-principals' on\n>> Windows, or we end up calling 'ssh-keygen -Y verify' with a bogus signer\n>> identity. ssh-keygen.c:2841 contains a call to puts(3), which confirms this\n>> hypothesis. Signature verification passes with the fix.\n>> [...]\n>> @@ -497,7 +497,7 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc,\n>> \t\t\tif (!*line)\n>> \t\t\t\tbreak;\n>>\n>> -\t\t\ttrust_size = strcspn(line, \"\\n\");\n>> +\t\t\ttrust_size = strcspn(line, \"\\r\\n\");\n>> \t\t\tprincipal = xmemdupz(line, trust_size);\n>\n>Just playing devil's advocate for a moment: this parsing is kind of\n>loose. Is there any chance that I could smuggle a CR into my principal\n>name, and make \"a principal\\rthat is fake\" now get parsed as \"a\n>principal\"? Our strcspn() here would cut off at the first CR.\n>\n>I'm guessing probably not, but when it comes to something with security\n>implications like this, it pays to be extra careful. I'm hoping somebody\n>familiar with the ssh-keygen side and how the rest of the parsing works\n>(like Fabian) can verify that this is OK.\n>\n\nA good point. I just tested this and CR is a valid character to use in a \nprincipal name in the allowed signers file and as of now the principal will \nbe passed to the verify call `as is` and everything works just fine. When we \nintroduce the patch above a principal with a CR in it will fail to verify.\n\nI've added Damien Miller to this thread. He knows more about what the \nexpected behaviour for the principal would/should be. I think at the moment \nalmost everything except \\n or \\0 goes. Maybe restricting \\r as well would \nmake life easier for other uses too?\n\n From a security perspective I don't think this is problem. The principal \ndoes not come from any user input but is actually looked up in the allowed \nsigners file using the signatures public key (with ssh-keygen -Y \nfind-principals). If I could manipulate this file I could change the key as \nwell.\n\nIf we add `trust on first use` in a future series I would assume we use the \nemail address from the commit/tag author ident when adding a new principal \nto the file. Can the ident contain a CR?\nEven if it did, I would only allow a list of allowed alphanumeric chars to \nbe added anyway since a principal can contain wildcards which we obviously \ndon't want to trust on first use ;).\n\nThanks\n"},{"id":"443098","messageId":"xmqqk0gjob0x.fsf@gitster.g","threadId":"57022","inReplyTo":"20211204131149.cvyu7dvf6p66dotq@fs","subject":"Re: [PATCH] gpg-interface: trim CR from ssh-keygen -Y find-principals","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-05T05:50:38Z","receivedAt":"2021-12-05T05:50:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Fabian Stelzer writes:\n\n> On 03.12.2021 10:58, Jeff King wrote:\n>>On Fri, Dec 03, 2021 at 01:31:16PM +0000, Johannes Schindelin via GitGitGadget wrote:\n>>\n>>> We need to trim \\r from the output of 'ssh-keygen -Y find-principals' on\n>>> Windows, or we end up calling 'ssh-keygen -Y verify' with a bogus signer\n>>> identity. ssh-keygen.c:2841 contains a call to puts(3), which confirms this\n>>> hypothesis. Signature verification passes with the fix.\n>>> [...]\n>>> @@ -497,7 +497,7 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc,\n>>> \t\t\tif (!*line)\n>>> \t\t\t\tbreak;\n>>>\n>>> -\t\t\ttrust_size = strcspn(line, \"\\n\");\n>>> +\t\t\ttrust_size = strcspn(line, \"\\r\\n\");\n>>> \t\t\tprincipal = xmemdupz(line, trust_size);\n>>\n>>Just playing devil's advocate for a moment: this parsing is kind of\n>>loose. Is there any chance that I could smuggle a CR into my principal\n>>name, and make \"a principal\\rthat is fake\" now get parsed as \"a\n>>principal\"? Our strcspn() here would cut off at the first CR.\n>>\n>>I'm guessing probably not, but when it comes to something with security\n>>implications like this, it pays to be extra careful. I'm hoping somebody\n>>familiar with the ssh-keygen side and how the rest of the parsing works\n>>(like Fabian) can verify that this is OK.\n>>\n>\n> A good point. I just tested this and CR is a valid character to use in\n> a principal name in the allowed signers file and as of now the\n> principal will be passed to the verify call `as is` and everything\n> works just fine. When we introduce the patch above a principal with a\n> CR in it will fail to verify.\n>\n> I've added Damien Miller to this thread. He knows more about what the\n> expected behaviour for the principal would/should be. I think at the\n> moment almost everything except \\n or \\0 goes. Maybe restricting \\r as\n> well would make life easier for other uses too?\n>\n> From a security perspective I don't think this is problem. The\n> principal does not come from any user input but is actually looked up\n> in the allowed signers file using the signatures public key (with\n> ssh-keygen -Y find-principals). If I could manipulate this file I\n> could change the key as well.\n>\n> If we add `trust on first use` in a future series I would assume we\n> use the email address from the commit/tag author ident when adding a\n> new principal to the file. Can the ident contain a CR?\n> Even if it did, I would only allow a list of allowed alphanumeric\n> chars to be added anyway since a principal can contain wildcards which\n> we obviously don't want to trust on first use ;).\n\nSo instead of the posted patch, we should do something along this\nline instead?\n\n\ttrust_size = strcspn(line, \"\\n\"); /* truncate at LF */\n\tif (trust_size && line[trust_size - 1] == '\\r')\n\t\ttrust_size--; /* the LF was part of CRLF at the end */\n\n\n"},{"id":"443120","messageId":"70cee773-9547-e3cf-9327-ac0213d327e@mindrot.org","threadId":"57022","inReplyTo":"20211204131149.cvyu7dvf6p66dotq@fs","subject":"Re: [PATCH] gpg-interface: trim CR from ssh-keygen -Y find-principals","fromName":"Damien Miller","fromEmail":"djm@mindrot.org","sentAt":"2021-12-05T23:06:27Z","receivedAt":"2021-12-05T23:13:43Z","isPatch":true,"sender":{"key":"djm@mindrot.org","avatar":null},"body":"On Sat, 4 Dec 2021, Fabian Stelzer wrote:\n\n> > I'm guessing probably not, but when it comes to something with security\n> > implications like this, it pays to be extra careful. I'm hoping somebody\n> > familiar with the ssh-keygen side and how the rest of the parsing works\n> > (like Fabian) can verify that this is OK.\n> > \n> \n> A good point. I just tested this and CR is a valid character to use in a\n> principal name in the allowed signers file and as of now the principal will be\n> passed to the verify call `as is` and everything works just fine. When we\n> introduce the patch above a principal with a CR in it will fail to verify.\n\nAre you sure? I thought that we split principals in allowed_signers on\nmost whitespace, including \\r. Follow:\n\nhttps://github.com/openssh/openssh-portable/blob/e9c7149/sshsig.c#L742\nhttps://github.com/openssh/openssh-portable/blob/e9c7149/misc.c#L452\nhttps://github.com/openssh/openssh-portable/blob/e9c7149/misc.c#L408\n\n> I've added Damien Miller to this thread. He knows more about what the expected\n> behaviour for the principal would/should be. I think at the moment almost\n> everything except \\n or \\0 goes. Maybe restricting \\r as well would make life\n> easier for other uses too?\n\nIMO sensible content for the principals section would be printable, non-\nwhitespace characters, excluding wildcards ('*', '?'). ssh-keygen mostly\nassumes that the file is in good order, but maybe it could be stricter.\n\n> If we add `trust on first use` in a future series I would assume we use the\n> email address from the commit/tag author ident when adding a new principal to\n> the file. Can the ident contain a CR?\n> Even if it did, I would only allow a list of allowed alphanumeric chars to be\n> added anyway since a principal can contain wildcards which we obviously don't\n> want to trust on first use ;).\n\nYeah. my mental model for the allowed_signers file is that it's similar\nto ~/.ssh/authorized_keys in that it directly controls authn/authz\ndecisions, and if you put bad stuff in there then you're going to have\na bad day...\n\n-d\n"},{"id":"443142","messageId":"20211206083925.tuy2w3wzlgpc36bj@fs","threadId":"57022","inReplyTo":"70cee773-9547-e3cf-9327-ac0213d327e@mindrot.org","subject":"Re: [PATCH] gpg-interface: trim CR from ssh-keygen -Y find-principals","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2021-12-06T08:39:25Z","receivedAt":"2021-12-06T08:39:30Z","isPatch":true,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"On 06.12.2021 10:06, Damien Miller wrote:\n>On Sat, 4 Dec 2021, Fabian Stelzer wrote:\n>\n>> > I'm guessing probably not, but when it comes to something with security\n>> > implications like this, it pays to be extra careful. I'm hoping somebody\n>> > familiar with the ssh-keygen side and how the rest of the parsing works\n>> > (like Fabian) can verify that this is OK.\n>> >\n>>\n>> A good point. I just tested this and CR is a valid character to use in a\n>> principal name in the allowed signers file and as of now the principal will be\n>> passed to the verify call `as is` and everything works just fine. When we\n>> introduce the patch above a principal with a CR in it will fail to verify.\n>\n>Are you sure? I thought that we split principals in allowed_signers on\n>most whitespace, including \\r. Follow:\n>\n>https://github.com/openssh/openssh-portable/blob/e9c7149/sshsig.c#L742\n>https://github.com/openssh/openssh-portable/blob/e9c7149/misc.c#L452\n>https://github.com/openssh/openssh-portable/blob/e9c7149/misc.c#L408\n>\n\nSorry, I should have mentioned that I quoted the principal. Within the \nquotes whitespace (and \\r) works. Since find-principals then returns one\nprincipal per line the line ending issue can come up.\n\n>> I've added Damien Miller to this thread. He knows more about what the expected\n>> behaviour for the principal would/should be. I think at the moment almost\n>> everything except \\n or \\0 goes. Maybe restricting \\r as well would make life\n>> easier for other uses too?\n>\n>IMO sensible content for the principals section would be printable, non-\n>whitespace characters, excluding wildcards ('*', '?'). ssh-keygen mostly\n>assumes that the file is in good order, but maybe it could be stricter.\n>\n\nOk, I think we can make sure of that when adding principals and use Junios \nsuggested patch for trimming the \\r at the line ending.\n\n>> If we add `trust on first use` in a future series I would assume we use the\n>> email address from the commit/tag author ident when adding a new principal to\n>> the file. Can the ident contain a CR?\n>> Even if it did, I would only allow a list of allowed alphanumeric chars to be\n>> added anyway since a principal can contain wildcards which we obviously don't\n>> want to trust on first use ;).\n>\n>Yeah. my mental model for the allowed_signers file is that it's similar\n>to ~/.ssh/authorized_keys in that it directly controls authn/authz\n>decisions, and if you put bad stuff in there then you're going to have\n>a bad day...\n>\n\nThanks for your input.\n"},{"id":"443648","messageId":"20211209163346.w5ofhoapmjnpgc6y@fs","threadId":"57022","inReplyTo":"CABPYr=y+sDDko9zPxQTOM6Tz4E7CafH7hJc6oB1zv7XYA9KH1A@mail.gmail.com","subject":"Re: [PATCH] gpg-interface: trim CR from ssh-keygen -Y find-principals","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2021-12-09T16:33:46Z","receivedAt":"2021-12-09T16:33:51Z","isPatch":true,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"On 06.12.2021 10:06, Pedro Martelletto wrote:\n>On Sun, Dec 5, 2021 at 6:50 AM Junio C Hamano wrote:\n>\n>> So instead of the posted patch, we should do something along this\n>> line instead?\n>>\n>> trust_size = strcspn(line, \"\\n\"); /* truncate at LF */\n>> if (trust_size && line[trust_size - 1] == '\\r')\n>> trust_size--; /* the LF was part of CRLF at the end */\n>>\n>>\n>>\n>I agree that's a more consistent fix. A minor nit: if the intention is to\n>only trim CR as part of a CRLF sequence, we need to ensure a LF is found:\n>\n\nThis shouldn't be necessary as we split/loop by LF just above.\n\nfor (line = ssh_principals_out.buf; *line;\n line = strchrnul(line + 1, '\\n')) {\n\twhile (*line == '\\n')\n\t\tline++;\n\tif (!*line)\n\t\tbreak;\n\n\ttrust_size = strcspn(line, \"\\n\");\n\tprincipal = xmemdupz(line, trust_size);\n\n-\nFabian\n\n>trust_size = strcspn(line, \"\\n\"); /* truncate at LF */\n>if (trust_size && trust_size != strlen(line) && line[trust_size - 1] ==\n>'\\r')\n> trust_size--; /* the LF was part of CRLF at the end */\n>\n>-p.\n"},{"id":"443655","messageId":"20211209172032.2iyda3rv4zsjry3s@fs","threadId":"57022","inReplyTo":"CABPYr=xfotWvTQK9k1eKHa0kP4SsB=TKKuM0d8cpMb5BtuUZLA@mail.gmail.com","subject":"Re: [PATCH] gpg-interface: trim CR from ssh-keygen -Y find-principals","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2021-12-09T17:20:32Z","receivedAt":"2021-12-09T17:20:36Z","isPatch":true,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"On 09.12.2021 17:58, Pedro Martelletto wrote:\n>On Thu, Dec 9, 2021 at 5:33 PM Fabian Stelzer wrote:\n>\n>> On 06.12.2021 10:06, Pedro Martelletto wrote:\n>> >On Sun, Dec 5, 2021 at 6:50 AM Junio C Hamano wrote:\n>> >\n>> >> So instead of the posted patch, we should do something along this\n>> >> line instead?\n>> >>\n>> >> trust_size = strcspn(line, \"\\n\"); /* truncate at LF */\n>> >> if (trust_size && line[trust_size - 1] == '\\r')\n>> >> trust_size--; /* the LF was part of CRLF at the end */\n>> >>\n>> >>\n>> >>\n>> >I agree that's a more consistent fix. A minor nit: if the intention is to\n>> >only trim CR as part of a CRLF sequence, we need to ensure a LF is found:\n>> >\n>>\n>> This shouldn't be necessary as we split/loop by LF just above.\n>>\n>> for (line = ssh_principals_out.buf; *line;\n>> line = strchrnul(line + 1, '\\n')) {\n>> while (*line == '\\n')\n>> line++;\n>> if (!*line)\n>> break;\n>>\n>> trust_size = strcspn(line, \"\\n\");\n>> principal = xmemdupz(line, trust_size);\n>>\n>\n>The loop ensures that 'line' points to the first character of\n>ssh_principals_out.buf or to a non-NUL character after a '\\n'. It does not\n>ensure that that 'line' contains a '\\n', e.g:\n>\"principalA\\nprincipalB\\nprincipalC\\r\" or just \"principalA\\r\".\n>\n\nOops, yep. You are of course right.\nI still dislike how we have to consider this in various places and i guess \nthere might be more bugs hidden on the windows platform with things like \nthis. I kinda wish we could strip \\r\\n -> \\n within pipe_command and then \nhave the rest of the code not have to deal with it :/\nEspecially since writing a test for this would involve mirroring at leas \nparts oft the ssh-keygen api.\nBut that would be a much bigger/riskier change so for this i think your \nlatest version is fine.\n\n-\nFabian\n"},{"id":"445216","messageId":"20211230102548.q6ugqiyicswfx24v@fs","threadId":"57022","inReplyTo":"CABPYr=xfotWvTQK9k1eKHa0kP4SsB=TKKuM0d8cpMb5BtuUZLA@mail.gmail.com","subject":"Re: [PATCH] gpg-interface: trim CR from ssh-keygen -Y find-principals","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2021-12-30T10:25:48Z","receivedAt":"2021-12-30T10:25:52Z","isPatch":true,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"On 09.12.2021 17:58, Pedro Martelletto wrote:\n>On Thu, Dec 9, 2021 at 5:33 PM Fabian Stelzer wrote:\n>\n>> On 06.12.2021 10:06, Pedro Martelletto wrote:\n>> >On Sun, Dec 5, 2021 at 6:50 AM Junio C Hamano wrote:\n>> >\n>> >> So instead of the posted patch, we should do something along this\n>> >> line instead?\n>> >>\n>> >> trust_size = strcspn(line, \"\\n\"); /* truncate at LF */\n>> >> if (trust_size && line[trust_size - 1] == '\\r')\n>> >> trust_size--; /* the LF was part of CRLF at the end */\n>> >>\n>> >>\n>> >>\n>> >I agree that's a more consistent fix. A minor nit: if the intention is to\n>> >only trim CR as part of a CRLF sequence, we need to ensure a LF is found:\n>> >\n>>\n>> This shouldn't be necessary as we split/loop by LF just above.\n>>\n>> for (line = ssh_principals_out.buf; *line;\n>> line = strchrnul(line + 1, '\\n')) {\n>> while (*line == '\\n')\n>> line++;\n>> if (!*line)\n>> break;\n>>\n>> trust_size = strcspn(line, \"\\n\");\n>> principal = xmemdupz(line, trust_size);\n>>\n>\n>The loop ensures that 'line' points to the first character of\n>ssh_principals_out.buf or to a non-NUL character after a '\\n'. It does not\n>ensure that that 'line' contains a '\\n', e.g:\n>\"principalA\\nprincipalB\\nprincipalC\\r\" or just \"principalA\\r\".\n>\n\nJust saw that this is still open. @pedro: do you want to send an updated \nversion of your patch or would you like me to pick this up and send one?\n\n"},{"id":"445345","messageId":"20220103095337.600536-1-fs@gigacodes.de","threadId":"57022","inReplyTo":"pull.1090.git.1638538276608.gitgitgadget@gmail.com","subject":"[PATCH v2] gpg-interface: trim CR from ssh-keygen","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2022-01-03T09:53:37Z","receivedAt":"2022-01-03T09:53:50Z","isPatch":true,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"We need to trim \\r from the output of 'ssh-keygen -Y find-principals' on\nWindows, or we end up calling 'ssh-keygen -Y verify' with a bogus signer\nidentity. ssh-keygen.c:2841 contains a call to puts(3), which confirms\nthis hypothesis. Signature verification passes with the fix.\n\nHelped-by: Pedro Martelletto \nSigned-off-by: Fabian Stelzer \n---\n gpg-interface.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex b52eb0e2e0..d5eca417e8 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -509,7 +509,10 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc,\n \t\t\tif (!*line)\n \t\t\t\tbreak;\n \n-\t\t\ttrust_size = strcspn(line, \"\\n\");\n+\t\t\ttrust_size = strcspn(line, \"\\n\"); /* truncate at LF */\n+\t\t\tif (trust_size && trust_size != strlen(line) &&\n+\t\t\t line[trust_size - 1] == '\\r')\n+\t\t\t\ttrust_size--; /* the LF was part of CRLF at the end */\n \t\t\tprincipal = xmemdupz(line, trust_size);\n \n \t\t\tchild_process_init(&ssh_keygen);\n-- \n2.33.1\n\n"},{"id":"445363","messageId":"CAPig+cS6h6o2_dJAZC1M1Ace29bN2mhPgaEtTWtj3oXfcHq9cA@mail.gmail.com","threadId":"57022","inReplyTo":"20220103095337.600536-1-fs@gigacodes.de","subject":"Re: [PATCH v2] gpg-interface: trim CR from ssh-keygen","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-01-03T17:17:06Z","receivedAt":"2022-01-03T17:17:18Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Jan 3, 2022 at 9:24 AM Fabian Stelzer wrote:\n> We need to trim \\r from the output of 'ssh-keygen -Y find-principals' on\n> Windows, or we end up calling 'ssh-keygen -Y verify' with a bogus signer\n> identity. ssh-keygen.c:2841 contains a call to puts(3), which confirms\n> this hypothesis. Signature verification passes with the fix.\n>\n> Helped-by: Pedro Martelletto \n> Signed-off-by: Fabian Stelzer \n> ---\n> diff --git a/gpg-interface.c b/gpg-interface.c\n> @@ -509,7 +509,10 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc,\n> - trust_size = strcspn(line, \"\\n\");\n> + trust_size = strcspn(line, \"\\n\"); /* truncate at LF */\n> + if (trust_size && trust_size != strlen(line) &&\n> + line[trust_size - 1] == '\\r')\n> + trust_size--; /* the LF was part of CRLF at the end */\n\nI may be misunderstanding, but isn't the strlen() unnecessary?\n\n if (trust_size && line[trust_size] &&\n line[trust_size - 1] == '\\r')\n trust_size--;\n"},{"id":"445370","messageId":"xmqqee5oieb2.fsf@gitster.g","threadId":"57022","inReplyTo":"CAPig+cS6h6o2_dJAZC1M1Ace29bN2mhPgaEtTWtj3oXfcHq9cA@mail.gmail.com","subject":"Re: [PATCH v2] gpg-interface: trim CR from ssh-keygen","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-03T23:34:41Z","receivedAt":"2022-01-03T23:34:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> On Mon, Jan 3, 2022 at 9:24 AM Fabian Stelzer wrote:\n>> We need to trim \\r from the output of 'ssh-keygen -Y find-principals' on\n>> Windows, or we end up calling 'ssh-keygen -Y verify' with a bogus signer\n>> identity. ssh-keygen.c:2841 contains a call to puts(3), which confirms\n>> this hypothesis. Signature verification passes with the fix.\n>>\n>> Helped-by: Pedro Martelletto \n>> Signed-off-by: Fabian Stelzer \n>> ---\n>> diff --git a/gpg-interface.c b/gpg-interface.c\n>> @@ -509,7 +509,10 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc,\n>> - trust_size = strcspn(line, \"\\n\");\n>> + trust_size = strcspn(line, \"\\n\"); /* truncate at LF */\n>> + if (trust_size && trust_size != strlen(line) &&\n>> + line[trust_size - 1] == '\\r')\n>> + trust_size--; /* the LF was part of CRLF at the end */\n>\n> I may be misunderstanding, but isn't the strlen() unnecessary?\n>\n> if (trust_size && line[trust_size] &&\n> line[trust_size - 1] == '\\r')\n> trust_size--;\n\nThat changes behaviour when \"line\" has more than one lines in it.\nstrcspn() finds the first LF, and the posted patch ignores CRLF not\nat the end of line[]. Your variant feels more correct if the\nobjective is to find the end of the first line (regardless of the\nchoice of the end-of-line convention, either LF or CRLF) and omit\nthe line terminator.\n"},{"id":"445372","messageId":"CAPig+cTM3wZz4NXjxYeBuFv0CVNS-T+pBFeVkfMQ-25pL1kBzw@mail.gmail.com","threadId":"57022","inReplyTo":"xmqqee5oieb2.fsf@gitster.g","subject":"Re: [PATCH v2] gpg-interface: trim CR from ssh-keygen","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-01-04T00:41:51Z","receivedAt":"2022-01-04T00:42:03Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Jan 3, 2022 at 6:34 PM Junio C Hamano wrote:\n> Eric Sunshine writes:\n> > On Mon, Jan 3, 2022 at 9:24 AM Fabian Stelzer wrote:\n> >> We need to trim \\r from the output of 'ssh-keygen -Y find-principals' on\n> >> Windows, or we end up calling 'ssh-keygen -Y verify' with a bogus signer\n> >> identity. ssh-keygen.c:2841 contains a call to puts(3), which confirms\n> >> this hypothesis. Signature verification passes with the fix.\n> >> ---\n> >> - trust_size = strcspn(line, \"\\n\");\n> >> + trust_size = strcspn(line, \"\\n\"); /* truncate at LF */\n> >> + if (trust_size && trust_size != strlen(line) &&\n> >> + line[trust_size - 1] == '\\r')\n> >> + trust_size--; /* the LF was part of CRLF at the end */\n> >\n> > I may be misunderstanding, but isn't the strlen() unnecessary?\n> >\n> > if (trust_size && line[trust_size] &&\n> > line[trust_size - 1] == '\\r')\n> > trust_size--;\n>\n> That changes behaviour when \"line\" has more than one lines in it.\n> strcspn() finds the first LF, and the posted patch ignores CRLF not\n> at the end of line[]. Your variant feels more correct if the\n> objective is to find the end of the first line (regardless of the\n> choice of the end-of-line convention, either LF or CRLF) and omit\n> the line terminator.\n\nOkay, that makes sense if that's the intention of the patch. Perhaps\nthe commit message should mention that `line` might contain multiple\nlines and that it's only interested in the very last LF (unless it's\nalready obvious to everyone else, even though it wasn't to me). I\nthink it can still be done without strlen(), but it gets uglier and\nless obvious[*], so strlen() is probably the way to go, and I presume\nthis isn't a hot path, so no big reason to avoid strlen().\n\n[*] Like this, for instance, which is safe because there must be at\nleast one character after the '\\n' since this is a NUL-terminated\nstring:\n\n if (trust_size && line[trust_size] == '\\n'\n line[true_size + 1] == '\\0' &&\n line[trust_size - 1] == '\\r')\n"},{"id":"445373","messageId":"xmqqmtkcguvm.fsf@gitster.g","threadId":"57022","inReplyTo":"CAPig+cTM3wZz4NXjxYeBuFv0CVNS-T+pBFeVkfMQ-25pL1kBzw@mail.gmail.com","subject":"Re: [PATCH v2] gpg-interface: trim CR from ssh-keygen","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-04T01:19:41Z","receivedAt":"2022-01-04T01:19:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> On Mon, Jan 3, 2022 at 6:34 PM Junio C Hamano wrote:\n>> Eric Sunshine writes:\n>> > On Mon, Jan 3, 2022 at 9:24 AM Fabian Stelzer wrote:\n>> >> We need to trim \\r from the output of 'ssh-keygen -Y find-principals' on\n>> >> Windows, or we end up calling 'ssh-keygen -Y verify' with a bogus signer\n>> >> identity. ssh-keygen.c:2841 contains a call to puts(3), which confirms\n>> >> this hypothesis. Signature verification passes with the fix.\n>> >> ---\n>> >> - trust_size = strcspn(line, \"\\n\");\n>> >> + trust_size = strcspn(line, \"\\n\"); /* truncate at LF */\n>> >> + if (trust_size && trust_size != strlen(line) &&\n>> >> + line[trust_size - 1] == '\\r')\n>> >> + trust_size--; /* the LF was part of CRLF at the end */\n>> >\n>> > I may be misunderstanding, but isn't the strlen() unnecessary?\n>> >\n>> > if (trust_size && line[trust_size] &&\n>> > line[trust_size - 1] == '\\r')\n>> > trust_size--;\n>>\n>> That changes behaviour when \"line\" has more than one lines in it.\n>> strcspn() finds the first LF, and the posted patch ignores CRLF not\n>> at the end of line[]. Your variant feels more correct if the\n>> objective is to find the end of the first line (regardless of the\n>> choice of the end-of-line convention, either LF or CRLF) and omit\n>> the line terminator.\n>\n> Okay, that makes sense if that's the intention of the patch. Perhaps\n> the commit message should mention that `line` might contain multiple\n> lines and that it's only interested in the very last LF (unless it's\n> already obvious to everyone else, even though it wasn't to me).\n\nI do not think that is the case. strcspn(line, \"\\n\") will stop at\nthe first one, so unless it is guaranteed that \"line\" has only one\nline in it, the patch as posted is not correct. Your variant\nwithout strlen() feels more correct, as I said.\n\n"},{"id":"445386","messageId":"CAPig+cR93GyN53JoZbaiROrNtzGjiet7eTPQOk-26G+mB0KaCA@mail.gmail.com","threadId":"57022","inReplyTo":"xmqqmtkcguvm.fsf@gitster.g","subject":"Re: [PATCH v2] gpg-interface: trim CR from ssh-keygen","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-01-04T03:06:14Z","receivedAt":"2022-01-04T03:06:29Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Jan 3, 2022 at 8:19 PM Junio C Hamano wrote:\n> Eric Sunshine writes:\n> > On Mon, Jan 3, 2022 at 6:34 PM Junio C Hamano wrote:\n> >> Eric Sunshine writes:\n> >> > On Mon, Jan 3, 2022 at 9:24 AM Fabian Stelzer wrote:\n> >> >> - trust_size = strcspn(line, \"\\n\");\n> >> >> + trust_size = strcspn(line, \"\\n\"); /* truncate at LF */\n> >> >> + if (trust_size && trust_size != strlen(line) &&\n> >> >> + line[trust_size - 1] == '\\r')\n> >> >> + trust_size--; /* the LF was part of CRLF at the end */\n> >> >\n> >> > I may be misunderstanding, but isn't the strlen() unnecessary?\n> >> >\n> >> > if (trust_size && line[trust_size] &&\n> >> > line[trust_size - 1] == '\\r')\n> >> > trust_size--;\n> >>\n> >> That changes behaviour when \"line\" has more than one lines in it.\n> >> strcspn() finds the first LF, and the posted patch ignores CRLF not\n> >> at the end of line[]. Your variant feels more correct if the\n> >> objective is to find the end of the first line (regardless of the\n> >> choice of the end-of-line convention, either LF or CRLF) and omit\n> >> the line terminator.\n> >\n> > Okay, that makes sense if that's the intention of the patch. Perhaps\n> > the commit message should mention that `line` might contain multiple\n> > lines and that it's only interested in the very last LF (unless it's\n> > already obvious to everyone else, even though it wasn't to me).\n>\n> I do not think that is the case. strcspn(line, \"\\n\") will stop at\n> the first one, so unless it is guaranteed that \"line\" has only one\n> line in it, the patch as posted is not correct. Your variant\n> without strlen() feels more correct, as I said.\n\nOkay, sorry for my unclear thinking. The existing code (before this\npatch) does indeed seem to be interested only in the first line of\n`line`, in which case I agree that the patch's use of strlen() does\nnot appear to be correct if `line` could ever contain more than one\nline.\n"},{"id":"445408","messageId":"20220104125534.wznwbkyxfcmyfqhb@fs","threadId":"57022","inReplyTo":"CAPig+cR93GyN53JoZbaiROrNtzGjiet7eTPQOk-26G+mB0KaCA@mail.gmail.com","subject":"Re: [PATCH v2] gpg-interface: trim CR from ssh-keygen","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2022-01-04T12:55:34Z","receivedAt":"2022-01-04T12:55:40Z","isPatch":true,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"On 03.01.2022 22:06, Eric Sunshine wrote:\n>On Mon, Jan 3, 2022 at 8:19 PM Junio C Hamano wrote:\n>> Eric Sunshine writes:\n>> > On Mon, Jan 3, 2022 at 6:34 PM Junio C Hamano wrote:\n>> >> Eric Sunshine writes:\n>> >> > On Mon, Jan 3, 2022 at 9:24 AM Fabian Stelzer wrote:\n>> >> >> - trust_size = strcspn(line, \"\\n\");\n>> >> >> + trust_size = strcspn(line, \"\\n\"); /* truncate at LF */\n>> >> >> + if (trust_size && trust_size != strlen(line) &&\n>> >> >> + line[trust_size - 1] == '\\r')\n>> >> >> + trust_size--; /* the LF was part of CRLF at the end */\n>> >> >\n>> >> > I may be misunderstanding, but isn't the strlen() unnecessary?\n>> >> >\n>> >> > if (trust_size && line[trust_size] &&\n>> >> > line[trust_size - 1] == '\\r')\n>> >> > trust_size--;\n>> >>\n>> >> That changes behaviour when \"line\" has more than one lines in it.\n>> >> strcspn() finds the first LF, and the posted patch ignores CRLF not\n>> >> at the end of line[]. Your variant feels more correct if the\n>> >> objective is to find the end of the first line (regardless of the\n>> >> choice of the end-of-line convention, either LF or CRLF) and omit\n>> >> the line terminator.\n>> >\n>> > Okay, that makes sense if that's the intention of the patch. Perhaps\n>> > the commit message should mention that `line` might contain multiple\n>> > lines and that it's only interested in the very last LF (unless it's\n>> > already obvious to everyone else, even though it wasn't to me).\n>>\n>> I do not think that is the case. strcspn(line, \"\\n\") will stop at\n>> the first one, so unless it is guaranteed that \"line\" has only one\n>> line in it, the patch as posted is not correct. Your variant\n>> without strlen() feels more correct, as I said.\n>\n>Okay, sorry for my unclear thinking. The existing code (before this\n>patch) does indeed seem to be interested only in the first line of\n>`line`, in which case I agree that the patch's use of strlen() does\n>not appear to be correct if `line` could ever contain more than one\n>line.\n\nI guess we need a bit more context for this patch to make sense:\n\nfor (line = ssh_principals_out.buf; *line;\n line = strchrnul(line + 1, '\\n')) {\n\twhile (*line == '\\n')\n\t\tline++;\n\tif (!*line)\n\t\tbreak;\n\n\ttrust_size = strcspn(line, \"\\n\"); /* truncate at LF */\n\tif (trust_size && trust_size != strlen(line) &&\n\t line[trust_size - 1] == '\\r')\n\t\ttrust_size--; /* the LF was part of CRLF at the end */\n\tprincipal = xmemdupz(line, trust_size);\n\nssh_principals_out contains the result of the find-principals call which \ncontains one found principal per line (normally LF, CRLF in some cygwin \nsetup).\n\nA principal can contain CR as a valid character. This is problematic if CR \nis the last char of the principal since we have no way of knowing then if we \nare in cygwin with CRLF line endings or another platform using LF and the CR \nis the last character of the principal.\nLets leave this rather weird edge case aside for now.\n\nSo what we want to do is split the buffer by line, no matter which line \nendings are used, and copy the principal without any line ending characters.\n\nThe `trust_size != strlen(line)` check was supposed to guard against `line` \nhaving no LF at all and ending with a CR. I think a\n`line[trust_size + 1] != '\\0'` would work as well.\n\nBut since this whole thing is already hard enough to follow i guess it's \nbetter we simply remove it instead of adding checks for the unlikely case we \nencounter a broken ssh-keygen. Especially since the effect would only be a \nfailed signature validation.\nWe could even remove the `if (trust_size)` condition since this only happens \nwhen `line` begins with LF which is already skipped over a few lines before. \nBut it's probably better to leave this in just in case the code changes.\n\nGenerally I think this is a common enough problem that there should be a \nfunction to split a strbuf by line no matter if LF or CRLF is used. Similar \nto strbuf_getline() but to read from a strbuf or maybe even handle this \nwithin pipe_command() when filling the strbuf. Maybe git even has better \ncode to handle this but i haven't found it yet?\n\n"},{"id":"445460","messageId":"xmqqo84rcn3j.fsf@gitster.g","threadId":"57022","inReplyTo":"20220104125534.wznwbkyxfcmyfqhb@fs","subject":"Re: [PATCH v2] gpg-interface: trim CR from ssh-keygen","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-04T19:33:36Z","receivedAt":"2022-01-04T19:33:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Fabian Stelzer writes:\n\n> I guess we need a bit more context for this patch to make sense:\n>\n> for (line = ssh_principals_out.buf; *line;\n> line = strchrnul(line + 1, '\\n')) {\n> \twhile (*line == '\\n')\n> \t\tline++;\n> \tif (!*line)\n> \t\tbreak;\n>\n> \ttrust_size = strcspn(line, \"\\n\"); /* truncate at LF */\n> \tif (trust_size && trust_size != strlen(line) &&\n> \t line[trust_size - 1] == '\\r')\n> \t\ttrust_size--; /* the LF was part of CRLF at the end */\n> \tprincipal = xmemdupz(line, trust_size);\n>\n> ssh_principals_out contains the result of the find-principals call\n> which contains one found principal per line (normally LF, CRLF in some\n> cygwin setup).\n\nAhh, OK. Sorry for being ultra lazy for not visiting the actual\nsource but just responding after reading only somebody else's\ncomments.\n\nSo, the code skips over one or more LFs (but users of platforms that\nuse CRLF line termination are screwed here already) to find the\nbeginning of a non-empty line. Then it wants to find the end of\nthat non-empty line (if there is still LF there in the buffer).\nSince strcspn() may not find any LF (i.e. it is an incomplete line),\nstrlen(line) is used to see if we found a LF or if we hit the\nterminating NUL. If the line ended with CR, we do not want to strip\nit.\n\nOK, so I was completely missing the idea. And I agree that it may\nbe a good idea to check how strcspn() returned to deal with an\nincomplete line, although as you hint later in the message I am\nresponding to, checking line[trust_size] would be a more obvious\nimplementation.\n\nIn any case, I think the earlier part of the loop is more confusing,\nand I think fixing that would naturally fix the trust_size\ncomputation. For example, wouldn't this easier to grok?\n\n\tconst char *next;\n\n\tfor (line = ssh_principals_out.buf;\n\t *line;\n\t line = next) {\n\t\tconst char *end_of_text;\n\n /* Find the terminating LF */\n \tnext = end_of_text = strchrnul(line, '\\n');\n\n\t\t/* Did we find a LF, and did we have CR before it? */\n\t\tif (*end_of_text &&\n line < end_of_text &&\n\t\t end_of_text[-1] == '\\r')\n\t\t\tend_of_text--;\n\n\t\t/* Unless we hit NUL, skip over the LF we found */\n\t\tif (*next)\n\t\t\tnext++;\n\n\t\t/* Not all lines are data. Skip empty ones */\n\t\tif (line == end_of_text)\n\t\t\t/* \n * You may want to allow skipping more than just\n\t\t\t * lines with 0-byte on them (e.g. comments?)\n\t\t\t * depending on the format you are reading.\n\t\t\t */\n\t\t\tcontinue;\n\n\t\t/* We now know we have an non-empty line. Process it */\n\t\tprincipal = xmemdupz(line, end_of_text - line);\n\t\t...\n\t}\n\t\t\nThe idea is to make sure that the place where the line ending\nconvention is taken care of is very isolated at the beginning of the\nloop.\n\nHmm?\n"},{"id":"445506","messageId":"CAPig+cQinNZp_2=eo7nokMCZ9gc-tAKO1V_jejL2Ei9J63tSDQ@mail.gmail.com","threadId":"57022","inReplyTo":"xmqqo84rcn3j.fsf@gitster.g","subject":"Re: [PATCH v2] gpg-interface: trim CR from ssh-keygen","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-01-05T07:09:55Z","receivedAt":"2022-01-05T07:10:13Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jan 4, 2022 at 2:33 PM Junio C Hamano wrote:\n> Fabian Stelzer writes:\n> > I guess we need a bit more context for this patch to make sense:\n> >\n> > for (line = ssh_principals_out.buf; *line;\n> > line = strchrnul(line + 1, '\\n')) {\n> > while (*line == '\\n')\n> > line++;\n> > if (!*line)\n> > break;\n> >\n> > trust_size = strcspn(line, \"\\n\"); /* truncate at LF */\n> > if (trust_size && trust_size != strlen(line) &&\n> > line[trust_size - 1] == '\\r')\n> > trust_size--; /* the LF was part of CRLF at the end */\n> > principal = xmemdupz(line, trust_size);\n>\n> Ahh, OK. Sorry for being ultra lazy for not visiting the actual\n> source but just responding after reading only somebody else's\n> comments.\n\nI'm also guilty of being lazy and not consulting the actual source. Sorry.\n\nFabian, thanks for all the extra context information.\n\n> OK, so I was completely missing the idea. And I agree that it may\n> be a good idea to check how strcspn() returned to deal with an\n> incomplete line, although as you hint later in the message I am\n> responding to, checking line[trust_size] would be a more obvious\n> implementation.\n>\n> In any case, I think the earlier part of the loop is more confusing,\n> and I think fixing that would naturally fix the trust_size\n> computation. For example, wouldn't this easier to grok?\n\nIndeed, the existing code is confusing me. I've been staring at it for\nseveral minutes and I think I'm still failing to understand the\npurpose of the +1 in the strchrnul() call. Perhaps I'm missing\nsomething obvious(?).\n\n> const char *next;\n>\n> for (line = ssh_principals_out.buf;\n> *line;\n> line = next) {\n> const char *end_of_text;\n>\n> /* Find the terminating LF */\n> next = end_of_text = strchrnul(line, '\\n');\n>\n> /* Did we find a LF, and did we have CR before it? */\n> if (*end_of_text &&\n> line < end_of_text &&\n> end_of_text[-1] == '\\r')\n> end_of_text--;\n\nIt took several seconds for me to convince myself that the -1 array\nindex was safe. Had the `line < end_of_text` condition been written\n`end_of_text > line`, I think it would have been immediately obvious,\nbut it's subjective, of course.\n\n> /* Unless we hit NUL, skip over the LF we found */\n> if (*next)\n> next++;\n>\n> /* Not all lines are data. Skip empty ones */\n> if (line == end_of_text)\n> /*\n> * You may want to allow skipping more than just\n> * lines with 0-byte on them (e.g. comments?)\n> * depending on the format you are reading.\n> */\n> continue;\n>\n> /* We now know we have an non-empty line. Process it */\n> principal = xmemdupz(line, end_of_text - line);\n> ...\n> }\n>\n> The idea is to make sure that the place where the line ending\n> convention is taken care of is very isolated at the beginning of the\n> loop.\n\nYes, this may be an improvement, though the cognitive load is still\nsomewhat high. Using one of the `split` functions from strbuf.h or\nstring-list.h might reduce the cognitive load significantly, even if\nthis code still needs to handle CR removal manually since none of the\n`split` functions are LF/CRLF agnostic. (Adding such a function might\nbe useful but could be outside the scope of this bug fix patch.)\n"},{"id":"445512","messageId":"20220105103611.upfmcrudw6n3ymx6@fs","threadId":"57022","inReplyTo":"CAPig+cQinNZp_2=eo7nokMCZ9gc-tAKO1V_jejL2Ei9J63tSDQ@mail.gmail.com","subject":"Re: [PATCH v2] gpg-interface: trim CR from ssh-keygen","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2022-01-05T10:36:11Z","receivedAt":"2022-01-05T10:36:19Z","isPatch":true,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"On 05.01.2022 02:09, Eric Sunshine wrote:\n>On Tue, Jan 4, 2022 at 2:33 PM Junio C Hamano wrote:\n>> Fabian Stelzer writes:\n>> > I guess we need a bit more context for this patch to make sense:\n>> >\n>> > for (line = ssh_principals_out.buf; *line;\n>> > line = strchrnul(line + 1, '\\n')) {\n>> > while (*line == '\\n')\n>> > line++;\n>> > if (!*line)\n>> > break;\n>> >\n>> > trust_size = strcspn(line, \"\\n\"); /* truncate at LF */\n>> > if (trust_size && trust_size != strlen(line) &&\n>> > line[trust_size - 1] == '\\r')\n>> > trust_size--; /* the LF was part of CRLF at the end */\n>> > principal = xmemdupz(line, trust_size);\n>>\n>> Ahh, OK. Sorry for being ultra lazy for not visiting the actual\n>> source but just responding after reading only somebody else's\n>> comments.\n>\n>I'm also guilty of being lazy and not consulting the actual source. Sorry.\n>\n>Fabian, thanks for all the extra context information.\n>\n>> OK, so I was completely missing the idea. And I agree that it may\n>> be a good idea to check how strcspn() returned to deal with an\n>> incomplete line, although as you hint later in the message I am\n>> responding to, checking line[trust_size] would be a more obvious\n>> implementation.\n>>\n>> In any case, I think the earlier part of the loop is more confusing,\n>> and I think fixing that would naturally fix the trust_size\n>> computation. For example, wouldn't this easier to grok?\n>\n>Indeed, the existing code is confusing me. I've been staring at it for\n>several minutes and I think I'm still failing to understand the\n>purpose of the +1 in the strchrnul() call. Perhaps I'm missing\n>something obvious(?).\n\nThis whole loop was basically copied from parse_gpg_output() above. Without \nthe +1 this would always find the same line in the buffer. The +1 skips over \nthe previously found LF.\n\n>\n>> const char *next;\n>>\n>> for (line = ssh_principals_out.buf;\n>> *line;\n>> line = next) {\n>> const char *end_of_text;\n>>\n>> /* Find the terminating LF */\n>> next = end_of_text = strchrnul(line, '\\n');\n>>\n>> /* Did we find a LF, and did we have CR before it? */\n>> if (*end_of_text &&\n>> line < end_of_text &&\n>> end_of_text[-1] == '\\r')\n>> end_of_text--;\n>\n>It took several seconds for me to convince myself that the -1 array\n>index was safe. Had the `line < end_of_text` condition been written\n>`end_of_text > line`, I think it would have been immediately obvious,\n>but it's subjective, of course.\n>\n>> /* Unless we hit NUL, skip over the LF we found */\n>> if (*next)\n>> next++;\n>>\n>> /* Not all lines are data. Skip empty ones */\n>> if (line == end_of_text)\n>> /*\n>> * You may want to allow skipping more than just\n>> * lines with 0-byte on them (e.g. comments?)\n>> * depending on the format you are reading.\n>> */\n>> continue;\n>>\n>> /* We now know we have an non-empty line. Process it */\n>> principal = xmemdupz(line, end_of_text - line);\n>> ...\n>> }\n>>\n>> The idea is to make sure that the place where the line ending\n>> convention is taken care of is very isolated at the beginning of the\n>> loop.\n>\n>Yes, this may be an improvement, though the cognitive load is still\n>somewhat high. Using one of the `split` functions from strbuf.h or\n>string-list.h might reduce the cognitive load significantly, even if\n>this code still needs to handle CR removal manually since none of the\n>`split` functions are LF/CRLF agnostic. (Adding such a function might\n>be useful but could be outside the scope of this bug fix patch.)\n\nHow about something like this:\n\nint string_find_line(char **line, size_t *len) {\n\tconst char *eol = NULL;\n\n\tif (*len > 0) {\n\t\t*line = *line + *len;\n\t\tif (**line && **line == '\\r')\n\t\t\t(*line)++;\n\t\tif (**line && **line == '\\n')\n\t\t\t(*line)++;\n\t}\n\n\tif (!**line)\n\t\treturn 0;\n\n\teol = strchrnul(*line, '\\n');\n\n\t/* Trim trailing CR from length */\n\tif (eol > *line && eol[-1] == '\\r')\n\t\teol--;\n\n\t*len = eol - *line;\n\treturn 1;\n}\n\nIts use would then simply be:\n\nchar *line = strbuf.buf;\nsize_t len = 0;\nwhile(string_find_line(&line,&len)) {\n\tif (!len)\n\t\tcontinue; /* Skip over empty lines */\n\tprincipal = xmemdupz(line, len);\n}\n\nNot sure about the name though.\nMaybe string_find_line() / _iterate_line / foreach_line ?\n\n"},{"id":"445576","messageId":"xmqqsfu1hq6x.fsf@gitster.g","threadId":"57022","inReplyTo":"20220105103611.upfmcrudw6n3ymx6@fs","subject":"Re: [PATCH v2] gpg-interface: trim CR from ssh-keygen","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-05T20:40:06Z","receivedAt":"2022-01-05T20:40:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Fabian Stelzer writes:\n\n> How about something like this:\n>\n> int string_find_line(char **line, size_t *len) {\n> \tconst char *eol = NULL;\n>\n> \tif (*len > 0) {\n> \t\t*line = *line + *len;\n> \t\tif (**line && **line == '\\r')\n> \t\t\t(*line)++;\n> \t\tif (**line && **line == '\\n')\n> \t\t\t(*line)++;\n> \t}\n>\n> \tif (!**line)\n> \t\treturn 0;\n>\n> \teol = strchrnul(*line, '\\n');\n>\n> \t/* Trim trailing CR from length */\n> \tif (eol > *line && eol[-1] == '\\r')\n> \t\teol--;\n>\n> \t*len = eol - *line;\n> \treturn 1;\n> }\n\nIt is a confusing piece of \"we handle one line at a time\" helper.\nIt is not obvious what the loop invariants are.\n\nIt would be most natural to readers if *line points at the very\nbeginning of the buffer, i.e. the beginning of the first line,\nand *len points at the very first character of that line, i.e. 0.\n\nBut then the first thing this function worries about is a case where\n*len is not 0. I obviously am biased, but sorry, I find what I gave\nyou 100 times simpler to understand.\n\n>\n> Its use would then simply be:\n>\n> char *line = strbuf.buf;\n> size_t len = 0;\n> while(string_find_line(&line,&len)) {\n> \tif (!len)\n> \t\tcontinue; /* Skip over empty lines */\n> \tprincipal = xmemdupz(line, len);\n> }\n>\n> Not sure about the name though.\n> Maybe string_find_line() / _iterate_line / foreach_line ?\n"},{"id":"445622","messageId":"20220106102603.cmb3rf4whd4hmfbb@fs","threadId":"57022","inReplyTo":"xmqqsfu1hq6x.fsf@gitster.g","subject":"Re: [PATCH v2] gpg-interface: trim CR from ssh-keygen","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2022-01-06T10:26:03Z","receivedAt":"2022-01-06T10:26:08Z","isPatch":true,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"On 05.01.2022 12:40, Junio C Hamano wrote:\n>Fabian Stelzer writes:\n>\n>> How about something like this:\n>>\n>> int string_find_line(char **line, size_t *len) {\n>> \tconst char *eol = NULL;\n>>\n>> \tif (*len > 0) {\n>> \t\t*line = *line + *len;\n>> \t\tif (**line && **line == '\\r')\n>> \t\t\t(*line)++;\n>> \t\tif (**line && **line == '\\n')\n>> \t\t\t(*line)++;\n>> \t}\n>>\n>> \tif (!**line)\n>> \t\treturn 0;\n>>\n>> \teol = strchrnul(*line, '\\n');\n>>\n>> \t/* Trim trailing CR from length */\n>> \tif (eol > *line && eol[-1] == '\\r')\n>> \t\teol--;\n>>\n>> \t*len = eol - *line;\n>> \treturn 1;\n>> }\n>\n>It is a confusing piece of \"we handle one line at a time\" helper.\n>It is not obvious what the loop invariants are.\n>\n>It would be most natural to readers if *line points at the very\n>beginning of the buffer, i.e. the beginning of the first line,\n>and *len points at the very first character of that line, i.e. 0.\n>\n>But then the first thing this function worries about is a case where\n>*len is not 0. I obviously am biased, but sorry, I find what I gave\n>you 100 times simpler to understand.\n>\n\nThere are a few more places where the same thing happens and text is just \nsplit by LF, ignoring CR. The gpg parsing where this code originated being \nthe most prominent example. However those just parse some parts from the \noutput and the worst that seems to happen is a trailing CR in some log \noutputs.\nIf we are ok with this then your version is indeed the better one. If we \nwant to correct the parsing at the other sites then I think a more \ngeneralized function would be better. Since the gpg stuff is in place for a \nlong time and no one complained we can probably leave it as is. I'll prepare \na new patch.\n\nThanks\n"},{"id":"445635","messageId":"xmqqpmp4eosr.fsf@gitster.g","threadId":"57022","inReplyTo":"20220106102603.cmb3rf4whd4hmfbb@fs","subject":"Re: [PATCH v2] gpg-interface: trim CR from ssh-keygen","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-06T17:50:44Z","receivedAt":"2022-01-06T17:50:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Fabian Stelzer writes:\n\n> There are a few more places where the same thing happens and text\n> is just split by LF, ignoring CR. The gpg parsing where this code\n> originated being the most prominent example. ... ... Since the\n> gpg stuff is in place for a long time and no one complained we can\n> probably leave it as is.\n\nYeah, that is the conclusion I was hoping for.\nThanks.\n"},{"id":"445692","messageId":"20220107090735.580225-1-fs@gigacodes.de","threadId":"57022","inReplyTo":"20220103095337.600536-1-fs@gigacodes.de","subject":"[PATCH v3] gpg-interface: trim CR from ssh-keygen","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2022-01-07T09:07:35Z","receivedAt":"2022-01-07T09:07:45Z","isPatch":true,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"We need to trim \\r from the output of 'ssh-keygen -Y find-principals' on\nWindows, or we end up calling 'ssh-keygen -Y verify' with a bogus signer\nidentity. ssh-keygen.c:2841 contains a call to puts(3), which confirms\nthis hypothesis. Signature verification passes with the fix.\n\nHelped-by: Pedro Martelletto \nSigned-off-by: Fabian Stelzer \n---\n gpg-interface.c | 34 ++++++++++++++++++++++++----------\n 1 file changed, 24 insertions(+), 10 deletions(-)\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex b52eb0e2e0..17b1e44baa 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -433,7 +433,6 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc,\n \tstruct tempfile *buffer_file;\n \tint ret = -1;\n \tconst char *line;\n-\tsize_t trust_size;\n \tchar *principal;\n \tstruct strbuf ssh_principals_out = STRBUF_INIT;\n \tstruct strbuf ssh_principals_err = STRBUF_INIT;\n@@ -502,15 +501,30 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc,\n \t\tret = -1;\n \t} else {\n \t\t/* Check every principal we found (one per line) */\n-\t\tfor (line = ssh_principals_out.buf; *line;\n-\t\t line = strchrnul(line + 1, '\\n')) {\n-\t\t\twhile (*line == '\\n')\n-\t\t\t\tline++;\n-\t\t\tif (!*line)\n-\t\t\t\tbreak;\n-\n-\t\t\ttrust_size = strcspn(line, \"\\n\");\n-\t\t\tprincipal = xmemdupz(line, trust_size);\n+\t\tconst char *next;\n+\t\tfor (line = ssh_principals_out.buf;\n+\t\t *line;\n+\t\t line = next) {\n+\t\t\tconst char *end_of_text;\n+\n+\t\t\tnext = end_of_text = strchrnul(line, '\\n');\n+\n+\t\t\t /* Did we find a LF, and did we have CR before it? */\n+\t\t\tif (*end_of_text &&\n+\t\t\t line < end_of_text &&\n+\t\t\t end_of_text[-1] == '\\r')\n+\t\t\t\tend_of_text--;\n+\n+\t\t\t/* Unless we hit NUL, skip over the LF we found */\n+\t\t\tif (*next)\n+\t\t\t\tnext++;\n+\n+\t\t\t/* Not all lines are data. Skip empty ones */\n+\t\t\tif (line == end_of_text)\n+\t\t\t\tcontinue;\n+\n+\t\t\t/* We now know we have an non-empty line. Process it */\n+\t\t\tprincipal = xmemdupz(line, end_of_text - line);\n \n \t\t\tchild_process_init(&ssh_keygen);\n \t\t\tstrbuf_release(&ssh_keygen_out);\n-- \n2.33.1\n\n"},{"id":"445814","messageId":"CAPig+cQMP_Ppg6uywAcFhaVqSoa71dD6UjXbUtC-bvK0WzJnZA@mail.gmail.com","threadId":"57022","inReplyTo":"20220105103611.upfmcrudw6n3ymx6@fs","subject":"Re: [PATCH v2] gpg-interface: trim CR from ssh-keygen","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-01-09T20:49:46Z","receivedAt":"2022-01-09T20:50:03Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jan 5, 2022 at 5:36 AM Fabian Stelzer wrote:\n> On 05.01.2022 02:09, Eric Sunshine wrote:\n> >> > line = strchrnul(line + 1, '\\n')) {\n> >> > while (*line == '\\n')\n> >> > line++;\n> >> > if (!*line)\n> >> > break;\n> >\n> >Indeed, the existing code is confusing me. I've been staring at it for\n> >several minutes and I think I'm still failing to understand the\n> >purpose of the +1 in the strchrnul() call. Perhaps I'm missing\n> >something obvious(?).\n>\n> This whole loop was basically copied from parse_gpg_output() above. Without\n> the +1 this would always find the same line in the buffer. The +1 skips over\n> the previously found LF.\n\nI still don't see the point of +1 in the strchrnul() call. After:\n\n line = strchrnul(line + 1, '\\n'))\n\n`line` is going to point either at '\\n' or at NUL. Then:\n\n while (*line == '\\n')\n line++;\n\nskips over the '\\n' if present. So, by the time the next loop\niteration starts, `line` will already be pointing past the '\\n' we\njust found, thus the +1 seems pointless (and maybe even buggy).\n\nBut perhaps I have a blind spot and am missing something obvious...\n"},{"id":"445815","messageId":"YdtVrT4gBvnXfNr6@flurp.local","threadId":"57022","inReplyTo":"20220107090735.580225-1-fs@gigacodes.de","subject":"Re: [PATCH v3] gpg-interface: trim CR from ssh-keygen","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-01-09T21:37:49Z","receivedAt":"2022-01-09T21:38:00Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Jan 07, 2022 at 10:07:35AM +0100, Fabian Stelzer wrote:\n> We need to trim \\r from the output of 'ssh-keygen -Y find-principals' on\n> Windows, or we end up calling 'ssh-keygen -Y verify' with a bogus signer\n> identity. ssh-keygen.c:2841 contains a call to puts(3), which confirms\n> this hypothesis. Signature verification passes with the fix.\n>\n> Helped-by: Pedro Martelletto \n> Signed-off-by: Fabian Stelzer \n\nShould this also have a \"Helped-by: Junio\" since this code was heavily\ninspired by his suggestion[1]?\n\n[1]: https://lore.kernel.org/git/xmqqo84rcn3j.fsf@gitster.g/\n\n> ---\n> diff --git a/gpg-interface.c b/gpg-interface.c\n> @@ -502,15 +501,30 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc,\n> +\t\tconst char *next;\n> +\t\tfor (line = ssh_principals_out.buf;\n> +\t\t *line;\n> +\t\t line = next) {\n> +\t\t\tconst char *end_of_text;\n> +\n> +\t\t\tnext = end_of_text = strchrnul(line, '\\n');\n> +\n> +\t\t\t /* Did we find a LF, and did we have CR before it? */\n> +\t\t\tif (*end_of_text &&\n> +\t\t\t line < end_of_text &&\n> +\t\t\t end_of_text[-1] == '\\r')\n> +\t\t\t\tend_of_text--;\n> +\n> +\t\t\t/* Unless we hit NUL, skip over the LF we found */\n> +\t\t\tif (*next)\n> +\t\t\t\tnext++;\n> +\n> +\t\t\t/* Not all lines are data. Skip empty ones */\n> +\t\t\tif (line == end_of_text)\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\t/* We now know we have an non-empty line. Process it */\n> +\t\t\tprincipal = xmemdupz(line, end_of_text - line);\n\nConsidering that this code makes a copy of the line _anyhow_ which it\nassigns to `principal`, it still seems like it would be simpler and\nfar easier to understand at-a-glance to instead take advantage of one\nof the existing string-splitting functions. For instance, something\nlike this:\n\n struct strbuf **line, **to_free;\n line = to_free = strbuf_split(&ssh_principals_out, '\\n');\n for (; *line; line++) {\n strbuf_trim_trailing_newline(*line);\n if (!(*line)->len)\n continue;\n principal = (*line)->buf;\n\nkeeping in mind that strbuf_trim_trailing_newline() takes care of\nCR/LF, and with appropriate cleanup at the end of the loop:\n\n strbuf_list_free(to_free);\n\n(and removal of `FREE_AND_NULL(principal)` which is no longer needed).\n\nSomething similar can be done with string_list_split(), as well.\n"},{"id":"445822","messageId":"20220110122859.3x37a7xdbmcsmuvm@fs","threadId":"57022","inReplyTo":"CAPig+cQMP_Ppg6uywAcFhaVqSoa71dD6UjXbUtC-bvK0WzJnZA@mail.gmail.com","subject":"Re: [PATCH v2] gpg-interface: trim CR from ssh-keygen","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2022-01-10T12:28:59Z","receivedAt":"2022-01-10T12:29:05Z","isPatch":true,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"On 09.01.2022 15:49, Eric Sunshine wrote:\n>On Wed, Jan 5, 2022 at 5:36 AM Fabian Stelzer wrote:\n>> On 05.01.2022 02:09, Eric Sunshine wrote:\n>> >> > line = strchrnul(line + 1, '\\n')) {\n>> >> > while (*line == '\\n')\n>> >> > line++;\n>> >> > if (!*line)\n>> >> > break;\n>> >\n>> >Indeed, the existing code is confusing me. I've been staring at it for\n>> >several minutes and I think I'm still failing to understand the\n>> >purpose of the +1 in the strchrnul() call. Perhaps I'm missing\n>> >something obvious(?).\n>>\n>> This whole loop was basically copied from parse_gpg_output() above. Without\n>> the +1 this would always find the same line in the buffer. The +1 skips over\n>> the previously found LF.\n>\n>I still don't see the point of +1 in the strchrnul() call. After:\n>\n> line = strchrnul(line + 1, '\\n'))\n>\n>`line` is going to point either at '\\n' or at NUL. Then:\n>\n> while (*line == '\\n')\n> line++;\n>\n>skips over the '\\n' if present. So, by the time the next loop\n>iteration starts, `line` will already be pointing past the '\\n' we\n>just found, thus the +1 seems pointless (and maybe even buggy).\n>\n>But perhaps I have a blind spot and am missing something obvious...\n\nHm, yeah. I think you are correct. The while below should make the +1 \nunnecessary. I think this never mattered to parse_gpg_output() since it is \nonly looking for the [GNUPG:] status line which probably comes first anyway. \nIf it does not then I think the loop will skip over it. Same thing with ssh \n(but we are changing this whole loop anyway)\n"},{"id":"445826","messageId":"20220110125901.apxqy7tzrc3edjwa@fs","threadId":"57022","inReplyTo":"YdtVrT4gBvnXfNr6@flurp.local","subject":"Re: [PATCH v3] gpg-interface: trim CR from ssh-keygen","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2022-01-10T12:59:01Z","receivedAt":"2022-01-10T13:00:42Z","isPatch":true,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"On 09.01.2022 16:37, Eric Sunshine wrote:\n>On Fri, Jan 07, 2022 at 10:07:35AM +0100, Fabian Stelzer wrote:\n>> We need to trim \\r from the output of 'ssh-keygen -Y find-principals' on\n>> Windows, or we end up calling 'ssh-keygen -Y verify' with a bogus signer\n>> identity. ssh-keygen.c:2841 contains a call to puts(3), which confirms\n>> this hypothesis. Signature verification passes with the fix.\n>>\n>> Helped-by: Pedro Martelletto \n>> Signed-off-by: Fabian Stelzer \n>\n>Should this also have a \"Helped-by: Junio\" since this code was heavily\n>inspired by his suggestion[1]?\n\nYeah, this should have a \"Written-by: Junio\" ^^\nI'm never sure when to add these headers (except the signed-off).\n\n>\n>[1]: https://lore.kernel.org/git/xmqqo84rcn3j.fsf@gitster.g/\n>\n>> ---\n>> diff --git a/gpg-interface.c b/gpg-interface.c\n>> @@ -502,15 +501,30 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc,\n>> +\t\tconst char *next;\n>> +\t\tfor (line = ssh_principals_out.buf;\n>> +\t\t *line;\n>> +\t\t line = next) {\n>> +\t\t\tconst char *end_of_text;\n>> +\n>> +\t\t\tnext = end_of_text = strchrnul(line, '\\n');\n>> +\n>> +\t\t\t /* Did we find a LF, and did we have CR before it? */\n>> +\t\t\tif (*end_of_text &&\n>> +\t\t\t line < end_of_text &&\n>> +\t\t\t end_of_text[-1] == '\\r')\n>> +\t\t\t\tend_of_text--;\n>> +\n>> +\t\t\t/* Unless we hit NUL, skip over the LF we found */\n>> +\t\t\tif (*next)\n>> +\t\t\t\tnext++;\n>> +\n>> +\t\t\t/* Not all lines are data. Skip empty ones */\n>> +\t\t\tif (line == end_of_text)\n>> +\t\t\t\tcontinue;\n>> +\n>> +\t\t\t/* We now know we have an non-empty line. Process it */\n>> +\t\t\tprincipal = xmemdupz(line, end_of_text - line);\n>\n>Considering that this code makes a copy of the line _anyhow_ which it\n>assigns to `principal`, it still seems like it would be simpler and\n>far easier to understand at-a-glance to instead take advantage of one\n>of the existing string-splitting functions. For instance, something\n>like this:\n>\n> struct strbuf **line, **to_free;\n> line = to_free = strbuf_split(&ssh_principals_out, '\\n');\n> for (; *line; line++) {\n> strbuf_trim_trailing_newline(*line);\n> if (!(*line)->len)\n> continue;\n> principal = (*line)->buf;\n>\n>keeping in mind that strbuf_trim_trailing_newline() takes care of\n>CR/LF, and with appropriate cleanup at the end of the loop:\n>\n> strbuf_list_free(to_free);\n>\n>(and removal of `FREE_AND_NULL(principal)` which is no longer needed).\n>\n>Something similar can be done with string_list_split(), as well.\n\nI agree that this is the most readable of the variants (and it works just as \nwell). Since in most cases there even will only ever be a single line of \noutput the extra work/allocation we might be doing with it is quite minimal.\n\nI have done something quite similar in get_default_ssh_signing_key() and got \na bit of negative feedback for it (being overkill for retrieving a single \nline) but ended up using it anyway.\n"},{"id":"445837","messageId":"xmqq8rvn34mf.fsf@gitster.g","threadId":"57022","inReplyTo":"YdtVrT4gBvnXfNr6@flurp.local","subject":"Re: [PATCH v3] gpg-interface: trim CR from ssh-keygen","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-10T17:03:20Z","receivedAt":"2022-01-10T17:03:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> of the existing string-splitting functions. For instance, something\n> like this:\n>\n> struct strbuf **line, **to_free;\n> line = to_free = strbuf_split(&ssh_principals_out, '\\n');\n> for (; *line; line++) {\n> strbuf_trim_trailing_newline(*line);\n> if (!(*line)->len)\n> continue;\n> principal = (*line)->buf;\n>\n> keeping in mind that strbuf_trim_trailing_newline() takes care of\n> CR/LF, and with appropriate cleanup at the end of the loop:\n>\n> strbuf_list_free(to_free);\n>\n> (and removal of `FREE_AND_NULL(principal)` which is no longer needed).\n>\n> Something similar can be done with string_list_split(), as well.\n\nUnless you are writing an interactive text editor, an array of\nlines, each of which can individually be manupulated cheaply when\ninserting or deleting a span of chars, is a way too ugly and overly\nexpensive data structure to keep your data in the long haul. In\nshort, strbuf_split() was a mistaken piece of API that does not\nbelong to this project ;-)\n\nThe cycles spent by crypto before getting to this point in the code\nis expensive enough that the extra cycles to separately scan to\nsplit them into lines and another scan from the end of the each line\nto trim may not matter, so I'd stop at saying \"I'd rather not to see\nthe above code\" instead of my usual \"Please don't\", from performance\nperspective in this case.\n\nBut from code cleanliness perspective, well, let me just say that\nthis is not Python or Java but a C project.\n\n\n"},{"id":"445846","messageId":"xmqqczkz1ntt.fsf@gitster.g","threadId":"57022","inReplyTo":"20220110125901.apxqy7tzrc3edjwa@fs","subject":"Re: [PATCH v3] gpg-interface: trim CR from ssh-keygen","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-10T17:51:26Z","receivedAt":"2022-01-10T17:51:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Fabian Stelzer writes:\n\n> On 09.01.2022 16:37, Eric Sunshine wrote:\n>>On Fri, Jan 07, 2022 at 10:07:35AM +0100, Fabian Stelzer wrote:\n>>> We need to trim \\r from the output of 'ssh-keygen -Y find-principals' on\n>>> Windows, or we end up calling 'ssh-keygen -Y verify' with a bogus signer\n>>> identity. ssh-keygen.c:2841 contains a call to puts(3), which confirms\n>>> this hypothesis. Signature verification passes with the fix.\n>>>\n>>> Helped-by: Pedro Martelletto \n>>> Signed-off-by: Fabian Stelzer \n>>\n>>Should this also have a \"Helped-by: Junio\" since this code was heavily\n>>inspired by his suggestion[1]?\n>\n> Yeah, this should have a \"Written-by: Junio\" ^^\n> I'm never sure when to add these headers (except the signed-off).\n\nHeh, helped-by might be OK but I certainly didn't write it. I\nmerely translated what you wrote without knowing exactly what's\non these lines (and what I knew, like the lines are the unit of\nprocessing and empty lines are to be skipped, I all learned from\nyour original without knowing why) ;-)\n\nSo if anything, Helped-by: is good enough, but I do not need more\ncredit or blame ;-)\n"}]}