{"thread":{"id":"58710","subject":"Git Bug Report: out of memory using git tag","startedAt":"2022-10-28T22:31:40Z","lastAt":"2022-11-03T04:33:45Z","messageCount":15,"participants":["Martin Englund","Jeff King","Philippe Blain","Elijah Newren","Eric Sunshine","Ævar Arnfjörð Bjarmason","Taylor Blau"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"465990","messageId":"CABYbkvP=fMmaFUD3bQbeQ-XKiMSP6g-u0p7Vq1Qt_K5=D5WJ+A@mail.gmail.com","threadId":"58710","inReplyTo":null,"subject":"Git Bug Report: out of memory using git tag","fromName":"Martin Englund","fromEmail":"martin@englund.nu","sentAt":"2022-10-28T22:29:33Z","receivedAt":"2022-10-28T22:31:40Z","isPatch":false,"sender":{"key":"martin@englund.nu","avatar":null},"body":"Thank you for filling out a Git bug report!\nPlease answer the following questions to help us understand your issue.\n\nWhat did you do before the bug happened? (Steps to reproduce your issue)\nI created a signed tag (git tag -s) using a ssh-agent key and then ran\ngit tag -l --format '%(contents:body)' v0.6.1\n\nWhat did you expect to happen? (Expected behavior)\nI get the output\n\nWhat happened instead? (Actual behavior)\nfatal: Out of memory, malloc failed (tried to allocate\n18446744073709551266 bytes)\n\nWhat's different between what you expected and what actually happened?\ngit tries to allocate an unreasonable amount of memory\n\nAnything else you want to add:\nI don't have 18,000 PB of memory\n\nPlease review the rest of the bug report below.\nYou can delete any lines you don't wish to share.\n\n\n[System Info]\ngit version:\ngit version 2.38.1.280.g63bba4fdd8\ncpu: x86_64\nbuilt from commit: 63bba4fdd86d80ef061c449daa97a981a9be0792\nsizeof-long: 8\nsizeof-size_t: 8\nshell-path: /bin/sh\nuname: Linux 5.15.0-1021-aws #25~20.04.1-Ubuntu SMP Thu Sep 22\n13:59:08 UTC 2022 x86_64\ncompiler info: gnuc: 9.4\nlibc info: glibc: 2.31\n$SHELL (typically, interactive shell): /bin/bash\n\nCheers,\n/Martin\n-- \nMartin Englund, martin@englund.nu / GnuPG key: FE91E717\nhttp://blog.englund.nu/\n"},{"id":"466198","messageId":"Y2EPgICMsmzKvCC8@coredump.intra.peff.net","threadId":"58710","inReplyTo":"CABYbkvP=fMmaFUD3bQbeQ-XKiMSP6g-u0p7Vq1Qt_K5=D5WJ+A@mail.gmail.com","subject":"Re: Git Bug Report: out of memory using git tag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-01T12:22:24Z","receivedAt":"2022-11-01T12:22:29Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 28, 2022 at 03:29:33PM -0700, Martin Englund wrote:\n\n> What did you do before the bug happened? (Steps to reproduce your issue)\n> I created a signed tag (git tag -s) using a ssh-agent key and then ran\n> git tag -l --format '%(contents:body)' v0.6.1\n> \n> What did you expect to happen? (Expected behavior)\n> I get the output\n> \n> What happened instead? (Actual behavior)\n> fatal: Out of memory, malloc failed (tried to allocate\n> 18446744073709551266 bytes)\n\nThanks for the report. This looks like pointer or size_t arithmetic that\nhas gone negative. Here's a minimal reproduction:\n\n  {\n    echo subject\n    echo \"-----BEGIN PGP SIGNATURE-----\"\n  } | git tag -F - foo\n  git tag -l --format='%(contents:body)' foo\n\nThe issue isn't unique to pgp signatures; the problem is in the parsing\ndone by ref-filter's find_subpos(), so any signature type exhibits the\nproblem. At the end of that function we do:\n\n      *nonsiglen = sigstart - buf;\n\nbut \"buf\" has moved beyond \"sigstart\". Presumably because it uses\nstrstr() to look for end-of-line in buf. Since there isn't one before\nthe signature begins, we go to the end of the signature.\n\nThe bug bisects to 9f75ce3d8f (ref-filter: handle CRLF at end-of-line\nmore gracefully, 2020-10-29). Before then, I think our loop was careful\nabout moving past the start of the signature. Author cc'd.\n\n-Peff\n"},{"id":"466271","messageId":"38d50c30-c6a3-5989-6e01-47c5467d9d6b@gmail.com","threadId":"58710","inReplyTo":"Y2EPgICMsmzKvCC8@coredump.intra.peff.net","subject":"Re: Git Bug Report: out of memory using git tag","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2022-11-02T00:41:55Z","receivedAt":"2022-11-02T00:42:01Z","isPatch":false,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi,\n\nLe 2022-11-01 à 08:22, Jeff King a écrit :\n> On Fri, Oct 28, 2022 at 03:29:33PM -0700, Martin Englund wrote:\n> \n>> What did you do before the bug happened? (Steps to reproduce your issue)\n>> I created a signed tag (git tag -s) using a ssh-agent key and then ran\n>> git tag -l --format '%(contents:body)' v0.6.1\n>>\n>> What did you expect to happen? (Expected behavior)\n>> I get the output\n>>\n>> What happened instead? (Actual behavior)\n>> fatal: Out of memory, malloc failed (tried to allocate\n>> 18446744073709551266 bytes)\n> \n> Thanks for the report. This looks like pointer or size_t arithmetic that\n> has gone negative. Here's a minimal reproduction:\n> \n>   {\n>     echo subject\n>     echo \"-----BEGIN PGP SIGNATURE-----\"\n>   } | git tag -F - foo\n>   git tag -l --format='%(contents:body)' foo\n> \n> The issue isn't unique to pgp signatures; the problem is in the parsing\n> done by ref-filter's find_subpos(), so any signature type exhibits the\n> problem. At the end of that function we do:\n> \n>       *nonsiglen = sigstart - buf;\n> \n> but \"buf\" has moved beyond \"sigstart\". Presumably because it uses\n> strstr() to look for end-of-line in buf. Since there isn't one before\n> the signature begins, we go to the end of the signature.\n> \n> The bug bisects to 9f75ce3d8f (ref-filter: handle CRLF at end-of-line\n> more gracefully, 2020-10-29). Before then, I think our loop was careful\n> about moving past the start of the signature. Author cc'd.\n> \n\nThanks for letting me know, and for the simple reproducer, that's very useful.\nI'll try to find time to fix that this week, but can't promise anything.\n\nPhil.\n"},{"id":"466272","messageId":"baac9c67-3f12-18f3-38ff-cd7e450a4da8@gmail.com","threadId":"58710","inReplyTo":"Y2EPgICMsmzKvCC8@coredump.intra.peff.net","subject":"Re: Git Bug Report: out of memory using git tag","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2022-11-02T00:42:06Z","receivedAt":"2022-11-02T00:42:12Z","isPatch":false,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi,\n\nLe 2022-11-01 à 08:22, Jeff King a écrit :\n> On Fri, Oct 28, 2022 at 03:29:33PM -0700, Martin Englund wrote:\n> \n>> What did you do before the bug happened? (Steps to reproduce your issue)\n>> I created a signed tag (git tag -s) using a ssh-agent key and then ran\n>> git tag -l --format '%(contents:body)' v0.6.1\n>>\n>> What did you expect to happen? (Expected behavior)\n>> I get the output\n>>\n>> What happened instead? (Actual behavior)\n>> fatal: Out of memory, malloc failed (tried to allocate\n>> 18446744073709551266 bytes)\n> \n> Thanks for the report. This looks like pointer or size_t arithmetic that\n> has gone negative. Here's a minimal reproduction:\n> \n>   {\n>     echo subject\n>     echo \"-----BEGIN PGP SIGNATURE-----\"\n>   } | git tag -F - foo\n>   git tag -l --format='%(contents:body)' foo\n> \n> The issue isn't unique to pgp signatures; the problem is in the parsing\n> done by ref-filter's find_subpos(), so any signature type exhibits the\n> problem. At the end of that function we do:\n> \n>       *nonsiglen = sigstart - buf;\n> \n> but \"buf\" has moved beyond \"sigstart\". Presumably because it uses\n> strstr() to look for end-of-line in buf. Since there isn't one before\n> the signature begins, we go to the end of the signature.\n> \n> The bug bisects to 9f75ce3d8f (ref-filter: handle CRLF at end-of-line\n> more gracefully, 2020-10-29). Before then, I think our loop was careful\n> about moving past the start of the signature. Author cc'd.\n> \n\nThanks for letting me know, and for the simple reproducer, that's very useful.\nI'll try to find time to fix that this week, but can't promise anything.\n\nPhil.\n"},{"id":"466280","messageId":"Y2IeqOT5Ao1Qa0Zl@coredump.intra.peff.net","threadId":"58710","inReplyTo":"38d50c30-c6a3-5989-6e01-47c5467d9d6b@gmail.com","subject":"Re: Git Bug Report: out of memory using git tag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-02T07:39:20Z","receivedAt":"2022-11-02T07:39:27Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 01, 2022 at 08:41:55PM -0400, Philippe Blain wrote:\n\n> > The issue isn't unique to pgp signatures; the problem is in the parsing\n> > done by ref-filter's find_subpos(), so any signature type exhibits the\n> > problem. At the end of that function we do:\n> > \n> >       *nonsiglen = sigstart - buf;\n> > \n> > but \"buf\" has moved beyond \"sigstart\". Presumably because it uses\n> > strstr() to look for end-of-line in buf. Since there isn't one before\n> > the signature begins, we go to the end of the signature.\n> > \n> > The bug bisects to 9f75ce3d8f (ref-filter: handle CRLF at end-of-line\n> > more gracefully, 2020-10-29). Before then, I think our loop was careful\n> > about moving past the start of the signature. Author cc'd.\n> > \n> \n> Thanks for letting me know, and for the simple reproducer, that's very useful.\n> I'll try to find time to fix that this week, but can't promise anything.\n\nAfter sleeping on it, I think I fully understand what's going on. There\nare actually _two_ bugs, but they are closely related. ;)\n\nHere are patches which fix them both. I may be setting a new record for\nthe ratio of commit message lines to changed code. But it took me a\nwhile to figure out what was going on, so I wanted to explain it fully.\n\n  [1/2]: ref-filter: fix parsing of signatures without blank lines\n  [2/2]: ref-filter: fix parsing of signatures with CRLF and no body\n\n ref-filter.c            |  8 ++++----\n t/t6300-for-each-ref.sh | 40 ++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 44 insertions(+), 4 deletions(-)\n\n-Peff\n"},{"id":"466281","messageId":"Y2IfT0UFJ7H/mHMz@coredump.intra.peff.net","threadId":"58710","inReplyTo":"Y2IeqOT5Ao1Qa0Zl@coredump.intra.peff.net","subject":"[PATCH 1/2] ref-filter: fix parsing of signatures without blank lines","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-02T07:42:07Z","receivedAt":"2022-11-02T07:42:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When ref-filter is asked to show %(content:subject), etc, we end up in\nfind_subpos() to parse out the three major parts: the subject, the body,\nand the signature (if any).\n\nWhen searching for the blank line between the subject and body, if we\ndon't find anything, we try to treat the whole message as the subject,\nwith no body. But our idea of \"the whole message\" needs to take into\naccount the signature, too. Since 9f75ce3d8f (ref-filter: handle CRLF at\nend-of-line more gracefully, 2020-10-29), the code instead goes all the\nway to the end of the buffer, which produces confusing output.\n\nHere's an example. If we have a tag message like this:\n\n  this is the subject\n  -----BEGIN SSH SIGNATURE-----\n  ...some stuff...\n  -----END SSH SIGNATURE-----\n\nthen the current parser will put the start of the body at the end of the\nwhole buffer. This produces two buggy outcomes:\n\n  - since the subject length is computed as (body - subject), showing\n    %(contents:subject) will print both the subject and the signature,\n    rather than just the single line\n\n  - since the body length is computed as (sig - body), and the body now\n    starts _after_ the signature, we end up with a negative length!\n    Fortunately we never access out-of-bounds memory, because the\n    negative length is fed to xmemdupz(), which casts it to a size_t,\n    and xmalloc() bails trying to allocate an absurdly large value.\n\n    In theory it would be possible for somebody making a malicious tag\n    to wrap it around to a more reasonable value, but it would require a\n    tag on the order of 2^63 bytes. And even if they did, all they get\n    is an out of bounds string read. So the security implications are\n    probably not interesting.\n\nWe can fix both by correctly putting the start of the body at the same\nindex as the start of the signature (effectively making the body empty).\n\nNote that this is a real issue with signatures generated with gpg.format\nset to \"ssh\", which would look like the example above. In the new tests\nhere I use a hard-coded tag message, for a few reasons:\n\n  - regardless of what the ssh-signing code produces now or in the\n    future, we should be testing this particular case\n\n  - skipping the actual signature makes the tests simpler to write (and\n    allows them to run on more systems)\n\n  - t6300 has helpers for working with gpg signatures; for the purposes\n    of this bug, \"BEGIN PGP\" is just as good a demonstration, and this\n    simplifies the tests\n\nCuriously, the same issue doesn't happen with real gpg signatures (and\nthere are even existing tests in t6300 with cover this). Those have a\nblank line between the header and the content, like:\n\n  this is the subject\n  -----BEGIN PGP SIGNATURE-----\n\n  ...some stuff...\n  -----END PGP SIGNATURE-----\n\nBecause we search for the subject/body separator line with a strstr(),\nwe find the blank line in the signature, even though it's outside of\nwhat we'd consider the body. But that puts us unto a separate code path,\nwhich realizes that we're now in the signature and adjusts the line back\nto \"sigstart\". So this patch is basically just making the \"no line found\nat all\" case match that. And note that \"sigstart\" is always defined (if\nthere is no signature, it points to the end of the buffer as you'd\nexpect).\n\nReported-by: Martin Englund <martin@englund.nu>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n ref-filter.c            |  2 +-\n t/t6300-for-each-ref.sh | 15 +++++++++++++++\n 2 files changed, 16 insertions(+), 1 deletion(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 914908fac5..6c2148c01e 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1380,7 +1380,7 @@ static void find_subpos(const char *buf,\n \t/* check if message uses CRLF */\n \t} else if (! (eol = strstr(*sub, \"\\r\\n\\r\\n\"))) {\n \t\t/* treat whole message as subject */\n-\t\teol = strrchr(*sub, '\\0');\n+\t\teol = sigstart;\n \t}\n \tbuf = eol;\n \t*sublen = buf - *sub;\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex dcaab7265f..d7e70027e6 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -1406,4 +1406,19 @@ test_expect_success 'for-each-ref reports broken tags' '\n \t\trefs/tags/broken-tag-*\n '\n \n+test_expect_success 'set up tag with signature and no blank lines' '\n+\tgit tag -F - fake-sig-no-blanks <<-\\EOF\n+\tthis is the subject\n+\t-----BEGIN PGP SIGNATURE-----\n+\tnot a real signature, but we just care about the\n+\tsubject/body parsing. It is important here that\n+\tthere are no blank lines in the signature.\n+\t-----END PGP SIGNATURE-----\n+\tEOF\n+'\n+\n+test_atom refs/tags/fake-sig-no-blanks contents:subject 'this is the subject'\n+test_atom refs/tags/fake-sig-no-blanks contents:body ''\n+test_atom refs/tags/fake-sig-no-blanks contents:signature \"$sig\"\n+\n test_done\n-- \n2.38.1.677.g4206adeb26\n\n"},{"id":"466282","messageId":"Y2IfwL96Ku/dGuJR@coredump.intra.peff.net","threadId":"58710","inReplyTo":"Y2IeqOT5Ao1Qa0Zl@coredump.intra.peff.net","subject":"[PATCH 2/2] ref-filter: fix parsing of signatures with CRLF and no body","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-02T07:44:00Z","receivedAt":"2022-11-02T07:44:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This commit fixes a bug when parsing tags that have CRLF line endings, a\nsignature, and no body, like this (the \"^M\" are marking the CRs):\n\n  this is the subject^M\n  -----BEGIN PGP SIGNATURE-----^M\n  ^M\n  ...some stuff...^M\n  -----END PGP SIGNATURE-----^M\n\nWhen trying to find the start of the body, we look for a blank line\nseparating the subject and body. In this case, there isn't one. But we\nsearch for it using strstr(), which will find the blank line in the\nsignature.\n\nIn the non-CRLF code path, we check whether the line we found is past\nthe start of the signature, and if so, put the body pointer at the start\nof the signature (effectively making the body empty). But the CRLF code\npath doesn't catch the same case, and we end up with the body pointer in\nthe middle of the signature field. This has two visible problems:\n\n  - printing %(contents:subject) will show part of the signature, too,\n    since the subject length is computed as (body - subject)\n\n  - the length of the body is (sig - body), which makes it negative.\n    Asking for %(contents:body) causes us to cast this to a very large\n    size_t when we feed it to xmemdupz(), which then complains about\n    trying to allocate too much memory.\n\nThese are essentially the same bugs fixed in the previous commit, except\nthat they happen when there is a CRLF blank line in the signature,\nrather than no blank line at all. Both are caused by the refactoring in\n9f75ce3d8f (ref-filter: handle CRLF at end-of-line more gracefully,\n2020-10-29).\n\nWe can fix this by doing the same \"sigstart\" check that we do in the\nnon-CRLF case. And rather than repeat ourselves, we can just use\nshort-circuiting OR to collapse both cases into a single conditional.\nI.e., rather than:\n\n  if (strstr(\"\\n\\n\"))\n    ...found blank, check if it's in signature...\n  else if (strstr(\"\\r\\n\\r\\n\"))\n    ...found blank, check if it's in signature...\n  else\n    ...no blank line found...\n\nwe can collapse this to:\n\n  if (strstr(\"\\n\\n\")) ||\n      strstr(\"\\r\\n\\r\\n\")))\n    ...found blank, check if it's in signature...\n  else\n    ...no blank line found...\n\nThe tests show the problem and the fix. Though it wasn't broken, I\nincluded contents:signature here to make sure it still behaves as\nexpected, but note the shell hackery needed to make it work. A\nless-clever option would be to skip using test_atom and just \"append_cr\n>expected\" ourselves.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n ref-filter.c            |  6 +++---\n t/t6300-for-each-ref.sh | 25 +++++++++++++++++++++++++\n 2 files changed, 28 insertions(+), 3 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 6c2148c01e..9dc2cd1451 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1375,10 +1375,10 @@ static void find_subpos(const char *buf,\n \t/* subject is first non-empty line */\n \t*sub = buf;\n \t/* subject goes to first empty line before signature begins */\n-\tif ((eol = strstr(*sub, \"\\n\\n\"))) {\n+\tif ((eol = strstr(*sub, \"\\n\\n\")) ||\n+\t    (eol = strstr(*sub, \"\\r\\n\\r\\n\"))) {\n \t\teol = eol < sigstart ? eol : sigstart;\n-\t/* check if message uses CRLF */\n-\t} else if (! (eol = strstr(*sub, \"\\r\\n\\r\\n\"))) {\n+\t} else {\n \t\t/* treat whole message as subject */\n \t\teol = sigstart;\n \t}\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex d7e70027e6..fa38b87441 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -1421,4 +1421,29 @@ test_atom refs/tags/fake-sig-no-blanks contents:subject 'this is the subject'\n test_atom refs/tags/fake-sig-no-blanks contents:body ''\n test_atom refs/tags/fake-sig-no-blanks contents:signature \"$sig\"\n \n+test_expect_success 'set up tag with CRLF signature' '\n+\tappend_cr <<-\\EOF |\n+\tthis is the subject\n+\t-----BEGIN PGP SIGNATURE-----\n+\n+\tnot a real signature, but we just care about\n+\tthe subject/body parsing. It is important here\n+\tthat there is a blank line separating this\n+\tfrom the signature header.\n+\t-----END PGP SIGNATURE-----\n+\tEOF\n+\tgit tag -F - --cleanup=verbatim fake-sig-crlf\n+'\n+\n+test_atom refs/tags/fake-sig-crlf contents:subject 'this is the subject'\n+test_atom refs/tags/fake-sig-crlf contents:body ''\n+\n+# CRLF is retained in the signature, so we have to pass our expected value\n+# through append_cr. But test_atom requires a shell string, which means command\n+# substitution, and the shell will strip trailing newlines from the output of\n+# the substitution. Hack around it by adding and then removing a dummy line.\n+sig_crlf=\"$(printf \"%s\" \"$sig\" | append_cr; echo dummy)\"\n+sig_crlf=${sig_crlf%dummy}\n+test_atom refs/tags/fake-sig-crlf contents:signature \"$sig_crlf\"\n+\n test_done\n-- \n2.38.1.677.g4206adeb26\n"},{"id":"466294","messageId":"CABPp-BGmNqV5H5meMggKYTcTJDxxt5dRT3rZRnRPz496OJn3Vg@mail.gmail.com","threadId":"58710","inReplyTo":"Y2IeqOT5Ao1Qa0Zl@coredump.intra.peff.net","subject":"Re: Git Bug Report: out of memory using git tag","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-11-02T08:14:59Z","receivedAt":"2022-11-02T08:15:27Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Nov 2, 2022 at 12:51 AM Jeff King <peff@peff.net> wrote:\n>\n> Here are patches which fix them both. I may be setting a new record for\n> the ratio of commit message lines to changed code\n\nIt looks like the first patch is 72 lines of commit message for a\none-line fix, and the second patch is 61 lines of commit message for a\ntwo line fix.\n\nI don't know what the record ratio is, but it's at least 96[1], so\nclearly you'll need to figure out how to pad your first commit message\nwith at least another 25 lines before this series can be accepted.\n;-)\n\n[1] https://lore.kernel.org/git/7vaac73265.fsf@alter.siamese.dyndns.org/\n"},{"id":"466296","messageId":"CAPig+cRkHyjoBNEbuUB9+MXnGMRD3W_ynpX4Gco16Y5dVJN=eg@mail.gmail.com","threadId":"58710","inReplyTo":"Y2IeqOT5Ao1Qa0Zl@coredump.intra.peff.net","subject":"Re: Git Bug Report: out of memory using git tag","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-11-02T08:24:20Z","receivedAt":"2022-11-02T08:24:40Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Nov 2, 2022 at 3:44 AM Jeff King <peff@peff.net> wrote:\n> After sleeping on it, I think I fully understand what's going on. There\n> are actually _two_ bugs, but they are closely related. ;)\n>\n> Here are patches which fix them both. I may be setting a new record for\n> the ratio of commit message lines to changed code. But it took me a\n> while to figure out what was going on, so I wanted to explain it fully.\n\nWell explained. For someone who has (probably) never looked at that\ncode (me), the explanations made perfect sense.\n\n(Oh, and I didn't even have to report any s///.)\n"},{"id":"466299","messageId":"Y2I0siBlVOngNUtK@coredump.intra.peff.net","threadId":"58710","inReplyTo":"CABPp-BGmNqV5H5meMggKYTcTJDxxt5dRT3rZRnRPz496OJn3Vg@mail.gmail.com","subject":"gigantic commit messages, was Re: Git Bug Report: out of memory using git tag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-02T09:13:22Z","receivedAt":"2022-11-02T09:15:38Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 02, 2022 at 01:14:59AM -0700, Elijah Newren wrote:\n\n> On Wed, Nov 2, 2022 at 12:51 AM Jeff King <peff@peff.net> wrote:\n> >\n> > Here are patches which fix them both. I may be setting a new record for\n> > the ratio of commit message lines to changed code\n> \n> It looks like the first patch is 72 lines of commit message for a\n> one-line fix, and the second patch is 61 lines of commit message for a\n> two line fix.\n> \n> I don't know what the record ratio is, but it's at least 96[1], so\n> clearly you'll need to figure out how to pad your first commit message\n> with at least another 25 lines before this series can be accepted.\n> ;-)\n\nWell, if we want to start digging things up... ;)\n\nTry this:\n\n  git log --no-merges --no-renames --format='%H %B' -z --numstat '*.c' |\n  perl -0ne '\n    chomp;\n    if (s/^([0-9a-f]{40}) //) {\n      if (defined $commit && $diff) {\n        my $ratio = $body / $diff;\n        print \"$ratio $body $diff $commit\\n\";\n      }\n      $commit = $1;\n      $body = () = /\\n/g;\n      $diff = 0;\n    } elsif (/^\\s*(\\d+)\\t/) {\n      # this counts only added lines, under the assumption that\n      # small commits generally remove/add in proportion. Of course\n      # ones that _only_ remove lines have infinite ratios.\n      $diff += $1;\n    } else {\n      die \"confusing record: $_\\n\";\n    }\n  ' |\n  sort -rn |\n  head\n\nwhich shows there are a few in the 100's. Pipe through:\n\n  awk '{print $4}' |\n  git log --stdin --no-walk=unsorted --stat\n\nfor a nicer view. I'm rejecting the top one on the grounds that it's\nmostly cut-and-paste output, and also that #2 is mine. ;)\n\n-Peff\n"},{"id":"466307","messageId":"49e1eec2-0567-f75e-7d0d-0f2d00ac472c@gmail.com","threadId":"58710","inReplyTo":"Y2IeqOT5Ao1Qa0Zl@coredump.intra.peff.net","subject":"Re: Git Bug Report: out of memory using git tag","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2022-11-02T12:13:27Z","receivedAt":"2022-11-02T12:13:37Z","isPatch":false,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi Peff,\n\nLe 2022-11-02 à 03:39, Jeff King a écrit :\n> \n> After sleeping on it, I think I fully understand what's going on. There\n> are actually _two_ bugs, but they are closely related. ;)\n> \n> Here are patches which fix them both. I may be setting a new record for\n> the ratio of commit message lines to changed code. But it took me a\n> while to figure out what was going on, so I wanted to explain it fully.\n> \n>   [1/2]: ref-filter: fix parsing of signatures without blank lines\n>   [2/2]: ref-filter: fix parsing of signatures with CRLF and no body\n\nI read both patches and I concur with Eric, very well explained. I'm sorry\nfor letting that split through at the time; thanks a lot for the fixes.\nI'm sure it took you less time that it would have taken me!\n\nThe new code is even clearer with this additional 'else if' removed.\n\nOne thing I think that is not mentioned in your commit messages is that 1/2\nwould also apply to a CRLF-using tag with no blank lines, i.e.\n\n  this is the subject^M\n  -----BEGIN PGP SIGNATURE-----^M\n  ...some stuff...^M\n  -----END PGP SIGNATURE-----^M\n\nParsing this tag correctly is fixed by 1/2, right?\n\nAnyway thanks again for the fixes,\n\nPhilippe.\n"},{"id":"466315","messageId":"221102.86pme52z8d.gmgdl@evledraar.gmail.com","threadId":"58710","inReplyTo":"Y2I0siBlVOngNUtK@coredump.intra.peff.net","subject":"Re: gigantic commit messages, was Re: Git Bug Report: out of memory using git tag","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-02T14:26:20Z","receivedAt":"2022-11-02T14:43:54Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Nov 02 2022, Jeff King wrote:\n\n> On Wed, Nov 02, 2022 at 01:14:59AM -0700, Elijah Newren wrote:\n>\n>> On Wed, Nov 2, 2022 at 12:51 AM Jeff King <peff@peff.net> wrote:\n>> >\n>> > Here are patches which fix them both. I may be setting a new record for\n>> > the ratio of commit message lines to changed code\n>> \n>> It looks like the first patch is 72 lines of commit message for a\n>> one-line fix, and the second patch is 61 lines of commit message for a\n>> two line fix.\n>> \n>> I don't know what the record ratio is, but it's at least 96[1], so\n>> clearly you'll need to figure out how to pad your first commit message\n>> with at least another 25 lines before this series can be accepted.\n>> ;-)\n>\n> Well, if we want to start digging things up... ;)\n>\n> Try this:\n>\n>   git log --no-merges --no-renames --format='%H %B' -z --numstat '*.c' |\n>   perl -0ne '\n>     chomp;\n>     if (s/^([0-9a-f]{40}) //) {\n>       if (defined $commit && $diff) {\n>         my $ratio = $body / $diff;\n>         print \"$ratio $body $diff $commit\\n\";\n>       }\n>       $commit = $1;\n>       $body = () = /\\n/g;\n>       $diff = 0;\n>     } elsif (/^\\s*(\\d+)\\t/) {\n>       # this counts only added lines, under the assumption that\n>       # small commits generally remove/add in proportion. Of course\n>       # ones that _only_ remove lines have infinite ratios.\n>       $diff += $1;\n>     } else {\n>       die \"confusing record: $_\\n\";\n>     }\n>   ' |\n>   sort -rn |\n>   head\n>\n> which shows there are a few in the 100's. Pipe through:\n>\n>   awk '{print $4}' |\n>   git log --stdin --no-walk=unsorted --stat\n>\n> for a nicer view. I'm rejecting the top one on the grounds that it's\n> mostly cut-and-paste output, and also that #2 is mine. ;)\n\nI think that '*.c' is cheating, if anything I should be getting more\npoints when you remove that, as I've been over explaining\nadding/removing a compiler flag or something. At least your #2 is tricky\nC code :)\n\nI haven't bothered to do this, but I think if you --word-diff\n--word-diff-regex=. and parse the resulting diff you'd get \"better\"\nresults.\n\nOr, for better & similar (but not the same): compute the levenshtein\ndistance of the pre- and post-image, and compute edit distance to commit\nmessage length.\n\nI haven't done that, but just from eyeballing it I think [1] beats your\n[2] by that criteria. Per:\n\t\n\t$ perl -MText::Levenshtein=distance -wE 'say distance @ARGV' int unsigned\n\t6\n\t$ perl -MText::Levenshtein=distance -wE 'say distance @ARGV' \"\" _lf\n\t3\n\nIt should get 2x the score v.s. yours, but yours is <2x the\nwords/characters.\n\n(Edit: But see [4] below)\n\nThere's also e.g. my [3] that's fairly high in the running per your\n\"only added lines\". But I think it shows the perils of doing that,\ni.e. in general I don't see why you'd omit deletions, that commit\nmessage is certainly spending most of its time talking about why the\ndeletion of the code at hand is OK.\n\nOnce you count deletions it'll get *way* down the list, as it's 11\ndeleted lines, 1 added.\n\nHrm, I take some of the above back, I think [4] might be the winner.\nThat's just an edit distance of 1, so it's around 2x the commit message\nlength of yours if we adjust for your score of 6. (~2.5 by\ncharacters)[5].\n\n1. 356c4732950 (credential: treat CR/LF as line endings in the\n   credential protocol, 2020-10-03)\n2. aec0bba106d (config: work around gcc-10 -Wstringop-overflow warning,\n   2020-08-04)\n3. f97fe358576 (pickaxe -G: don't special-case create/delete,\n   2021-04-12)\n4. c58bebd4c67 (ci: update Cirrus-CI image to FreeBSD 12.3, 2022-05-25)\n5. All measured with \"git show --no-notes --no-patch <commit> | wc\",\n   because I was lazy.\n"},{"id":"466319","messageId":"CABPp-BHwWYij2zqTSnsuu1ib97M4kJhfjMeEFqV13nttdqT1yw@mail.gmail.com","threadId":"58710","inReplyTo":"221102.86pme52z8d.gmgdl@evledraar.gmail.com","subject":"Re: gigantic commit messages, was Re: Git Bug Report: out of memory using git tag","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-11-02T15:43:52Z","receivedAt":"2022-11-02T15:45:21Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Nov 2, 2022 at 7:43 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:\n>\n> On Wed, Nov 02 2022, Jeff King wrote:\n>\n> > On Wed, Nov 02, 2022 at 01:14:59AM -0700, Elijah Newren wrote:\n> >\n> >> On Wed, Nov 2, 2022 at 12:51 AM Jeff King <peff@peff.net> wrote:\n> >> >\n> >> > Here are patches which fix them both. I may be setting a new record for\n> >> > the ratio of commit message lines to changed code\n> >>\n> >> It looks like the first patch is 72 lines of commit message for a\n> >> one-line fix, and the second patch is 61 lines of commit message for a\n> >> two line fix.\n> >>\n> >> I don't know what the record ratio is, but it's at least 96[1], so\n> >> clearly you'll need to figure out how to pad your first commit message\n> >> with at least another 25 lines before this series can be accepted.\n> >> ;-)\n> >\n> > Well, if we want to start digging things up... ;)\n> >\n> > Try this:\n> >\n> >   git log --no-merges --no-renames --format='%H %B' -z --numstat '*.c' |\n> >   perl -0ne '\n> >     chomp;\n> >     if (s/^([0-9a-f]{40}) //) {\n> >       if (defined $commit && $diff) {\n> >         my $ratio = $body / $diff;\n> >         print \"$ratio $body $diff $commit\\n\";\n> >       }\n> >       $commit = $1;\n> >       $body = () = /\\n/g;\n> >       $diff = 0;\n> >     } elsif (/^\\s*(\\d+)\\t/) {\n> >       # this counts only added lines, under the assumption that\n> >       # small commits generally remove/add in proportion. Of course\n> >       # ones that _only_ remove lines have infinite ratios.\n> >       $diff += $1;\n> >     } else {\n> >       die \"confusing record: $_\\n\";\n> >     }\n> >   ' |\n> >   sort -rn |\n> >   head\n> >\n> > which shows there are a few in the 100's. Pipe through:\n> >\n> >   awk '{print $4}' |\n> >   git log --stdin --no-walk=unsorted --stat\n> >\n> > for a nicer view. I'm rejecting the top one on the grounds that it's\n> > mostly cut-and-paste output, and also that #2 is mine. ;)\n>\n> I think that '*.c' is cheating, if anything I should be getting more\n> points when you remove that, as I've been over explaining\n> adding/removing a compiler flag or something. At least your #2 is tricky\n> C code :)\n>\n> I haven't bothered to do this, but I think if you --word-diff\n> --word-diff-regex=. and parse the resulting diff you'd get \"better\"\n> results.\n>\n> Or, for better & similar (but not the same): compute the levenshtein\n> distance of the pre- and post-image, and compute edit distance to commit\n> message length.\n>\n> I haven't done that, but just from eyeballing it I think [1] beats your\n> [2] by that criteria. Per:\n>\n>         $ perl -MText::Levenshtein=distance -wE 'say distance @ARGV' int unsigned\n>         6\n>         $ perl -MText::Levenshtein=distance -wE 'say distance @ARGV' \"\" _lf\n>         3\n>\n> It should get 2x the score v.s. yours, but yours is <2x the\n> words/characters.\n>\n> (Edit: But see [4] below)\n>\n> There's also e.g. my [3] that's fairly high in the running per your\n> \"only added lines\". But I think it shows the perils of doing that,\n> i.e. in general I don't see why you'd omit deletions, that commit\n> message is certainly spending most of its time talking about why the\n> deletion of the code at hand is OK.\n>\n> Once you count deletions it'll get *way* down the list, as it's 11\n> deleted lines, 1 added.\n>\n> Hrm, I take some of the above back, I think [4] might be the winner.\n> That's just an edit distance of 1, so it's around 2x the commit message\n> length of yours if we adjust for your score of 6. (~2.5 by\n> characters)[5].\n>\n> 1. 356c4732950 (credential: treat CR/LF as line endings in the\n>    credential protocol, 2020-10-03)\n> 2. aec0bba106d (config: work around gcc-10 -Wstringop-overflow warning,\n>    2020-08-04)\n> 3. f97fe358576 (pickaxe -G: don't special-case create/delete,\n>    2021-04-12)\n> 4. c58bebd4c67 (ci: update Cirrus-CI image to FreeBSD 12.3, 2022-05-25)\n> 5. All measured with \"git show --no-notes --no-patch <commit> | wc\",\n>    because I was lazy.\n\nHehe, my offhand joke started a contest over the whimsical question of\nwho's the most long-winded.  I think my work here is done.  :-)\n"},{"id":"466366","messageId":"Y2MOYfalWOB9YlP2@nand.local","threadId":"58710","inReplyTo":"Y2IeqOT5Ao1Qa0Zl@coredump.intra.peff.net","subject":"Re: Git Bug Report: out of memory using git tag","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-03T00:42:09Z","receivedAt":"2022-11-03T00:42:17Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Nov 02, 2022 at 03:39:20AM -0400, Jeff King wrote:\n> After sleeping on it, I think I fully understand what's going on. There\n> are actually _two_ bugs, but they are closely related. ;)\n>\n> Here are patches which fix them both. I may be setting a new record for\n> the ratio of commit message lines to changed code. But it took me a\n> while to figure out what was going on, so I wanted to explain it fully.\n>\n>   [1/2]: ref-filter: fix parsing of signatures without blank lines\n>   [2/2]: ref-filter: fix parsing of signatures with CRLF and no body\n\nIndeed. I agree with others below in the thread that this is very well\nexplained, and the fix(es) both look sensible to me.\n\nWill queue, thanks.\n\n\nThanks,\nTaylor\n"},{"id":"466377","messageId":"Y2NEeWwnfHnIbNl8@coredump.intra.peff.net","threadId":"58710","inReplyTo":"49e1eec2-0567-f75e-7d0d-0f2d00ac472c@gmail.com","subject":"Re: Git Bug Report: out of memory using git tag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-03T04:32:57Z","receivedAt":"2022-11-03T04:33:45Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 02, 2022 at 08:13:27AM -0400, Philippe Blain wrote:\n\n> >   [1/2]: ref-filter: fix parsing of signatures without blank lines\n> >   [2/2]: ref-filter: fix parsing of signatures with CRLF and no body\n> [...]\n> One thing I think that is not mentioned in your commit messages is that 1/2\n> would also apply to a CRLF-using tag with no blank lines, i.e.\n> \n>   this is the subject^M\n>   -----BEGIN PGP SIGNATURE-----^M\n>   ...some stuff...^M\n>   -----END PGP SIGNATURE-----^M\n> \n> Parsing this tag correctly is fixed by 1/2, right?\n\nRight. Because there's no blank line to find, neither the \"\\n\\n\" nor the\n\"\\r\\n\\r\\n\" strstr matches, so it is essentially the same case. The fact\nthat the other non-blank lines are CRLF doesn't matter for this part of\nthe code. :)\n\n> Anyway thanks again for the fixes,\n\nNo problem!\n\n-Peff\n"}]}