{"thread":{"id":"57746","subject":"Corrupt name-rev output","startedAt":"2022-04-17T16:54:33Z","lastAt":"2022-05-17T16:21:30Z","messageCount":8,"participants":["Thomas Hurst","Junio C Hamano","Elijah Newren","René Scharfe","Ævar Arnfjörð Bjarmason"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"453795","messageId":"Ylw+M5wwUYKcLM+M@voi.aagh.net","threadId":"57746","inReplyTo":null,"subject":"Corrupt name-rev output","fromName":"Thomas Hurst","fromEmail":"tom@hur.st","sentAt":"2022-04-17T16:20:03Z","receivedAt":"2022-04-17T16:54:33Z","isPatch":false,"sender":{"key":"tom@hur.st","avatar":null},"body":"I've noticed a series of about 20 commits in the HardenedBSD repository\nfairly reliably produce garbage names from git name-rev - usually\nfragments of another commit, sometimes unprintable nonsense. Sometimes\nit works just fine...\n\nHere's a quick demo showing how to reproduce the problem:\n\n% uname -mrs\nFreeBSD 13.0-RELEASE-p11 amd64\n% git --version\ngit version 2.35.2\n% git clone --bare --mirror https://github.com/HardenedBSD/hardenedBSD.git\n% cd hardenedBSD.git\n% git rev-list --branches=\\* |\n git name-rev --stdin --refs=heads/\\* |\n egrep -v '^[0-9a-f]{40}( \\([a-zA-Z0-9_/.^~-]+\\))?$'\n3eb67b534cab6a78b44b13e4323fd60353003089 (y: marcel\nMFC after: 3 days\nRelnotes: yes\nSponsored by: ScaleEngine Inc.\nDifferential Revision: https://reviews.freebsd.org/D3065\n~3)\n3ac660fc0c6eb0f876972e7e415c89f1ebed1939 (y: marcel\nMFC after: 3 days\nRelnotes: yes\nSponsored by: ScaleEngine Inc.\nDifferential Revision: https://reviews.freebsd.org/D3065\n~4)\n\n\nHere's the result of another run, which includes all the affected\nhashes - the name here presumably also has some non-printable garbage\nnot shown:\n\n3eb67b534cab6a78b44b13e4323fd60353003089 (H~3)\n3ac660fc0c6eb0f876972e7e415c89f1ebed1939 (H~4)\n888a2df901616fb2900279c75580de3d4bc93278 (H~5)\n58b9ad4f2e737ff922ff49ae564a32ee94c0ff6b (H~6)\n59181d461b8d8c45144dc1856fa94da8a197b2ee (H~7)\n9dd8627f2d53008b73ed93f92b71e24e8e319d93 (H~8)\n3e1340e9928c7398551d3192a08acb678defc75e (H~9)\n6ed06b88b912202db5b9d28b281ddbf4a1b0652a (H~10)\n03b1b642f686502c128ac78ccf9dba7605f2aedf (H~11)\ne7861fb764975a2c5f2f558b0800656679959624 (H~12)\n4c28d4a51469dbb69bac67ec3b1c3a3d114ddf80 (H~13)\n9ad0ed2485e40adce13b3900230640db66a38311 (H~14)\n88dbdc409320e2785be4b1fcf0392999e3ab4374 (H~15)\nc09997f49b7ca54f8d7305be57a037a294c32780 (H~16)\n1f95262a4cda63a14016278f8ccc42c217858c36 (H~17)\n50fec7a08d222bfd97499afa1c4000322c2280ff (H~18)\ndf3a215e38869117f8297e0910e08ae95eea5a59 (H~19)\ncc81f4cf69d01cfc329827dbbc44da219bdddc05 (H~20)\n5a7f7f242655f04a42e152d2c2f45819e9de4376 (H~21)\n39cb585f2d4d4db10a282b06d9f7a9f07b658804 (H~22)\n1264cc1d38cb1f1f03e34e395b33011108945638 (H~23)\n98883cdf600f4aa3d5aba1d60248000132b6ad58 (H~24)\n541df39729d32a70ba4da5184a8e82aaa3c87ae5 (H~25)\n288fdc199a6936caab9d89d7deeb9b2bcf3b1f68 (H~26)\n\nPassing these commits into name-rev as arguments finds them under\nhardened/current/relro~199^2\n\ngit fsck --full does not reveal or fix anything, and the problem also\npersists with a build from source from the next branch.\n\nI was unable to reproduce on an Ubuntu machine with 2.32.0, so I used\nthat as a starting point for bisection and landed here:\n\n 3656f842789d25d75da41c6c029470052a573b54\n name-rev: prefer shorter names over following merges\n\n-- \nThomas 'Freaky' Hurst\n http://hur.st/\n"},{"id":"453947","messageId":"xmqq4k2otpkb.fsf@gitster.g","threadId":"57746","inReplyTo":"Ylw+M5wwUYKcLM+M@voi.aagh.net","subject":"Re: Corrupt name-rev output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-20T03:13:24Z","receivedAt":"2022-04-20T03:13:36Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Hurst writes:\n\n> I've noticed a series of about 20 commits in the HardenedBSD repository\n> fairly reliably produce garbage names from git name-rev - usually\n> fragments of another commit, sometimes unprintable nonsense. Sometimes\n> it works just fine...\n>\n> Here's a quick demo showing how to reproduce the problem:\n>\n> % uname -mrs\n> FreeBSD 13.0-RELEASE-p11 amd64\n> % git --version\n> git version 2.35.2\n> % git clone --bare --mirror https://github.com/HardenedBSD/hardenedBSD.git\n> % cd hardenedBSD.git\n> % git rev-list --branches=\\* |\n> git name-rev --stdin --refs=heads/\\* |\n> egrep -v '^[0-9a-f]{40}( \\([a-zA-Z0-9_/.^~-]+\\))?$'\n> 3eb67b534cab6a78b44b13e4323fd60353003089 (y: marcel\n> MFC after: 3 days\n> Relnotes: yes\n> Sponsored by: ScaleEngine Inc.\n> Differential Revision: https://reviews.freebsd.org/D3065\n> ~3)\n> 3ac660fc0c6eb0f876972e7e415c89f1ebed1939 (y: marcel\n> ...\n> Passing these commits into name-rev as arguments finds them under\n> hardened/current/relro~199^2\n>\n> git fsck --full does not reveal or fix anything, and the problem also\n> persists with a build from source from the next branch.\n>\n> I was unable to reproduce on an Ubuntu machine with 2.32.0, so I used\n> that as a starting point for bisection and landed here:\n>\n> 3656f842789d25d75da41c6c029470052a573b54\n> name-rev: prefer shorter names over following merges\n\ncommit 3656f842789d25d75da41c6c029470052a573b54\nAuthor: Elijah Newren \n\nHmph, Elijah, does this ring a bell?\n\nThanks.\n"},{"id":"454045","messageId":"CABPp-BGd8194tPo97Zmuu2xX_aqHYfBrVUX0F0r6EPAaUA3U2w@mail.gmail.com","threadId":"57746","inReplyTo":"xmqq4k2otpkb.fsf@gitster.g","subject":"Re: Corrupt name-rev output","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-04-21T02:11:59Z","receivedAt":"2022-04-21T02:12:14Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Apr 19, 2022 at 8:13 PM Junio C Hamano wrote:\n>\n> Thomas Hurst writes:\n>\n> > I've noticed a series of about 20 commits in the HardenedBSD repository\n> > fairly reliably produce garbage names from git name-rev - usually\n> > fragments of another commit, sometimes unprintable nonsense. Sometimes\n> > it works just fine...\n> >\n> > Here's a quick demo showing how to reproduce the problem:\n> >\n> > % uname -mrs\n> > FreeBSD 13.0-RELEASE-p11 amd64\n> > % git --version\n> > git version 2.35.2\n> > % git clone --bare --mirror https://github.com/HardenedBSD/hardenedBSD.git\n> > % cd hardenedBSD.git\n> > % git rev-list --branches=\\* |\n> > git name-rev --stdin --refs=heads/\\* |\n> > egrep -v '^[0-9a-f]{40}( \\([a-zA-Z0-9_/.^~-]+\\))?$'\n> > 3eb67b534cab6a78b44b13e4323fd60353003089 (y: marcel\n> > MFC after: 3 days\n> > Relnotes: yes\n> > Sponsored by: ScaleEngine Inc.\n> > Differential Revision: https://reviews.freebsd.org/D3065\n> > ~3)\n> > 3ac660fc0c6eb0f876972e7e415c89f1ebed1939 (y: marcel\n> > ...\n> > Passing these commits into name-rev as arguments finds them under\n> > hardened/current/relro~199^2\n> >\n> > git fsck --full does not reveal or fix anything, and the problem also\n> > persists with a build from source from the next branch.\n> >\n> > I was unable to reproduce on an Ubuntu machine with 2.32.0, so I used\n> > that as a starting point for bisection and landed here:\n> >\n> > 3656f842789d25d75da41c6c029470052a573b54\n> > name-rev: prefer shorter names over following merges\n>\n> commit 3656f842789d25d75da41c6c029470052a573b54\n> Author: Elijah Newren \n>\n> Hmph, Elijah, does this ring a bell?\n\nAfter digging around last night and tonight, this appears to be a poor\ninteraction with commit 2d53975488 (\"name-rev: release unused name\nstrings\", 2020-02-04), which frees shared strings and relies on all\nother users of that shared string to update their name, which\napparently seemed to rely on some intricacies of how the algorithm\nworked that are no longer valid with my change, resulting in some\nuse-after-frees (though for some reason valgrind isn't spotting them\nfor me, which made it harder to track these down).\n\nReverting 2d53975488 fixes the problem.\n\nMaybe this means we need to have tip_name be a string + a refcount, so\nthat we can know when we can safely free it? Adding Rene to the cc\nfor comments. Rene: If it helps, there's a slightly simpler\nreproduction: clone the repo Thomas mentions, and then instead of his\n\"rev-list | name-rev | grep\" sequence just run:\n\n git name-rev --refs=heads/\\* 3eb67b534cab6a78b44b13e4323fd60353003089\n"},{"id":"454097","messageId":"779eb30b-fdb9-81fb-5d43-c8d388c5cb5a@web.de","threadId":"57746","inReplyTo":"CABPp-BGd8194tPo97Zmuu2xX_aqHYfBrVUX0F0r6EPAaUA3U2w@mail.gmail.com","subject":"Re: Corrupt name-rev output","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-04-21T17:55:37Z","receivedAt":"2022-04-21T17:57:01Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 21.04.22 um 04:11 schrieb Elijah Newren:\n> On Tue, Apr 19, 2022 at 8:13 PM Junio C Hamano wrote:\n>>\n>> Thomas Hurst writes:\n>>\n>>> I've noticed a series of about 20 commits in the HardenedBSD repository\n>>> fairly reliably produce garbage names from git name-rev - usually\n>>> fragments of another commit, sometimes unprintable nonsense. Sometimes\n>>> it works just fine...\n>>>\n>>> Here's a quick demo showing how to reproduce the problem:\n>>>\n>>> % uname -mrs\n>>> FreeBSD 13.0-RELEASE-p11 amd64\n>>> % git --version\n>>> git version 2.35.2\n>>> % git clone --bare --mirror https://github.com/HardenedBSD/hardenedBSD.git\n>>> % cd hardenedBSD.git\n>>> % git rev-list --branches=\\* |\n>>> git name-rev --stdin --refs=heads/\\* |\n>>> egrep -v '^[0-9a-f]{40}( \\([a-zA-Z0-9_/.^~-]+\\))?$'\n>>> 3eb67b534cab6a78b44b13e4323fd60353003089 (y: marcel\n>>> MFC after: 3 days\n>>> Relnotes: yes\n>>> Sponsored by: ScaleEngine Inc.\n>>> Differential Revision: https://reviews.freebsd.org/D3065\n>>> ~3)\n>>> 3ac660fc0c6eb0f876972e7e415c89f1ebed1939 (y: marcel\n>>> ...\n>>> Passing these commits into name-rev as arguments finds them under\n>>> hardened/current/relro~199^2\n>>>\n>>> git fsck --full does not reveal or fix anything, and the problem also\n>>> persists with a build from source from the next branch.\n>>>\n>>> I was unable to reproduce on an Ubuntu machine with 2.32.0, so I used\n>>> that as a starting point for bisection and landed here:\n>>>\n>>> 3656f842789d25d75da41c6c029470052a573b54\n>>> name-rev: prefer shorter names over following merges\n>>\n>> commit 3656f842789d25d75da41c6c029470052a573b54\n>> Author: Elijah Newren \n>>\n>> Hmph, Elijah, does this ring a bell?\n>\n> After digging around last night and tonight, this appears to be a poor\n> interaction with commit 2d53975488 (\"name-rev: release unused name\n> strings\", 2020-02-04), which frees shared strings and relies on all\n> other users of that shared string to update their name, which\n> apparently seemed to rely on some intricacies of how the algorithm\n> worked that are no longer valid with my change, resulting in some\n> use-after-frees (though for some reason valgrind isn't spotting them\n> for me, which made it harder to track these down).\n\nSo we need better test coverage.\n\nThe strings are freed for generation 0 because their new name was always\nbetter for higher generations as well and thus would be replaced and not\nlooked at again. I think that assumption doesn't hold anymore because\n3656f84278 (name-rev: prefer shorter names over following merges,\n2021-12-04) raised the bar for names to be accepted for generation > 0.\n\n> Reverting 2d53975488 fixes the problem.\n\nThat's a good band-aid. Reverting 3656f84278 would help as well, but\nchange the output.\n\n> Maybe this means we need to have tip_name be a string + a refcount, so\n> that we can know when we can safely free it?\n\nSure, at the cost of storing and maintaining the refcounts.\n\nIIRC I also explored avoiding to append suffixes to candidate name\nstrings in get_parent_name() and instead storing a reference to the\nparent. Many tag names are shorter than the eight bytes of a pointer\nand certainly shorter than an object ID, so while that would reduce the\nnumber of strings to deal with (and thus the need to free them), it\nprobably would increases the overall memory usage.\n\n> Adding Rene to the cc\n> for comments. Rene: If it helps, there's a slightly simpler\n> reproduction: clone the repo Thomas mentions, and then instead of his\n> \"rev-list | name-rev | grep\" sequence just run:\n>\n> git name-rev --refs=heads/\\* 3eb67b534cab6a78b44b13e4323fd60353003089\n"},{"id":"454271","messageId":"340c8810-d912-7b18-d46e-a9d43f20216a@web.de","threadId":"57746","inReplyTo":"779eb30b-fdb9-81fb-5d43-c8d388c5cb5a@web.de","subject":"Re: Corrupt name-rev output","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-04-22T18:44:40Z","receivedAt":"2022-04-22T18:56:41Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 21.04.22 um 19:55 schrieb René Scharfe:\n> Am 21.04.22 um 04:11 schrieb Elijah Newren:\n>\n>> Reverting 2d53975488 fixes the problem.\n>\n> That's a good band-aid.\nOr perhaps it's all we need. I can't replicate the original reduction\nof peak memory usage for the Chromium repo anymore. In fact, the very\nnext commit, 079f970971 (name-rev: sort tip names before applying,\n2020-02-05), reduced the number of times free(3) is called there from\n44245 to 5, and 3656f84278 (name-rev: prefer shorter names over\nfollowing merges, 2021-12-04) brought that number down to zero.\n\nI can't reproduce the issue with the hardenedBSD repo, by the way, but\ne.g. with 'git name-rev 58b82150da' in the Linux repo.\n\n--- >8 ---\nSubject: [PATCH] Revert \"name-rev: release unused name strings\"\n\nThis reverts commit 2d53975488df195e1431c3f90bfb5b60018d5bf6.\n\n3656f84278 (name-rev: prefer shorter names over following merges,\n2021-12-04) broke the assumption of 2d53975488 (name-rev: release unused\nname strings, 2020-02-04) that a better name for a child is a better\nname for all of its ancestors as well, because it added a penalty for\ngeneration > 0. This leads to strings being free(3)'d that are still\nneeded.\n\n079f970971 (name-rev: sort tip names before applying, 2020-02-05)\nalready reduced the number of free(3) calls for the use case that\nmotivated the original patch (name-rev --all in the Chromium repository)\nfrom ca. 44000 to 5, and 3656f84278 eliminated even those few. So this\nrevert won't affect name-rev's performance on that particular repo.\n\nReported-by: Thomas Hurst \nHelped-by: Elijah Newren \nSigned-off-by: René Scharfe \n---\n builtin/name-rev.c | 21 +++++----------------\n 1 file changed, 5 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/name-rev.c b/builtin/name-rev.c\nindex c59b5699fe..02ea9d1633 100644\n--- a/builtin/name-rev.c\n+++ b/builtin/name-rev.c\n@@ -18,7 +18,7 @@\n #define CUTOFF_DATE_SLOP 86400\n\n struct rev_name {\n-\tchar *tip_name;\n+\tconst char *tip_name;\n \ttimestamp_t taggerdate;\n \tint generation;\n \tint distance;\n@@ -84,7 +84,7 @@ static int commit_is_before_cutoff(struct commit *commit)\n\n static int is_valid_rev_name(const struct rev_name *name)\n {\n-\treturn name && (name->generation || name->tip_name);\n+\treturn name && name->tip_name;\n }\n\n static struct rev_name *get_commit_rev_name(const struct commit *commit)\n@@ -146,20 +146,9 @@ static struct rev_name *create_or_update_name(struct commit *commit,\n {\n \tstruct rev_name *name = commit_rev_name_at(&rev_names, commit);\n\n-\tif (is_valid_rev_name(name)) {\n-\t\tif (!is_better_name(name, taggerdate, generation, distance, from_tag))\n-\t\t\treturn NULL;\n-\n-\t\t/*\n-\t\t * This string might still be shared with ancestors\n-\t\t * (generation > 0). We can release it here regardless,\n-\t\t * because the new name that has just won will be better\n-\t\t * for them as well, so name_rev() will replace these\n-\t\t * stale pointers when it processes the parents.\n-\t\t */\n-\t\tif (!name->generation)\n-\t\t\tfree(name->tip_name);\n-\t}\n+\tif (is_valid_rev_name(name) &&\n+\t !is_better_name(name, taggerdate, generation, distance, from_tag))\n+\t\treturn NULL;\n\n \tname->taggerdate = taggerdate;\n \tname->generation = generation;\n--\n2.35.3\n"},{"id":"454343","messageId":"xmqqzgkb21c3.fsf@gitster.g","threadId":"57746","inReplyTo":"340c8810-d912-7b18-d46e-a9d43f20216a@web.de","subject":"Re: Corrupt name-rev output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-23T16:47:56Z","receivedAt":"2022-04-23T16:48:04Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Subject: [PATCH] Revert \"name-rev: release unused name strings\"\n>\n> This reverts commit 2d53975488df195e1431c3f90bfb5b60018d5bf6.\n>\n> 3656f84278 (name-rev: prefer shorter names over following merges,\n> 2021-12-04) broke the assumption of 2d53975488 (name-rev: release unused\n> name strings, 2020-02-04) that a better name for a child is a better\n> name for all of its ancestors as well, because it added a penalty for\n> generation > 0. This leads to strings being free(3)'d that are still\n> needed.\n\nNicely described.\n\nThanks.\n"},{"id":"455377","messageId":"220517.86sfp878zr.gmgdl@evledraar.gmail.com","threadId":"57746","inReplyTo":"340c8810-d912-7b18-d46e-a9d43f20216a@web.de","subject":"Re: Corrupt name-rev output","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-05-17T10:15:20Z","receivedAt":"2022-05-17T10:25:19Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Apr 22 2022, René Scharfe wrote:\n\n> Am 21.04.22 um 19:55 schrieb René Scharfe:\n>> Am 21.04.22 um 04:11 schrieb Elijah Newren:\n>>\n>>> Reverting 2d53975488 fixes the problem.\n>>\n>> That's a good band-aid.\n> Or perhaps it's all we need. I can't replicate the original reduction\n> of peak memory usage for the Chromium repo anymore. In fact, the very\n> next commit, 079f970971 (name-rev: sort tip names before applying,\n> 2020-02-05), reduced the number of times free(3) is called there from\n> 44245 to 5, and 3656f84278 (name-rev: prefer shorter names over\n> following merges, 2021-12-04) brought that number down to zero.\n>\n> I can't reproduce the issue with the hardenedBSD repo, by the way, but\n> e.g. with 'git name-rev 58b82150da' in the Linux repo.\n>\n> --- >8 ---\n> Subject: [PATCH] Revert \"name-rev: release unused name strings\"\n>\n> This reverts commit 2d53975488df195e1431c3f90bfb5b60018d5bf6.\n>\n> 3656f84278 (name-rev: prefer shorter names over following merges,\n> 2021-12-04) broke the assumption of 2d53975488 (name-rev: release unused\n> name strings, 2020-02-04) that a better name for a child is a better\n> name for all of its ancestors as well, because it added a penalty for\n> generation > 0. This leads to strings being free(3)'d that are still\n> needed.\n>\n> 079f970971 (name-rev: sort tip names before applying, 2020-02-05)\n> already reduced the number of free(3) calls for the use case that\n> motivated the original patch (name-rev --all in the Chromium repository)\n> from ca. 44000 to 5, and 3656f84278 eliminated even those few. So this\n> revert won't affect name-rev's performance on that particular repo.\n>\n> Reported-by: Thomas Hurst \n> Helped-by: Elijah Newren \n> Signed-off-by: René Scharfe \n> ---\n> builtin/name-rev.c | 21 +++++----------------\n> 1 file changed, 5 insertions(+), 16 deletions(-)\n>\n> diff --git a/builtin/name-rev.c b/builtin/name-rev.c\n> index c59b5699fe..02ea9d1633 100644\n> --- a/builtin/name-rev.c\n> +++ b/builtin/name-rev.c\n> @@ -18,7 +18,7 @@\n> #define CUTOFF_DATE_SLOP 86400\n>\n> struct rev_name {\n> -\tchar *tip_name;\n> +\tconst char *tip_name;\n> \ttimestamp_t taggerdate;\n> \tint generation;\n> \tint distance;\n> @@ -84,7 +84,7 @@ static int commit_is_before_cutoff(struct commit *commit)\n>\n> static int is_valid_rev_name(const struct rev_name *name)\n> {\n> -\treturn name && (name->generation || name->tip_name);\n> +\treturn name && name->tip_name;\n> }\n>\n> static struct rev_name *get_commit_rev_name(const struct commit *commit)\n> @@ -146,20 +146,9 @@ static struct rev_name *create_or_update_name(struct commit *commit,\n> {\n> \tstruct rev_name *name = commit_rev_name_at(&rev_names, commit);\n>\n> -\tif (is_valid_rev_name(name)) {\n> -\t\tif (!is_better_name(name, taggerdate, generation, distance, from_tag))\n> -\t\t\treturn NULL;\n> -\n> -\t\t/*\n> -\t\t * This string might still be shared with ancestors\n> -\t\t * (generation > 0). We can release it here regardless,\n> -\t\t * because the new name that has just won will be better\n> -\t\t * for them as well, so name_rev() will replace these\n> -\t\t * stale pointers when it processes the parents.\n> -\t\t */\n> -\t\tif (!name->generation)\n> -\t\t\tfree(name->tip_name);\n> -\t}\n> +\tif (is_valid_rev_name(name) &&\n> +\t !is_better_name(name, taggerdate, generation, distance, from_tag))\n> +\t\treturn NULL;\n>\n> \tname->taggerdate = taggerdate;\n> \tname->generation = generation;\n\nI haven't dug into whether it's a false positive, but with this change\nGCC's -fanalyzer has started complaining about a potential NULL\ndereference:\n\n builtin/name-rev.c:230:50: error: dereference of NULL ‘name’ [CWE-476] [-Werror=analyzer-null-dereference]\n 230 | generation = name->generation + 1;\n\nThis \"fixes\" it, and passes all tests, but presumably a better fix\ninvolves the callers of get_commit_rev_name() (or that function itself)\ndeciding if they're OK with NULL here earlier?:\n\t\n\tdiff --git a/builtin/name-rev.c b/builtin/name-rev.c\n\tindex 02ea9d16330..1d3a620ac72 100644\n\t--- a/builtin/name-rev.c\n\t+++ b/builtin/name-rev.c\n\t@@ -209,6 +209,7 @@ static void name_rev(struct commit *start_commit,\n\t \t\tstruct rev_name *name = get_commit_rev_name(commit);\n\t \t\tstruct commit_list *parents;\n\t \t\tint parent_number = 1;\n\t+\t\tassert(name);\n\t \n\t \t\tparents_to_queue_nr = 0;\n\t \n\t\n"},{"id":"455389","messageId":"dece627d-ccf8-2375-0c50-c59637e561d3@web.de","threadId":"57746","inReplyTo":"220517.86sfp878zr.gmgdl@evledraar.gmail.com","subject":"Re: Corrupt name-rev output","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-05-17T16:20:58Z","receivedAt":"2022-05-17T16:21:30Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 17.05.22 um 12:15 schrieb Ævar Arnfjörð Bjarmason:\n>\n> On Fri, Apr 22 2022, René Scharfe wrote:\n>\n>> Am 21.04.22 um 19:55 schrieb René Scharfe:\n>>> Am 21.04.22 um 04:11 schrieb Elijah Newren:\n>>>\n>>>> Reverting 2d53975488 fixes the problem.\n>>>\n>>> That's a good band-aid.\n>> Or perhaps it's all we need. I can't replicate the original reduction\n>> of peak memory usage for the Chromium repo anymore. In fact, the very\n>> next commit, 079f970971 (name-rev: sort tip names before applying,\n>> 2020-02-05), reduced the number of times free(3) is called there from\n>> 44245 to 5, and 3656f84278 (name-rev: prefer shorter names over\n>> following merges, 2021-12-04) brought that number down to zero.\n>>\n>> I can't reproduce the issue with the hardenedBSD repo, by the way, but\n>> e.g. with 'git name-rev 58b82150da' in the Linux repo.\n>>\n>> --- >8 ---\n>> Subject: [PATCH] Revert \"name-rev: release unused name strings\"\n>>\n>> This reverts commit 2d53975488df195e1431c3f90bfb5b60018d5bf6.\n>>\n>> 3656f84278 (name-rev: prefer shorter names over following merges,\n>> 2021-12-04) broke the assumption of 2d53975488 (name-rev: release unused\n>> name strings, 2020-02-04) that a better name for a child is a better\n>> name for all of its ancestors as well, because it added a penalty for\n>> generation > 0. This leads to strings being free(3)'d that are still\n>> needed.\n>>\n>> 079f970971 (name-rev: sort tip names before applying, 2020-02-05)\n>> already reduced the number of free(3) calls for the use case that\n>> motivated the original patch (name-rev --all in the Chromium repository)\n>> from ca. 44000 to 5, and 3656f84278 eliminated even those few. So this\n>> revert won't affect name-rev's performance on that particular repo.\n>>\n>> Reported-by: Thomas Hurst \n>> Helped-by: Elijah Newren \n>> Signed-off-by: René Scharfe \n>> ---\n>> builtin/name-rev.c | 21 +++++----------------\n>> 1 file changed, 5 insertions(+), 16 deletions(-)\n>>\n>> diff --git a/builtin/name-rev.c b/builtin/name-rev.c\n>> index c59b5699fe..02ea9d1633 100644\n>> --- a/builtin/name-rev.c\n>> +++ b/builtin/name-rev.c\n>> @@ -18,7 +18,7 @@\n>> #define CUTOFF_DATE_SLOP 86400\n>>\n>> struct rev_name {\n>> -\tchar *tip_name;\n>> +\tconst char *tip_name;\n>> \ttimestamp_t taggerdate;\n>> \tint generation;\n>> \tint distance;\n>> @@ -84,7 +84,7 @@ static int commit_is_before_cutoff(struct commit *commit)\n>>\n>> static int is_valid_rev_name(const struct rev_name *name)\n>> {\n>> -\treturn name && (name->generation || name->tip_name);\n>> +\treturn name && name->tip_name;\n>> }\n>>\n>> static struct rev_name *get_commit_rev_name(const struct commit *commit)\n>> @@ -146,20 +146,9 @@ static struct rev_name *create_or_update_name(struct commit *commit,\n>> {\n>> \tstruct rev_name *name = commit_rev_name_at(&rev_names, commit);\n>>\n>> -\tif (is_valid_rev_name(name)) {\n>> -\t\tif (!is_better_name(name, taggerdate, generation, distance, from_tag))\n>> -\t\t\treturn NULL;\n>> -\n>> -\t\t/*\n>> -\t\t * This string might still be shared with ancestors\n>> -\t\t * (generation > 0). We can release it here regardless,\n>> -\t\t * because the new name that has just won will be better\n>> -\t\t * for them as well, so name_rev() will replace these\n>> -\t\t * stale pointers when it processes the parents.\n>> -\t\t */\n>> -\t\tif (!name->generation)\n>> -\t\t\tfree(name->tip_name);\n>> -\t}\n>> +\tif (is_valid_rev_name(name) &&\n>> +\t !is_better_name(name, taggerdate, generation, distance, from_tag))\n>> +\t\treturn NULL;\n>>\n>> \tname->taggerdate = taggerdate;\n>> \tname->generation = generation;\n>\n> I haven't dug into whether it's a false positive, but with this change\n> GCC's -fanalyzer has started complaining about a potential NULL\n> dereference:\n>\n> builtin/name-rev.c:230:50: error: dereference of NULL ‘name’ [CWE-476] [-Werror=analyzer-null-dereference]\n> 230 | generation = name->generation + 1;\n>\n> This \"fixes\" it, and passes all tests, but presumably a better fix\n> involves the callers of get_commit_rev_name() (or that function itself)\n> deciding if they're OK with NULL here earlier?:\n>\n> \tdiff --git a/builtin/name-rev.c b/builtin/name-rev.c\n> \tindex 02ea9d16330..1d3a620ac72 100644\n> \t--- a/builtin/name-rev.c\n> \t+++ b/builtin/name-rev.c\n> \t@@ -209,6 +209,7 @@ static void name_rev(struct commit *start_commit,\n> \t \t\tstruct rev_name *name = get_commit_rev_name(commit);\n> \t \t\tstruct commit_list *parents;\n> \t \t\tint parent_number = 1;\n> \t+\t\tassert(name);\n>\n> \t \t\tparents_to_queue_nr = 0;\n>\n>\n\nIt's a false positive AFAICS because we set the name of the first commit\nto go into the queue with create_or_update_name(), then go through its\nparents and set their name as well before putting them into the queue to\ntraverse ancestors recursively. I.e. everything we pull from the queue\nmust have a non-NULL name, right?\n\nThis begs the question why a free(3) call would make a difference to the\nanalyzer -- it doesn't affect NULL pointers after all.\n\nAnother good question is whether we should remove the validity check and\nuse commit_rev_name_peek() directly instead of get_commit_rev_name() in\nthe loop since we know it must be valid. This might save a few cycles\nand perhaps calm down the analyzer.\n\nRené\n"}]}