{"thread":{"id":"57952","subject":"[RFC PATCH 00/15] Fix GCC -fanalyzer warnings & add -fanalyzer DEVOPTS mode","startedAt":"2022-06-03T18:39:18Z","lastAt":"2022-06-07T19:28:47Z","messageCount":51,"participants":["Ævar Arnfjörð Bjarmason","René Scharfe","Junio C Hamano","Glen Choo","Phillip Wood"],"isPatch":true,"patchVersion":1,"patchTotal":15},"messages":[{"id":"456605","messageId":"RFC-cover-00.15-00000000000-20220603T183608Z-avarab@gmail.com","threadId":"57952","inReplyTo":null,"subject":"[RFC PATCH 00/15] Fix GCC -fanalyzer warnings & add -fanalyzer DEVOPTS mode","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-03T18:37:37Z","receivedAt":"2022-06-03T18:39:18Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The -fanalyzer mode in GCC v12 is much better about reporting real\nissues. This RFC series currently conflicts with \"next\", but I thought\nsending it for comment sooner than later made sense.\n\nWith this series applied you can run:\n\n make DEVOPTS=analyzer\n\nWhich will turn all of these into warnings even under -Werror, we can\nalso:\n\n make DEVOPTS=\"analyzer no-suppress-analyzer\"\n\nThis works on both GCC 12 and GCC 11, but the former will yield better\nresults. We need to quiet some issues on GCC 11. GCC 10 ships with\n\"-fanalyzer\", but it has so many false positives that we'll $(error)\nout if you try to run it with DEVOPTS=fanalyzer.\n\nIf you're CC'd on this series you were either involved in some\ndiscussion about -fanalyzer on the ML, or one of the commits mentioned\nin this series is yours. Comments on individual sets of patches below:\n\nÆvar Arnfjörð Bjarmason (15):\n remote.c: don't dereference NULL in freeing loop\n pull.c: don't feed NULL to strcmp() on get_rebase_fork_point() path\n reftable: don't memset() a NULL from failed malloc()\n\nThese are all real bugs, in rough order of more to less severe.\n\n diff-lib.c: don't dereference NULL in oneway_diff()\n refs/packed-backend.c: add a BUG() if iter is NULL\n ref-filter.c: BUG() out on show_ref() with NULL refname\n\nThese may or may not be real bugs, I'm pretty sure they are...\n\n strbuf.c: placate -fanalyzer in strbuf_grow()\n\nThe first of cases where -fanalyzer isn't \"wrong\", but it is in the\nsense that the error it points out is a hard constraint that we\n(hopefully) ensure elsewhere in various APIs.\n\n strbuf.c: use st_add3(), not unsigned_add_overflows()\n\nA while-we're-at-it for code spotted in the last commit.\n\n add-patch: assert parse_diff() expectations with BUG()\n reftable: don't have reader_get_block() confuse -fanalyzer\n blame.c: clarify the state of \"final_commit\" for -fanalyzer\n pack.h: wrap write_*file*() functions\n pack-write API: pass down \"verify\" not arbitrary flags\n\nThese are all cases where -fanalyzer flags \"genuine\" issues, but on\ncloser inspection it's because we're passing the equivalent of two\nvariables we know go hand-in-hand, but probably no compiler can be\nsmart enough to spot that.\n\nE.g. the first one here is a case where \"git add -p\" would segfault if\nfed diffs that aren't in the format \"git diff\" would emit, but since\nwe control both sides it doesn't happen in practice.\n\nRegardless, sprinkling assertions and BUG() guards in that code seems\nprudent.\n\n config.mak.dev: add a DEVOPTS=analyzer mode to use GCC's -fanalyzer\n\nAdd the new DEVOPTS mode and quiet some issues that need -Wno-error=*.\n\n config.mak.dev: add and use ASSERT_FOR_FANALYZER() macro\n\nQuiet some outstanding issues, the main reason these aren't in commits\nlike the above is because I didn't get to them yet, and may not any\ntime soon. They may be genuine bugs, false alarms etc.\n\nEven if we don't get to these I think having a CI mode with -fanalyzer\nand a whitelist of current issues would be an improment, since we'd\nerror on *new* isssues.\n\nThis series doesn't add such a CI mode yet, due to conflicts with CI\nwork in \"next\"..\n\nÆvar Arnfjörð Bjarmason (15):\n remote.c: don't dereference NULL in freeing loop\n pull.c: don't feed NULL to strcmp() on get_rebase_fork_point() path\n reftable: don't memset() a NULL from failed malloc()\n diff-lib.c: don't dereference NULL in oneway_diff()\n refs/packed-backend.c: add a BUG() if iter is NULL\n ref-filter.c: BUG() out on show_ref() with NULL refname\n strbuf.c: placate -fanalyzer in strbuf_grow()\n strbuf.c: use st_add3(), not unsigned_add_overflows()\n add-patch: assert parse_diff() expectations with BUG()\n reftable: don't have reader_get_block() confuse -fanalyzer\n blame.c: clarify the state of \"final_commit\" for -fanalyzer\n pack.h: wrap write_*file*() functions\n pack-write API: pass down \"verify\" not arbitrary flags\n config.mak.dev: add a DEVOPTS=analyzer mode to use GCC's -fanalyzer\n config.mak.dev: add and use ASSERT_FOR_FANALYZER() macro\n\n Makefile | 14 +++++++++\n add-patch.c | 7 ++++-\n blame.c | 2 +-\n builtin/fast-import.c | 2 +-\n builtin/index-pack.c | 35 +++++++++-------------\n builtin/name-rev.c | 1 +\n builtin/pack-objects.c | 9 ++----\n builtin/pull.c | 14 +++++----\n config.mak.dev | 65 +++++++++++++++++++++++++++++++++++++++++\n diff-lib.c | 3 ++\n dir.c | 1 +\n git-compat-util.h | 16 ++++++++++\n gpg-interface.c | 2 ++\n graph.c | 2 ++\n line-log.c | 2 ++\n midx.c | 2 +-\n pack-write.c | 38 +++++++++++-------------\n pack.h | 24 +++++++++------\n ref-filter.c | 4 ++-\n refs/packed-backend.c | 2 ++\n reftable/publicbasics.c | 2 ++\n reftable/reader.c | 11 +++----\n remote.c | 8 ++---\n strbuf.c | 10 ++++---\n unpack-trees.c | 1 +\n utf8.c | 1 +\n 26 files changed, 195 insertions(+), 83 deletions(-)\n\n-- \n2.36.1.1124.g577fa9c2ebd\n\n"},{"id":"456606","messageId":"RFC-patch-01.15-b3a678d934a-20220603T183608Z-avarab@gmail.com","threadId":"57952","inReplyTo":"RFC-cover-00.15-00000000000-20220603T183608Z-avarab@gmail.com","subject":"[RFC PATCH 01/15] remote.c: don't dereference NULL in freeing loop","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-03T18:37:38Z","receivedAt":"2022-06-03T18:39:19Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a bug in fd3cb0501e1 (remote: move static variables into\nper-repository struct, 2021-11-17) where we'd free(remote->pushurl[i])\nafter having NULL'd out remote->pushurl. itself.\n\nWhile we're at it let's get rid of the redundant braces per the\nCodingGuidelines, which also serves to show in the diff context that\nwe were doing a FREE_AND_NULL(remote->pushurl) afterwards too, let's\nkeep that one.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n remote.c | 8 ++------\n 1 file changed, 2 insertions(+), 6 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex 930fdc9c2f6..61240562df1 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -144,14 +144,10 @@ static void remote_clear(struct remote *remote)\n \tfree((char *)remote->name);\n \tfree((char *)remote->foreign_vcs);\n \n-\tfor (i = 0; i < remote->url_nr; i++) {\n+\tfor (i = 0; i < remote->url_nr; i++)\n \t\tfree((char *)remote->url[i]);\n-\t}\n-\tFREE_AND_NULL(remote->pushurl);\n-\n-\tfor (i = 0; i < remote->pushurl_nr; i++) {\n+\tfor (i = 0; i < remote->pushurl_nr; i++)\n \t\tfree((char *)remote->pushurl[i]);\n-\t}\n \tFREE_AND_NULL(remote->pushurl);\n \tfree((char *)remote->receivepack);\n \tfree((char *)remote->uploadpack);\n-- \n2.36.1.1124.g577fa9c2ebd\n\n"},{"id":"456607","messageId":"RFC-patch-02.15-4a055969ea5-20220603T183608Z-avarab@gmail.com","threadId":"57952","inReplyTo":"RFC-cover-00.15-00000000000-20220603T183608Z-avarab@gmail.com","subject":"[RFC PATCH 02/15] pull.c: don't feed NULL to strcmp() on get_rebase_fork_point() path","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-03T18:37:39Z","receivedAt":"2022-06-03T18:39:22Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix an issue with feeding NULL to strcmp() noted by GCC's -fanalyzer,\nthis fixes a bug in 1678b81ecce (pull: teach git pull about --rebase,\n2015-06-18).\n\nIn cmd_pull() we could go through the function without initializing\nthe \"repo\" argument (the -fanalyzer output shows how exactly), we'd\nthen call get_rebase_fork_point (), which would in turn call\nget_tracking_branch() with that \"NULL\" repo argument.\n\nLet's avoid this potential issue by returning NULL in this case, which\nwill have get_rebase_fork_point() return -1 in turn.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/pull.c | 14 ++++++++------\n 1 file changed, 8 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 01155ba67b2..ed8df004028 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -756,14 +756,16 @@ static const char *get_tracking_branch(const char *remote, const char *refspec)\n \t\tstarts_with(spec_src, \"remotes/\"))\n \t\tspec_src = \"\";\n \n-\tif (*spec_src) {\n-\t\tif (!strcmp(remote, \".\"))\n-\t\t\tmerge_branch = mkpath(\"refs/heads/%s\", spec_src);\n-\t\telse\n-\t\t\tmerge_branch = mkpath(\"refs/remotes/%s/%s\", remote, spec_src);\n-\t} else\n+\tif ((*spec_src && !remote) || !*spec_src) {\n \t\tmerge_branch = NULL;\n+\t\tgoto cleanup;\n+\t}\n \n+\tif (!strcmp(remote, \".\"))\n+\t\tmerge_branch = mkpath(\"refs/heads/%s\", spec_src);\n+\telse\n+\t\tmerge_branch = mkpath(\"refs/remotes/%s/%s\", remote, spec_src);\n+cleanup:\n \trefspec_item_clear(&spec);\n \treturn merge_branch;\n }\n-- \n2.36.1.1124.g577fa9c2ebd\n\n"},{"id":"456608","messageId":"RFC-patch-03.15-0b570d112fc-20220603T183608Z-avarab@gmail.com","threadId":"57952","inReplyTo":"RFC-cover-00.15-00000000000-20220603T183608Z-avarab@gmail.com","subject":"[RFC PATCH 03/15] reftable: don't memset() a NULL from failed malloc()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-03T18:37:40Z","receivedAt":"2022-06-03T18:39:25Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Return NULL instead of possibly feeding a NULL to memset() in\nreftable_calloc(). This issue was noted by GCC 12's -fanalyzer:\n\n\treftable/publicbasics.c: In function ‘reftable_calloc’:\n\treftable/publicbasics.c:43:9: error: use of possibly-NULL ‘p’ where non-null expected [CWE-690] [-Werror=analyzer-possible-null-argument]\n\t 43 | memset(p, 0, sz);\n\t | ^~~~~~~~~~~~~~~~\n\t[...]\n\nThis bug has been with us ever since this code was added in\nef8a6c62687 (reftable: utility functions, 2021-10-07).\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n reftable/publicbasics.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/reftable/publicbasics.c b/reftable/publicbasics.c\nindex 0ad7d5c0ff2..a18167f5ab7 100644\n--- a/reftable/publicbasics.c\n+++ b/reftable/publicbasics.c\n@@ -40,6 +40,8 @@ void reftable_free(void *p)\n void *reftable_calloc(size_t sz)\n {\n \tvoid *p = reftable_malloc(sz);\n+\tif (!p)\n+\t\treturn NULL;\n \tmemset(p, 0, sz);\n \treturn p;\n }\n-- \n2.36.1.1124.g577fa9c2ebd\n\n"},{"id":"456609","messageId":"RFC-patch-04.15-3a287c19d7e-20220603T183608Z-avarab@gmail.com","threadId":"57952","inReplyTo":"RFC-cover-00.15-00000000000-20220603T183608Z-avarab@gmail.com","subject":"[RFC PATCH 04/15] diff-lib.c: don't dereference NULL in oneway_diff()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-03T18:37:41Z","receivedAt":"2022-06-03T18:39:27Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a control flow issue dating back to d1f2d7e8ca6 (Make\nrun_diff_index() use unpack_trees(), not read_tree(), 2008-01-19)\nwhere we'd assume \"tree\" must be non-NULL if idx was NULL. As\n-fanalyzer shows we'd end up dereferencing \"tree\" in that case in\nce_path_match():\n\ndir.h:541:41: warning: dereference of NULL ‘ce’ [CWE-476] [-Wanalyzer-null-dereference]\n 541 | S_ISDIR(ce->ce_mode) || S_ISGITLINK(ce->ce_mode));\n | ^\n ‘oneway_diff’: events 1-2\n |\n |diff-lib.c:493:12:\n | 493 | static int oneway_diff(const struct cache_entry * const *src,\n | | ^~~~~~~~~~~\n | | |\n | | (1) entry to ‘oneway_diff’\n |......\n | 506 | if (tree == o->df_conflict_entry)\n | | ~\n | | |\n | | (2) following ‘true’ branch...\n |\n ‘oneway_diff’: event 3\n |\n | 507 | tree = NULL;\n | | ^\n | | |\n | | (3) ...to here\n |\n ‘oneway_diff’: events 4-8\n |\n | 507 | tree = NULL;\n | | ^\n | | |\n | | (4) ‘tree’ is NULL\n | 508 |\n | 509 | if (ce_path_match(revs->diffopt.repo->index,\n | | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n | | |\n | | (5) following ‘false’ branch (when ‘idx’ is NULL)...\n | | (6) ...to here\n | | (7) ‘tree’ is NULL\n | | (8) calling ‘ce_path_match’ from ‘oneway_diff’\n | 510 | idx ? idx : tree,\n | | ~~~~~~~~~~~~~~~~~\n | 511 | &revs->prune_data, NULL)) {\n | | ~~~~~~~~~~~~~~~~~~~~~~~~\n |\n +--> ‘ce_path_match’: event 9\n |\n |dir.h:535:19:\n | 535 | static inline int ce_path_match(struct index_state *istate,\n | | ^~~~~~~~~~~~~\n | | |\n | | (9) entry to ‘ce_path_match’\n |\n ‘ce_path_match’: event 10\n |\n | 541 | S_ISDIR(ce->ce_mode) || S_ISGITLINK(ce->ce_mode));\n | | ^\n | | |\n | | (10) dereference of NULL ‘ce\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n diff-lib.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex ca085a03efc..8373ad7e3ea 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -506,6 +506,9 @@ static int oneway_diff(const struct cache_entry * const *src,\n \tif (tree == o->df_conflict_entry)\n \t\ttree = NULL;\n \n+\tif (!idx && !tree)\n+\t\treturn 0;\n+\n \tif (ce_path_match(revs->diffopt.repo->index,\n \t\t\t idx ? idx : tree,\n \t\t\t &revs->prune_data, NULL)) {\n-- \n2.36.1.1124.g577fa9c2ebd\n\n"},{"id":"456610","messageId":"RFC-patch-05.15-46e0c307941-20220603T183608Z-avarab@gmail.com","threadId":"57952","inReplyTo":"RFC-cover-00.15-00000000000-20220603T183608Z-avarab@gmail.com","subject":"[RFC PATCH 05/15] refs/packed-backend.c: add a BUG() if iter is NULL","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-03T18:37:42Z","receivedAt":"2022-06-03T18:39:29Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Adjust code added in 2775d8724d7 (packed_ref_store: implement\nreference transactions, 2017-09-08) to BUG() out in a case there GCC\nv12's -fanalyzer flagged that the \"iter->oid\" seen in the context was\nreachable where iter was NULL.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n refs/packed-backend.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 97b68377673..65991bbcaf5 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -1226,6 +1226,8 @@ static int write_with_updates(struct packed_ref_store *refs,\n \t\t\tstruct object_id peeled;\n \t\t\tint peel_error = ref_iterator_peel(iter, &peeled);\n \n+\t\t\tif (!iter)\n+\t\t\t\tBUG(\"must have iter if cmp < 0\");\n \t\t\tif (write_packed_entry(out, iter->refname,\n \t\t\t\t\t iter->oid,\n \t\t\t\t\t peel_error ? NULL : &peeled))\n-- \n2.36.1.1124.g577fa9c2ebd\n\n"},{"id":"456611","messageId":"RFC-patch-06.15-2d04035d7aa-20220603T183608Z-avarab@gmail.com","threadId":"57952","inReplyTo":"RFC-cover-00.15-00000000000-20220603T183608Z-avarab@gmail.com","subject":"[RFC PATCH 06/15] ref-filter.c: BUG() out on show_ref() with NULL refname","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-03T18:37:43Z","receivedAt":"2022-06-03T18:39:35Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Adjust code originally added in 5339bdad96a (ref-filter: introduce\nremote_ref_atom_parser(), 2016-02-17) to BUG() out rather than\npotentially segfault if we get a NULL refname here.\n\nAs noted by GCC v12's -fanalyzer that will happen if this follows the\n\"refname = NULL\" branch added in cc72385fe35 (for-each-ref: let\nupstream/push optionally report the remote name, 2017-10-05).\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n ref-filter.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 2413f889f48..91aa8e89268 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1644,7 +1644,9 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname,\n \t\t\t\t struct branch *branch, const char **s)\n {\n \tint num_ours, num_theirs;\n-\tif (atom->u.remote_ref.option == RR_REF)\n+\tif (atom->u.remote_ref.option == RR_REF && !refname)\n+\t\tBUG(\"must get refname with [...]remote_ref.option == RR_REF\");\n+\telse if (atom->u.remote_ref.option == RR_REF)\n \t\t*s = show_ref(&atom->u.remote_ref.refname, refname);\n \telse if (atom->u.remote_ref.option == RR_TRACK) {\n \t\tif (stat_tracking_info(branch, &num_ours, &num_theirs,\n-- \n2.36.1.1124.g577fa9c2ebd\n\n"},{"id":"456612","messageId":"RFC-patch-07.15-cf1a5f3ed0f-20220603T183608Z-avarab@gmail.com","threadId":"57952","inReplyTo":"RFC-cover-00.15-00000000000-20220603T183608Z-avarab@gmail.com","subject":"[RFC PATCH 07/15] strbuf.c: placate -fanalyzer in strbuf_grow()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-03T18:37:44Z","receivedAt":"2022-06-03T18:39:37Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the strbuf_grow() function so that GCC v12's -fanalyze doesn't\nyell at us about sb->buf[0] dereferencing NULL, this also makes this\ncode easier to follow.\n\nThis BUG() should be unreachable since the state of our \"sb->buf\" and\n\"sb->alloc\" goes hand-in-hand, but -fanalyzer isn't smart enough to\nknow that, and adding the BUG() also makes it clearer to human readers\nthat that's what happens here.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n strbuf.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/strbuf.c b/strbuf.c\nindex dd9eb85527a..61c4630aeeb 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -97,6 +97,8 @@ void strbuf_grow(struct strbuf *sb, size_t extra)\n \tif (new_buf)\n \t\tsb->buf = NULL;\n \tALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc);\n+\tif (new_buf && !sb->buf)\n+\t\tBUG(\"for a new buffer ALLOC_GROW() should always do work!\");\n \tif (new_buf)\n \t\tsb->buf[0] = '\\0';\n }\n-- \n2.36.1.1124.g577fa9c2ebd\n\n"},{"id":"456613","messageId":"RFC-patch-08.15-2c4b7832144-20220603T183608Z-avarab@gmail.com","threadId":"57952","inReplyTo":"RFC-cover-00.15-00000000000-20220603T183608Z-avarab@gmail.com","subject":"[RFC PATCH 08/15] strbuf.c: use st_add3(), not unsigned_add_overflows()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-03T18:37:45Z","receivedAt":"2022-06-03T18:39:39Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the strbuf_grow() function to use st_add3() instead of doing\nits own unsigned_add_overflows() checks. The overflow checking here\nwas originally added in b449f4cfc97 (Rework strbuf API and semantics.,\n2007-09-06) and adjusted in 1368f65002b (compat: helper for detecting\nunsigned overflow, 2010-10-10). Instead we compute a \"sz\" with\nst_add3().\n\nThat was done at a time when the underlying xrealloc() in\ngit-compat-util.h didn't use st_mult() yet, that has been the case\nsince the later e7792a74bcf (harden REALLOC_ARRAY and xcalloc against\nsize_t overflow, 2016-02-22).\n\nThe only behavior change here should be the very obscure edge case\nthat we'd previously die() in cases where we strictly didn't need to,\nas we'd check both \"extra + 1\" and \"sb->len + extra + 1\" for\noverflow. If we overflowed only on the latter but were doing the\nformer we'd needlessly die() die. I don't think that difference\nmattered, but it's noted here for completeness.\n\nWhile we're at it add an inline comment about why we're adding 1 to\nthe values, that's also explained in the API documentation in\nstrbuf.h, but since we're using that magic constant here...\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n strbuf.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/strbuf.c b/strbuf.c\nindex 61c4630aeeb..f0a74d2bfb1 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -91,12 +91,12 @@ void strbuf_attach(struct strbuf *sb, void *buf, size_t len, size_t alloc)\n void strbuf_grow(struct strbuf *sb, size_t extra)\n {\n \tint new_buf = !sb->alloc;\n-\tif (unsigned_add_overflows(extra, 1) ||\n-\t unsigned_add_overflows(sb->len, extra + 1))\n-\t\tdie(\"you want to use way too much memory\");\n+\tconst size_t sz_buf = new_buf ? 0 : sb->len;\n+\tconst size_t sz = st_add3(sz_buf, extra, 1 /* for \\0 */);\n+\n \tif (new_buf)\n \t\tsb->buf = NULL;\n-\tALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc);\n+\tALLOC_GROW(sb->buf, sz, sb->alloc);\n \tif (new_buf && !sb->buf)\n \t\tBUG(\"for a new buffer ALLOC_GROW() should always do work!\");\n \tif (new_buf)\n-- \n2.36.1.1124.g577fa9c2ebd\n\n"},{"id":"456614","messageId":"RFC-patch-09.15-de0f7722608-20220603T183608Z-avarab@gmail.com","threadId":"57952","inReplyTo":"RFC-cover-00.15-00000000000-20220603T183608Z-avarab@gmail.com","subject":"[RFC PATCH 09/15] add-patch: assert parse_diff() expectations with BUG()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-03T18:37:46Z","receivedAt":"2022-06-03T18:39:40Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Assert that this code added in [1], [2] and other related commits\nexpects that once we see a \"diff \" line we should have a non-NULL\n\"file_diff\" and \"hunk\".\n\nIn practice this would have always been the case, as we are parsing\nour own \"diff\" output, but e.g. GCC v12's -fanalyzer doesn't know\nthat, and will alert us that in the \"else if\" and below in this\nfunction we could be dereferencing NULL if we were processing anything\nexcept our expected input.\n\n1. f6aa7ecc343 (built-in add -i: start implementing the `patch`\n functionality in C, 2019-12-13)\n2. 80399aec5ab (built-in add -p: support multi-file diffs, 2019-12-13)\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n add-patch.c | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex 55d719f7845..087bf317b07 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -478,11 +478,16 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)\n \twhile (p != pend) {\n \t\tchar *eol = memchr(p, '\\n', pend - p);\n \t\tconst char *deleted = NULL, *mode_change = NULL;\n+\t\tconst char *const diff_l = \"diff \";\n+\t\tint is_diff_line = starts_with(p, diff_l);\n \n \t\tif (!eol)\n \t\t\teol = pend;\n \n-\t\tif (starts_with(p, \"diff \")) {\n+\t\tif (!is_diff_line && (!file_diff || !hunk))\n+\t\t\tBUG(\"expected '%s' line to follow a '%s' line\", p, diff_l);\n+\n+\t\tif (is_diff_line) {\n \t\t\tcomplete_file(marker, hunk);\n \t\t\tALLOC_GROW_BY(s->file_diff, s->file_diff_nr, 1,\n \t\t\t\t file_diff_alloc);\n-- \n2.36.1.1124.g577fa9c2ebd\n\n"},{"id":"456615","messageId":"RFC-patch-10.15-b50558d3b24-20220603T183608Z-avarab@gmail.com","threadId":"57952","inReplyTo":"RFC-cover-00.15-00000000000-20220603T183608Z-avarab@gmail.com","subject":"[RFC PATCH 10/15] reftable: don't have reader_get_block() confuse -fanalyzer","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-03T18:37:47Z","receivedAt":"2022-06-03T18:39:43Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the control flow in reftable code add in 46bc0e731a7 (reftable:\nread reftable files, 2021-10-07) to work around a false positive\nspotted by GCC's -fanalyzer option[1]. The code was added in\n46bc0e731a7 (reftable: read reftable files, 2021-10-07).\n\nUsually we'd just leave such false positives alone, but having looked\nat it the control flow was also odd and confusing to humans, so let's\nchange it.\n\nWhat -fanalyzer complained about was:\n\n\t|......\n\t| 294 | if (next_off >= r_size)\n\t| | ~\n\t| | |\n\t| | (24) following ‘false’ branch (when ‘next_off < r_size’)...\n\t|......\n\t| 294 | if (next_off >= r_size)\n\t| | ~\n\t| | |\n\t| | (24) following ‘false’ branch (when ‘next_off < r_size’)...\n\t|......\n\t| 297 | err = reader_get_block(r, &block, next_off, guess_block_size, r->size);\n\t| | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n\t| | |\n\t| | (25) ...to here\n\t| | (26) calling ‘reader_get_block’ from ‘reader_init_block_reader’\n\t|\n\t+--> ‘reader_get_block’: events 27-29\n\t |\n\t | 59 | static int reader_get_block(struct reftable_reader *r,\n\t | | ^~~~~~~~~~~~~~~~\n\t | | |\n\t | | (27) entry to ‘reader_get_block’\n\t |......\n\t | 63 | if (off >= r_size)\n\t | | ~\n\t | | |\n\t | | (28) following ‘true’ branch (when ‘off >= r_size’)...\n\t | 64 | return 0;\n\t | | ~\n\t | | |\n\t | | (29) ...to here\n\t [...]\n\t | 275 | *typ = data[0];\n\t | | ~~~~~~~\n\t | | |\n\t | | (37) ...to here\n\t | | (38) dereference of NULL ‘data’\n\nI.e. it thought that we could take the \"return 0\" in\nreader_get_block() due to \"(off >= r->size)\", which followed the\nidentical \"(next_off >= r_size)\" check in reader_init_block_reader()\njust before reader_get_block() was called.\n\nBut whatever GCC's -fanalyzer thinks it's confusing that we're\nchecking this twice, and making it look as though these parameters\nmight change within the reader_init_block_reader() function, but they\nwon't. So let's just do the check once early, and use \"const\" types to\nassert that they'll be constant throughout.\n\n1. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105285\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n reftable/reader.c | 11 ++++++-----\n 1 file changed, 6 insertions(+), 5 deletions(-)\n\ndiff --git a/reftable/reader.c b/reftable/reader.c\nindex 54b4025105c..e1649929b30 100644\n--- a/reftable/reader.c\n+++ b/reftable/reader.c\n@@ -58,9 +58,9 @@ reader_offsets_for(struct reftable_reader *r, uint8_t typ)\n \n static int reader_get_block(struct reftable_reader *r,\n \t\t\t struct reftable_block *dest, uint64_t off,\n-\t\t\t uint32_t sz)\n+\t\t\t uint32_t sz, uint64_t r_size)\n {\n-\tif (off >= r->size)\n+\tif (off >= r_size)\n \t\treturn 0;\n \n \tif (off + sz > r->size) {\n@@ -281,6 +281,7 @@ static int32_t extract_block_size(uint8_t *data, uint8_t *typ, uint64_t off,\n int reader_init_block_reader(struct reftable_reader *r, struct block_reader *br,\n \t\t\t uint64_t next_off, uint8_t want_typ)\n {\n+\tconst uint64_t r_size = r->size;\n \tint32_t guess_block_size = r->block_size ? r->block_size :\n \t\t\t\t\t\t\t DEFAULT_BLOCK_SIZE;\n \tstruct reftable_block block = { NULL };\n@@ -289,10 +290,10 @@ int reader_init_block_reader(struct reftable_reader *r, struct block_reader *br,\n \tuint32_t header_off = next_off ? 0 : header_size(r->version);\n \tint32_t block_size = 0;\n \n-\tif (next_off >= r->size)\n+\tif (next_off >= r_size)\n \t\treturn 1;\n \n-\terr = reader_get_block(r, &block, next_off, guess_block_size);\n+\terr = reader_get_block(r, &block, next_off, guess_block_size, r_size);\n \tif (err < 0)\n \t\tgoto done;\n \n@@ -309,7 +310,7 @@ int reader_init_block_reader(struct reftable_reader *r, struct block_reader *br,\n \n \tif (block_size > guess_block_size) {\n \t\treftable_block_done(&block);\n-\t\terr = reader_get_block(r, &block, next_off, block_size);\n+\t\terr = reader_get_block(r, &block, next_off, block_size, r_size);\n \t\tif (err < 0) {\n \t\t\tgoto done;\n \t\t}\n-- \n2.36.1.1124.g577fa9c2ebd\n\n"},{"id":"456617","messageId":"RFC-patch-11.15-66518467e1d-20220603T183608Z-avarab@gmail.com","threadId":"57952","inReplyTo":"RFC-cover-00.15-00000000000-20220603T183608Z-avarab@gmail.com","subject":"[RFC PATCH 11/15] blame.c: clarify the state of \"final_commit\" for -fanalyzer","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-03T18:37:48Z","receivedAt":"2022-06-03T18:40:15Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change code added in d0d0ef1f67c (blame: create scoreboard setup\nfunction, 2017-05-24) so that GCC v12's -fanalyzer doesn't think that\nwe can have a \"NULL\" final_commit in the modified branch.\n\nThis happens because the analyzer gives up in the\nprepare_revision_walk() function, and thinks that the \"sb->reverse &&\nsb->revs->first_parent_only\" condition we already checked a few lines\nabove can have a different result at this point.\n\nThat isn't the case, but what we really mean here is \"if we previously\nset up the final commit [because that was true]\", so let's do that\ninstead.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n blame.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/blame.c b/blame.c\nindex da1052ac94b..f6b1865ba65 100644\n--- a/blame.c\n+++ b/blame.c\n@@ -2816,7 +2816,7 @@ void setup_scoreboard(struct blame_scoreboard *sb,\n \tif (prepare_revision_walk(sb->revs))\n \t\tdie(_(\"revision walk setup failed\"));\n \n-\tif (sb->reverse && sb->revs->first_parent_only) {\n+\tif (final_commit) {\n \t\tstruct commit *c = final_commit;\n \n \t\tsb->revs->children.name = \"children\";\n-- \n2.36.1.1124.g577fa9c2ebd\n\n"},{"id":"456616","messageId":"RFC-patch-14.15-9cf550688d4-20220603T183608Z-avarab@gmail.com","threadId":"57952","inReplyTo":"RFC-cover-00.15-00000000000-20220603T183608Z-avarab@gmail.com","subject":"[RFC PATCH 14/15] config.mak.dev: add a DEVOPTS=analyzer mode to use GCC's -fanalyzer","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-03T18:37:51Z","receivedAt":"2022-06-03T18:40:16Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add an option to compile with GCC's -fanalyzer, which as noted in [1]\nand [2] has become much more useful in the recently released GCC v12\nseries.\n\nHere we're quieting a few outstanding -fanalyzer issues that require\nus to use -Wno-error=* on an entire file:\n\n * range-diff.c, because it involves loop variables and would be\n painfully verbose to instrument with the ASSERT_FOR_FANALYZER() macro\n introduced in the subsequent commit.\n\n * http-fetch.c and fsmonitor-settings.c, because those aren't issues\n where we're referencing NULL, and therefore we can't quiet it with an\n assert().\n\nFor non-GCC compilers I considered wrapping the DEVOPTS logic in:\n\n\tifneq ($(filter gcc10,$(COMPILER_FEATURES)),)\n\tendif\n\nWhich would make it OK to specify DEVOPTS=analyzer under other\ncompilers, or on older GCC. But then we'd silently ignore the option\non those. Let's instead trust the compiler to error out if it doesn't\nsupport -fanalyzer.\n\nThere are various limitations and bugs in the analyzer engine, e.g. I\nfiled [3] for a false positive in builtin/merge-file.c before GCC v12\nwas released, which was subsequently fixed in GCC 12 trunk in [4], but\nmany other issues remain.\n\n1. https://developers.redhat.com/articles/2022/04/12/state-static-analysis-gcc-12-compiler\n2. https://gcc.gnu.org/gcc-12/changes.html\n3. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105264\n4. https://gcc.gnu.org/git/gitweb.cgi?p=gcc.git;h=a358e4b60815b41e27f3508014ceb592f86b9b45\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Makefile | 14 ++++++++++++\n config.mak.dev | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 75 insertions(+)\n\ndiff --git a/Makefile b/Makefile\nindex 18ca6744a50..129d55f5992 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -507,6 +507,20 @@ include shared.mak\n # no-pedantic:\n #\n # Disable -pedantic compilation.\n+#\n+# analyzer:\n+#\n+# Compile with GCC's -fanalyzer, this analysis is much more\n+# expensive than other GCC warnings.\n+#\n+# The set of analysis flags is curated based on known issues\n+# and compiler version. Known issues are made into non-fatal\n+# warnings (even \"no-error\" isn't set).\n+#\n+# no-suppress-analyzer:\n+#\n+# When using \"analyzer\" disable the suppression of known\n+# -fanalyzer issues.\n \n GIT-VERSION-FILE: FORCE\n \t@$(SHELL_PATH) ./GIT-VERSION-GEN\ndiff --git a/config.mak.dev b/config.mak.dev\nindex c3104f400b2..d6f5be92297 100644\n--- a/config.mak.dev\n+++ b/config.mak.dev\n@@ -72,3 +72,64 @@ DEVELOPER_CFLAGS += -Wno-error=dangling-pointer\n endif\n \n GIT_TEST_PERL_FATAL_WARNINGS = YesPlease\n+\n+# GCC's -fanalyzer mode\n+ifeq ($(filter analyzer,$(DEVOPTS)),analyzer)\n+\n+ifeq ($(filter gcc1,$(COMPILER_FEATURES)),)\n+$(error you must be using a new-ish version of GCC for DEVOPTS=analyzer, your \\\n+$(CC) is not GCC at all!)\n+endif\n+\n+DEVELOPER_CFLAGS += -fanalyzer\n+\n+## -fanalyzer exists exists as of gcc10, but versions older than gcc12\n+## have a lot of false positives.\n+ifeq ($(filter gcc12,$(COMPILER_FEATURES)),)\n+DEVELOPER_CFLAGS += -Wno-analyzer-double-free\n+DEVELOPER_CFLAGS += -Wno-analyzer-free-of-non-heap\n+endif\n+\n+## Helper templates to whitelist existing issues\n+define fn_disable_analyzer_tmpl\n+$(1).sp $(1).s $(1).o: EXTRA_CPPFLAGS += $(2)\n+\n+endef\n+\n+define fn_disable_analyzer\n+$(foreach f,$(2),$(call fn_disable_analyzer_tmpl,$(f),$(1)))\n+endef\n+\n+## -Wno-error=analyzer-null-dereference\n+$(eval $(call fn_disable_analyzer, \\\n+\t-Wno-error=analyzer-null-dereference, \\\n+\trange-diff \\\n+))\n+## -Wno-error=analyzer-malloc-leak\n+$(eval $(call fn_disable_analyzer, \\\n+\t-Wno-error=analyzer-malloc-leak, \\\n+\tfsmonitor-settings \\\n+))\n+## per-GCC version annotations\n+### -Wno-error=analyzer-use-of-uninitialized-value: gcc >= 12\n+ifneq ($(filter gcc12,$(COMPILER_FEATURES)),)\n+$(eval $(call fn_disable_analyzer, \\\n+\t-Wno-error=analyzer-use-of-uninitialized-value, \\\n+\thttp-fetch \\\n+))\n+else # gcc < 12\n+### -Wno-error=analyzer-null-dereference: gcc == 11\n+ifneq ($(filter gcc11,$(COMPILER_FEATURES)),)\n+$(eval $(call fn_disable_analyzer, \\\n+\t-Wno-error=analyzer-null-dereference, \\\n+\tmerge \\\n+\txdiff/xemit \\\n+\treftable/reader \\\n+))\n+else\n+$(error Your GCC version is too old for -fanalyze, or you are using \\\n+gcc10 which has it, but has too many false positives!)\n+endif\n+endif # gcc < 12\n+\n+endif\n-- \n2.36.1.1124.g577fa9c2ebd\n\n"},{"id":"456618","messageId":"RFC-patch-12.15-9f0f515ed3a-20220603T183608Z-avarab@gmail.com","threadId":"57952","inReplyTo":"RFC-cover-00.15-00000000000-20220603T183608Z-avarab@gmail.com","subject":"[RFC PATCH 12/15] pack.h: wrap write_*file*() functions","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-03T18:37:49Z","receivedAt":"2022-06-03T18:40:17Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Whitespace-wrap some overly long write_*file*() lines in preparation\nfor changing their signatures in a subsequent commit. I named the\n\"opts\" argument to \"write_idx_file()\" while I was at it for\nconsistency.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n pack.h | 15 ++++++++++++---\n 1 file changed, 12 insertions(+), 3 deletions(-)\n\ndiff --git a/pack.h b/pack.h\nindex b22bfc4a18e..802c047efe6 100644\n--- a/pack.h\n+++ b/pack.h\n@@ -81,7 +81,10 @@ struct progress;\n /* Note, the data argument could be NULL if object type is blob */\n typedef int (*verify_fn)(const struct object_id *, enum object_type, unsigned long, void*, int*);\n \n-const char *write_idx_file(const char *index_name, struct pack_idx_entry **objects, int nr_objects, const struct pack_idx_option *, const unsigned char *sha1);\n+const char *write_idx_file(const char *index_name,\n+\t\t\t struct pack_idx_entry **objects, int nr_objects,\n+\t\t\t const struct pack_idx_option *opts,\n+\t\t\t const unsigned char *sha1);\n int check_pack_crc(struct packed_git *p, struct pack_window **w_curs, off_t offset, off_t len, unsigned int nr);\n int verify_pack_index(struct packed_git *);\n int verify_pack(struct repository *, struct packed_git *, verify_fn fn, struct progress *, uint32_t);\n@@ -93,8 +96,14 @@ struct ref;\n \n void write_promisor_file(const char *promisor_name, struct ref **sought, int nr_sought);\n \n-const char *write_rev_file(const char *rev_name, struct pack_idx_entry **objects, uint32_t nr_objects, const unsigned char *hash, unsigned flags);\n-const char *write_rev_file_order(const char *rev_name, uint32_t *pack_order, uint32_t nr_objects, const unsigned char *hash, unsigned flags);\n+const char *write_rev_file(const char *rev_name,\n+\t\t\t struct pack_idx_entry **objects,\n+\t\t\t uint32_t nr_objects,\n+\t\t\t const unsigned char *hash, unsigned flags);\n+const char *write_rev_file_order(const char *rev_name, uint32_t *pack_order,\n+\t\t\t\t uint32_t nr_objects,\n+\t\t\t\t const unsigned char *hash,\n+\t\t\t\t unsigned flags);\n \n /*\n * The \"hdr\" output buffer should be at least this big, which will handle sizes\n-- \n2.36.1.1124.g577fa9c2ebd\n\n"},{"id":"456620","messageId":"RFC-patch-13.15-63eeb66185a-20220603T183608Z-avarab@gmail.com","threadId":"57952","inReplyTo":"RFC-cover-00.15-00000000000-20220603T183608Z-avarab@gmail.com","subject":"[RFC PATCH 13/15] pack-write API: pass down \"verify\" not arbitrary flags","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-03T18:37:50Z","receivedAt":"2022-06-03T18:40:54Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the pack-write API to accept a boolean \"verify\" parameter\ninstead of passing down the \"struct pack_idx_option\" flags directly.\n\nThis simplifies the code for both humans and machines, e.g. GCC's\n-fanalyzer would correctly note that there were potential paths\nthrough this code where we'd deference NULL, but in reality we\nwouldn't hit them because certain flags would always go hand-in-hand.\n\nLet's instead separate the underlying API from the total set of flags,\nand stop passing the flags down to functions that don't need the full\nset of flags, they'll just get the specific state they need.\n\nSee e37d0b8730b (builtin/index-pack.c: write reverse indexes,\n2021-01-25) for the initial implementation, and [1] for the initial\nWIP version of this commit on the ML.\n\n1. https://lore.kernel.org/git/87v93bidhn.fsf@evledraar.gmail.com/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/fast-import.c | 2 +-\n builtin/index-pack.c | 35 ++++++++++++++---------------------\n builtin/pack-objects.c | 9 +++------\n midx.c | 2 +-\n pack-write.c | 38 +++++++++++++++++---------------------\n pack.h | 15 ++++++---------\n 6 files changed, 42 insertions(+), 59 deletions(-)\n\ndiff --git a/builtin/fast-import.c b/builtin/fast-import.c\nindex 28d3193c380..7cd2a3b8203 100644\n--- a/builtin/fast-import.c\n+++ b/builtin/fast-import.c\n@@ -787,7 +787,7 @@ static const char *create_index(void)\n \t\tdie(\"internal consistency error creating the index\");\n \n \ttmpfile = write_idx_file(NULL, idx, object_count, &pack_idx_opts,\n-\t\t\t\t pack_data->hash);\n+\t\t\t\t pack_data->hash, 0);\n \tfree(idx);\n \treturn tmpfile;\n }\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 3e385b48002..6691c5836e4 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -1589,12 +1589,9 @@ static int git_index_pack_config(const char *k, const char *v, void *cb)\n \t\t}\n \t\treturn 0;\n \t}\n-\tif (!strcmp(k, \"pack.writereverseindex\")) {\n-\t\tif (git_config_bool(k, v))\n-\t\t\topts->flags |= WRITE_REV;\n-\t\telse\n-\t\t\topts->flags &= ~WRITE_REV;\n-\t}\n+\tif (!strcmp(k, \"pack.writereverseindex\"))\n+\t\topts->write_rev = git_config_bool(k, v);\n+\n \treturn git_default_config(k, v, cb);\n }\n \n@@ -1713,7 +1710,7 @@ static void show_pack_info(int stat_only)\n \n int cmd_index_pack(int argc, const char **argv, const char *prefix)\n {\n-\tint i, fix_thin_pack = 0, verify = 0, stat_only = 0, rev_index;\n+\tint i, fix_thin_pack = 0, verify = 0, stat_only = 0;\n \tconst char *curr_index;\n \tconst char *curr_rev_index = NULL;\n \tconst char *index_name = NULL, *pack_name = NULL, *rev_index_name = NULL;\n@@ -1747,10 +1744,8 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \tif (prefix && chdir(prefix))\n \t\tdie(_(\"Cannot come back to cwd\"));\n \n-\tif (git_env_bool(GIT_TEST_WRITE_REV_INDEX, 0))\n-\t\trev_index = 1;\n-\telse\n-\t\trev_index = !!(opts.flags & (WRITE_REV_VERIFY | WRITE_REV));\n+\topts.write_rev = git_env_bool(GIT_TEST_WRITE_REV_INDEX,\n+\t\t\t\t opts.write_rev);\n \n \tfor (i = 1; i < argc; i++) {\n \t\tconst char *arg = argv[i];\n@@ -1835,9 +1830,9 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \t\t\t\t\tdie(_(\"unknown hash algorithm '%s'\"), arg);\n \t\t\t\trepo_set_hash_algo(the_repository, hash_algo);\n \t\t\t} else if (!strcmp(arg, \"--rev-index\")) {\n-\t\t\t\trev_index = 1;\n+\t\t\t\topts.write_rev = 1;\n \t\t\t} else if (!strcmp(arg, \"--no-rev-index\")) {\n-\t\t\t\trev_index = 0;\n+\t\t\t\topts.write_rev = 0;\n \t\t\t} else\n \t\t\t\tusage(index_pack_usage);\n \t\t\tcontinue;\n@@ -1859,9 +1854,7 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \tif (!index_name && pack_name)\n \t\tindex_name = derive_filename(pack_name, \"pack\", \"idx\", &index_name_buf);\n \n-\topts.flags &= ~(WRITE_REV | WRITE_REV_VERIFY);\n-\tif (rev_index) {\n-\t\topts.flags |= verify ? WRITE_REV_VERIFY : WRITE_REV;\n+\tif (opts.write_rev) {\n \t\tif (index_name)\n \t\t\trev_index_name = derive_filename(index_name,\n \t\t\t\t\t\t\t \"idx\", \"rev\",\n@@ -1872,10 +1865,9 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \t\tif (!index_name)\n \t\t\tdie(_(\"--verify with no packfile name given\"));\n \t\tread_idx_option(&opts, index_name);\n-\t\topts.flags |= WRITE_IDX_VERIFY | WRITE_IDX_STRICT;\n \t}\n \tif (strict)\n-\t\topts.flags |= WRITE_IDX_STRICT;\n+\t\topts.write_idx_strict = 1;\n \n \tif (HAVE_THREADS && !nr_threads) {\n \t\tnr_threads = online_cpus();\n@@ -1919,11 +1911,12 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \tALLOC_ARRAY(idx_objects, nr_objects);\n \tfor (i = 0; i < nr_objects; i++)\n \t\tidx_objects[i] = &objects[i].idx;\n-\tcurr_index = write_idx_file(index_name, idx_objects, nr_objects, &opts, pack_hash);\n-\tif (rev_index)\n+\tcurr_index = write_idx_file(index_name, idx_objects, nr_objects, &opts,\n+\t\t\t\t pack_hash, verify);\n+\tif (opts.write_rev)\n \t\tcurr_rev_index = write_rev_file(rev_index_name, idx_objects,\n \t\t\t\t\t\tnr_objects, pack_hash,\n-\t\t\t\t\t\topts.flags);\n+\t\t\t\t\t\tverify);\n \tfree(idx_objects);\n \n \tif (!verify)\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 014dcd4bc98..6aeaad8560f 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3160,10 +3160,7 @@ static int git_pack_config(const char *k, const char *v, void *cb)\n \t\treturn 0;\n \t}\n \tif (!strcmp(k, \"pack.writereverseindex\")) {\n-\t\tif (git_config_bool(k, v))\n-\t\t\tpack_idx_opts.flags |= WRITE_REV;\n-\t\telse\n-\t\t\tpack_idx_opts.flags &= ~WRITE_REV;\n+\t\tpack_idx_opts.write_rev = git_config_bool(k, v);\n \t\treturn 0;\n \t}\n \tif (!strcmp(k, \"uploadpack.blobpackfileuri\")) {\n@@ -4007,8 +4004,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \n \treset_pack_idx_option(&pack_idx_opts);\n \tgit_config(git_pack_config, NULL);\n-\tif (git_env_bool(GIT_TEST_WRITE_REV_INDEX, 0))\n-\t\tpack_idx_opts.flags |= WRITE_REV;\n+\tpack_idx_opts.write_rev = git_env_bool(GIT_TEST_WRITE_REV_INDEX,\n+\t\t\t\t\t pack_idx_opts.write_rev);\n \n \tprogress = isatty(2);\n \targc = parse_options(argc, argv, prefix, pack_objects_options,\ndiff --git a/midx.c b/midx.c\nindex 3db0e47735f..f0bb56b2c64 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -905,7 +905,7 @@ static void write_midx_reverse_index(char *midx_name, unsigned char *midx_hash,\n \tstrbuf_addf(&buf, \"%s-%s.rev\", midx_name, hash_to_hex(midx_hash));\n \n \ttmp_file = write_rev_file_order(NULL, ctx->pack_order, ctx->entries_nr,\n-\t\t\t\t\tmidx_hash, WRITE_REV);\n+\t\t\t\t\tmidx_hash, 0);\n \n \tif (finalize_object_file(tmp_file, buf.buf))\n \t\tdie(_(\"cannot store reverse index file\"));\ndiff --git a/pack-write.c b/pack-write.c\nindex 51812cb1299..7973aa75579 100644\n--- a/pack-write.c\n+++ b/pack-write.c\n@@ -44,7 +44,7 @@ static int need_large_offset(off_t offset, const struct pack_idx_option *opts)\n */\n const char *write_idx_file(const char *index_name, struct pack_idx_entry **objects,\n \t\t\t int nr_objects, const struct pack_idx_option *opts,\n-\t\t\t const unsigned char *sha1)\n+\t\t\t const unsigned char *sha1, int verify)\n {\n \tstruct hashfile *f;\n \tstruct pack_idx_entry **sorted_by_sha, **list, **last;\n@@ -65,7 +65,7 @@ const char *write_idx_file(const char *index_name, struct pack_idx_entry **objec\n \telse\n \t\tsorted_by_sha = list = last = NULL;\n \n-\tif (opts->flags & WRITE_IDX_VERIFY) {\n+\tif (verify) {\n \t\tassert(index_name);\n \t\tf = hashfd_check(index_name);\n \t} else {\n@@ -117,7 +117,7 @@ const char *write_idx_file(const char *index_name, struct pack_idx_entry **objec\n \t\tif (index_version < 2)\n \t\t\thashwrite_be32(f, obj->offset);\n \t\thashwrite(f, obj->oid.hash, the_hash_algo->rawsz);\n-\t\tif ((opts->flags & WRITE_IDX_STRICT) &&\n+\t\tif (opts->write_idx_strict &&\n \t\t (i && oideq(&list[-2]->oid, &obj->oid)))\n \t\t\tdie(\"The same object %s appears twice in the pack\",\n \t\t\t oid_to_hex(&obj->oid));\n@@ -159,9 +159,10 @@ const char *write_idx_file(const char *index_name, struct pack_idx_entry **objec\n \t}\n \n \thashwrite(f, sha1, the_hash_algo->rawsz);\n+\n \tfinalize_hashfile(f, NULL, FSYNC_COMPONENT_PACK_METADATA,\n \t\t\t CSUM_HASH_IN_STREAM | CSUM_CLOSE |\n-\t\t\t ((opts->flags & WRITE_IDX_VERIFY) ? 0 : CSUM_FSYNC));\n+\t\t\t (verify ? 0 : CSUM_FSYNC));\n \treturn index_name;\n }\n \n@@ -216,22 +217,19 @@ const char *write_rev_file(const char *rev_name,\n \t\t\t struct pack_idx_entry **objects,\n \t\t\t uint32_t nr_objects,\n \t\t\t const unsigned char *hash,\n-\t\t\t unsigned flags)\n+\t\t\t int verify)\n {\n \tuint32_t *pack_order;\n \tuint32_t i;\n \tconst char *ret;\n \n-\tif (!(flags & WRITE_REV) && !(flags & WRITE_REV_VERIFY))\n-\t\treturn NULL;\n-\n \tALLOC_ARRAY(pack_order, nr_objects);\n \tfor (i = 0; i < nr_objects; i++)\n \t\tpack_order[i] = i;\n \tQSORT_S(pack_order, nr_objects, pack_order_cmp, objects);\n \n \tret = write_rev_file_order(rev_name, pack_order, nr_objects, hash,\n-\t\t\t\t flags);\n+\t\t\t\t verify);\n \n \tfree(pack_order);\n \n@@ -242,15 +240,12 @@ const char *write_rev_file_order(const char *rev_name,\n \t\t\t\t uint32_t *pack_order,\n \t\t\t\t uint32_t nr_objects,\n \t\t\t\t const unsigned char *hash,\n-\t\t\t\t unsigned flags)\n+\t\t\t\t int verify)\n {\n \tstruct hashfile *f;\n \tint fd;\n \n-\tif ((flags & WRITE_REV) && (flags & WRITE_REV_VERIFY))\n-\t\tdie(_(\"cannot both write and verify reverse index\"));\n-\n-\tif (flags & WRITE_REV) {\n+\tif (!verify) {\n \t\tif (!rev_name) {\n \t\t\tstruct strbuf tmp_file = STRBUF_INIT;\n \t\t\tfd = odb_mkstemp(&tmp_file, \"pack/tmp_rev_XXXXXX\");\n@@ -260,7 +255,7 @@ const char *write_rev_file_order(const char *rev_name,\n \t\t\tfd = xopen(rev_name, O_CREAT|O_EXCL|O_WRONLY, 0600);\n \t\t}\n \t\tf = hashfd(fd, rev_name);\n-\t} else if (flags & WRITE_REV_VERIFY) {\n+\t} else {\n \t\tstruct stat statbuf;\n \t\tif (stat(rev_name, &statbuf)) {\n \t\t\tif (errno == ENOENT) {\n@@ -270,8 +265,7 @@ const char *write_rev_file_order(const char *rev_name,\n \t\t\t\tdie_errno(_(\"could not stat: %s\"), rev_name);\n \t\t}\n \t\tf = hashfd_check(rev_name);\n-\t} else\n-\t\treturn NULL;\n+\t}\n \n \twrite_rev_header(f);\n \n@@ -283,7 +277,8 @@ const char *write_rev_file_order(const char *rev_name,\n \n \tfinalize_hashfile(f, NULL, FSYNC_COMPONENT_PACK_METADATA,\n \t\t\t CSUM_HASH_IN_STREAM | CSUM_CLOSE |\n-\t\t\t ((flags & WRITE_IDX_VERIFY) ? 0 : CSUM_FSYNC));\n+\t\t\t (verify ? 0 : CSUM_FSYNC));\n+\n \n \treturn rev_name;\n }\n@@ -494,12 +489,13 @@ void stage_tmp_packfiles(struct strbuf *name_buffer,\n \t\tdie_errno(\"unable to make temporary pack file readable\");\n \n \t*idx_tmp_name = (char *)write_idx_file(NULL, written_list, nr_written,\n-\t\t\t\t\t pack_idx_opts, hash);\n+\t\t\t\t\t pack_idx_opts, hash, 0);\n \tif (adjust_shared_perm(*idx_tmp_name))\n \t\tdie_errno(\"unable to make temporary index file readable\");\n \n-\trev_tmp_name = write_rev_file(NULL, written_list, nr_written, hash,\n-\t\t\t\t pack_idx_opts->flags);\n+\tif (pack_idx_opts->write_rev)\n+\t\trev_tmp_name = write_rev_file(NULL, written_list, nr_written, hash,\n+\t\t\t\t\t 0);\n \n \trename_tmp_packfile(name_buffer, pack_tmp_name, \"pack\");\n \tif (rev_tmp_name)\ndiff --git a/pack.h b/pack.h\nindex 802c047efe6..ccc4c129b0c 100644\n--- a/pack.h\n+++ b/pack.h\n@@ -38,12 +38,8 @@ struct pack_header {\n #define PACK_IDX_SIGNATURE 0xff744f63\t/* \"\\377tOc\" */\n \n struct pack_idx_option {\n-\tunsigned flags;\n-\t/* flag bits */\n-#define WRITE_IDX_VERIFY 01 /* verify only, do not write the idx file */\n-#define WRITE_IDX_STRICT 02\n-#define WRITE_REV 04\n-#define WRITE_REV_VERIFY 010\n+\tunsigned int write_idx_strict:1,\n+\t\t write_rev:1;\n \n \tuint32_t version;\n \tuint32_t off32_limit;\n@@ -84,7 +80,8 @@ typedef int (*verify_fn)(const struct object_id *, enum object_type, unsigned lo\n const char *write_idx_file(const char *index_name,\n \t\t\t struct pack_idx_entry **objects, int nr_objects,\n \t\t\t const struct pack_idx_option *opts,\n-\t\t\t const unsigned char *sha1);\n+\t\t\t const unsigned char *sha1,\n+\t\t\t int verify);\n int check_pack_crc(struct packed_git *p, struct pack_window **w_curs, off_t offset, off_t len, unsigned int nr);\n int verify_pack_index(struct packed_git *);\n int verify_pack(struct repository *, struct packed_git *, verify_fn fn, struct progress *, uint32_t);\n@@ -99,11 +96,11 @@ void write_promisor_file(const char *promisor_name, struct ref **sought, int nr_\n const char *write_rev_file(const char *rev_name,\n \t\t\t struct pack_idx_entry **objects,\n \t\t\t uint32_t nr_objects,\n-\t\t\t const unsigned char *hash, unsigned flags);\n+\t\t\t const unsigned char *hash, int verify);\n const char *write_rev_file_order(const char *rev_name, uint32_t *pack_order,\n \t\t\t\t uint32_t nr_objects,\n \t\t\t\t const unsigned char *hash,\n-\t\t\t\t unsigned flags);\n+\t\t\t\t int verify);\n \n /*\n * The \"hdr\" output buffer should be at least this big, which will handle sizes\n-- \n2.36.1.1124.g577fa9c2ebd\n\n"},{"id":"456621","messageId":"RFC-patch-15.15-16bd2270b4c-20220603T183608Z-avarab@gmail.com","threadId":"57952","inReplyTo":"RFC-cover-00.15-00000000000-20220603T183608Z-avarab@gmail.com","subject":"[RFC PATCH 15/15] config.mak.dev: add and use ASSERT_FOR_FANALYZER() macro","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-03T18:37:52Z","receivedAt":"2022-06-03T18:40:55Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add an ASSERT_FOR_FANALYZER() macro to quiet those -fanalyzer issues\nthat haven't been diagnosed yet to either change the code involved, or\nto add an assert() or BUG() to it to placate GCC v12's -fanalyzer\nmode.\n\nAs in the preceding commit we could also use -Wno-error=* here, which\nwas the initial implementation in config.mak.dev, i.e. something like:\n\n\t## -Wno-error=analyzer-null-dereference\n\t$(eval $(call fn_disable_analyzer, \\\n\t\t-Wno-error=analyzer-null-dereference, \\\n\t\tdir \\\n\t\tgpg-interface \\\n\t\tgraph \\\n\t\trange-diff \\\n\t\tutf8 \\\n\t))\n\t## -Wno-error=analyzer-null-dereference: pre-gcc12\n\tifeq ($(filter gcc12,$(COMPILER_FEATURES)),)\n\t$(eval $(call fn_disable_analyzer, \\\n\t\t-Wno-error=analyzer-null-dereference, \\\n\t\tmerge \\\n\t\tbuiltin/name-rev \\\n\t))\n\tendif\n\nBut any such approach will unfortunately quiet *all* in the relevant\nfiles, including any new ones. Instead we want to quiet specific\nissues involved with ASSERT_FOR_FANALYZER().\n\nA drawback of this overall approach is that this only works under\ne.g. CFLAGS=-O0, but not CFLAGS=-O3. The compiler takes the liberty to\nre-arrange our code to get around these assert()s, and other new\nissues will also crop up. All of this is expected (-fanalyzer is\nsubject to optimization passes, like most other options), but let's\nfocus on -O0 for now.\n\nCommentary on specific cases:\n\n * builtin/name-rev.c: Since 45a14f578e1 (Revert \"name-rev: release\n unused name strings\", 2022-04-22) GCC v12's -fanalyzer has complained\n about this code, per René's analysis in [1] it's incorrect to do\n so. So let's add an ASSERT_FOR_FANALYZER() to placate it.\n\n * graph.c: In 0f0f389f120 (graph: tidy up display of left-skewed\n merges, 2019-10-15) a previous \"assert\" was removed, and another\n unconditional deference of the return value of\n first_interesting_parent() was added without checking it. Since it\n can return NULL let's add ASSERT_FOR_FANALYZER()'s here to note for\n -fanalyzer that we won't be dereferencing these in practice.\n\n1. https://lore.kernel.org/git/dece627d-ccf8-2375-0c50-c59637e561d3@web.de/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/name-rev.c | 1 +\n config.mak.dev | 4 ++++\n dir.c | 1 +\n git-compat-util.h | 16 ++++++++++++++++\n gpg-interface.c | 2 ++\n graph.c | 2 ++\n line-log.c | 2 ++\n unpack-trees.c | 1 +\n utf8.c | 1 +\n 9 files changed, 30 insertions(+)\n\ndiff --git a/builtin/name-rev.c b/builtin/name-rev.c\nindex 02ea9d16330..419159961b9 100644\n--- a/builtin/name-rev.c\n+++ b/builtin/name-rev.c\n@@ -223,6 +223,7 @@ static void name_rev(struct commit *start_commit,\n \t\t\tif (commit_is_before_cutoff(parent))\n \t\t\t\tcontinue;\n \n+\t\t\tASSERT_FOR_FANALYZER(name);\n \t\t\tif (parent_number > 1) {\n \t\t\t\tgeneration = 0;\n \t\t\t\tdistance = name->distance + MERGE_TRAVERSAL_WEIGHT;\ndiff --git a/config.mak.dev b/config.mak.dev\nindex d6f5be92297..1d47fc04163 100644\n--- a/config.mak.dev\n+++ b/config.mak.dev\n@@ -83,6 +83,10 @@ endif\n \n DEVELOPER_CFLAGS += -fanalyzer\n \n+ifeq ($(filter no-suppress-analyzer,$(DEVOPTS)),)\n+DEVELOPER_CFLAGS += -DSUPPRESS_FSANITIZER\n+endif\n+\n ## -fanalyzer exists exists as of gcc10, but versions older than gcc12\n ## have a lot of false positives.\n ifeq ($(filter gcc12,$(COMPILER_FEATURES)),)\ndiff --git a/dir.c b/dir.c\nindex 3633c0852ee..c80b584b4a7 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -3781,6 +3781,7 @@ static void invalidate_one_directory(struct untracked_cache *uc,\n \t\t\t\t struct untracked_cache_dir *ucd)\n {\n \tuc->dir_invalidated++;\n+\tASSERT_FOR_FANALYZER(ucd);\n \tucd->valid = 0;\n \tucd->untracked_nr = 0;\n }\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 96293b6c43a..a553884c048 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -984,6 +984,21 @@ static inline unsigned long cast_size_t_to_ulong(size_t a)\n \treturn (unsigned long)a;\n }\n \n+/**\n+ * Transitory helper macro to quiet currently known GCC -fsanitizer\n+ * issues.\n+ *\n+ * We lie to it and say that we're confident that the given \"expr\" to\n+ * ASSERT_FOR_FANALYZER() cannot be NULL (or 0). Those\n+ * grandfathered-in issues should be fixed, but at least we're\n+ * stemming the tide.\n+ */\n+#ifdef SUPPRESS_FSANITIZER\n+#define ASSERT_FOR_FANALYZER(expr) assert((expr))\n+#else\n+#define ASSERT_FOR_FANALYZER(expr) do {} while (0)\n+#endif\n+\n #ifdef HAVE_ALLOCA_H\n # include \n # define xalloca(size) (alloca(size))\n@@ -1037,6 +1052,7 @@ int xstrncmpz(const char *s, const char *t, size_t len);\n \tBUILD_ASSERT_OR_ZERO(sizeof(*(dst)) == sizeof(*(src))))\n static inline void copy_array(void *dst, const void *src, size_t n, size_t size)\n {\n+\tASSERT_FOR_FANALYZER(dst);\n \tif (n)\n \t\tmemcpy(dst, src, st_mult(size, n));\n }\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex 280f1fa1a58..9cba3b86e45 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -242,6 +242,7 @@ static void parse_gpg_output(struct signature_check *sigc)\n \t\t\t\t\tnext = strchrnul(line, ' ');\n \t\t\t\t\treplace_cstring(&sigc->key, line, next);\n \t\t\t\t\t/* Do we have signer information? */\n+\t\t\t\t\tASSERT_FOR_FANALYZER(next);\n \t\t\t\t\tif (*next && (sigcheck_gpg_status[i].flags & GPG_STATUS_UID)) {\n \t\t\t\t\t\tline = next + 1;\n \t\t\t\t\t\tnext = strchrnul(line, '\\n');\n@@ -283,6 +284,7 @@ static void parse_gpg_output(struct signature_check *sigc)\n \t\t\t\t\t */\n \t\t\t\t\tlimit = strchrnul(line, '\\n');\n \t\t\t\t\tfor (j = 9; j > 0; j--) {\n+\t\t\t\t\t\tASSERT_FOR_FANALYZER(next);\n \t\t\t\t\t\tif (!*next || limit <= next)\n \t\t\t\t\t\t\tbreak;\n \t\t\t\t\t\tline = next + 1;\ndiff --git a/graph.c b/graph.c\nindex 568b6e7cd41..39f7e95d4ab 100644\n--- a/graph.c\n+++ b/graph.c\n@@ -1105,6 +1105,7 @@ static void graph_output_post_merge_line(struct git_graph *graph, struct graph_l\n \t\t\tseen_this = 1;\n \n \t\t\tfor (j = 0; j < graph->num_parents; j++) {\n+\t\t\t\tASSERT_FOR_FANALYZER(parents);\n \t\t\t\tpar_column = graph_find_new_column_by_commit(graph, parents->item);\n \t\t\t\tassert(par_column >= 0);\n \n@@ -1138,6 +1139,7 @@ static void graph_output_post_merge_line(struct git_graph *graph, struct graph_l\n \t\t\t}\n \t\t}\n \n+\t\tASSERT_FOR_FANALYZER(first_parent);\n \t\tif (col_commit == first_parent->item)\n \t\t\tparent_col = col;\n \t}\ndiff --git a/line-log.c b/line-log.c\nindex 51d93310a4d..1295f46deaf 100644\n--- a/line-log.c\n+++ b/line-log.c\n@@ -154,6 +154,7 @@ static void range_set_union(struct range_set *out,\n \twhile (i < a->nr || j < b->nr) {\n \t\tstruct range *new_range;\n \t\tif (i < a->nr && j < b->nr) {\n+\t\t\tASSERT_FOR_FANALYZER(rb);\n \t\t\tif (ra[i].start < rb[j].start)\n \t\t\t\tnew_range = &ra[i++];\n \t\t\telse if (ra[i].start > rb[j].start)\n@@ -166,6 +167,7 @@ static void range_set_union(struct range_set *out,\n \t\t\tnew_range = &ra[i++];\n \t\telse /* a exhausted */\n \t\t\tnew_range = &rb[j++];\n+\t\tASSERT_FOR_FANALYZER(new_range);\n \t\tif (new_range->start == new_range->end)\n \t\t\t; /* empty range */\n \t\telse if (!out->nr || out->ranges[out->nr-1].end < new_range->start) {\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex a1d0ff3a4d3..b6e10b05a00 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -2195,6 +2195,7 @@ static int verify_clean_subdirectory(const struct cache_entry *ce,\n \tchar *pathbuf;\n \tint cnt = 0;\n \n+\tASSERT_FOR_FANALYZER(ce);\n \tif (S_ISGITLINK(ce->ce_mode)) {\n \t\tstruct object_id oid;\n \t\tint sub_head = resolve_gitlink_ref(ce->name, \"HEAD\", &oid);\ndiff --git a/utf8.c b/utf8.c\nindex de4ce5c0e68..3ffc0a97f3b 100644\n--- a/utf8.c\n+++ b/utf8.c\n@@ -130,6 +130,7 @@ static ucs_char_t pick_one_utf8_char(const char **start, size_t *remainder_p)\n \t */\n \tremainder = (remainder_p ? *remainder_p : 999);\n \n+\tASSERT_FOR_FANALYZER(remainder < 1 || s);\n \tif (remainder < 1) {\n \t\tgoto invalid;\n \t} else if (*s < 0x80) {\n-- \n2.36.1.1124.g577fa9c2ebd\n\n"},{"id":"456634","messageId":"0c296c0e-bbd4-57a6-1cab-3b98c3bc8927@web.de","threadId":"57952","inReplyTo":"RFC-patch-01.15-b3a678d934a-20220603T183608Z-avarab@gmail.com","subject":"Re: [RFC PATCH 01/15] remote.c: don't dereference NULL in freeing loop","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-06-03T21:07:52Z","receivedAt":"2022-06-03T21:08:07Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 03.06.22 um 20:37 schrieb Ævar Arnfjörð Bjarmason:\n> Fix a bug in fd3cb0501e1 (remote: move static variables into\n> per-repository struct, 2021-11-17) where we'd free(remote->pushurl[i])\n> after having NULL'd out remote->pushurl. itself.\n>\n> While we're at it let's get rid of the redundant braces per the\n> CodingGuidelines, which also serves to show in the diff context that\n> we were doing a FREE_AND_NULL(remote->pushurl) afterwards too, let's\n> keep that one.\n\nThe extended context is helping, but the brace removal makes this change\nharder to read.\n\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> remote.c | 8 ++------\n> 1 file changed, 2 insertions(+), 6 deletions(-)\n>\n> diff --git a/remote.c b/remote.c\n> index 930fdc9c2f6..61240562df1 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -144,14 +144,10 @@ static void remote_clear(struct remote *remote)\n> \tfree((char *)remote->name);\n> \tfree((char *)remote->foreign_vcs);\n>\n> -\tfor (i = 0; i < remote->url_nr; i++) {\n> +\tfor (i = 0; i < remote->url_nr; i++)\n> \t\tfree((char *)remote->url[i]);\n> -\t}\n> -\tFREE_AND_NULL(remote->pushurl);\n\nSo you remove the redundant release of the pushurl array, but the url\narray, which should be freed here after its elements are released, still\nleaks. The duplicate FREE_AND_NULL perhaps resulted from a copy&paste\nerror.\n\n> -\n> -\tfor (i = 0; i < remote->pushurl_nr; i++) {\n> +\tfor (i = 0; i < remote->pushurl_nr; i++)\n> \t\tfree((char *)remote->pushurl[i]);\n> -\t}\n> \tFREE_AND_NULL(remote->pushurl);\n\nWhy set pushurl to NULL after release? This results in an invalid state\nunless pushurl_nr und pushurl_alloc are reset to zero. Same goes for\nthe url array above -- either a simple free(3) call suffices or url_nr\nand url_alloc need to be cleared as well.\n\n> \tfree((char *)remote->receivepack);\n> \tfree((char *)remote->uploadpack);\n"},{"id":"456637","messageId":"20bc49fd-4142-ac63-0fcc-30a5c68fa82b@web.de","threadId":"57952","inReplyTo":"RFC-patch-02.15-4a055969ea5-20220603T183608Z-avarab@gmail.com","subject":"Re: [RFC PATCH 02/15] pull.c: don't feed NULL to strcmp() on get_rebase_fork_point() path","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-06-03T21:27:37Z","receivedAt":"2022-06-03T21:28:08Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 03.06.22 um 20:37 schrieb Ævar Arnfjörð Bjarmason:\n> Fix an issue with feeding NULL to strcmp() noted by GCC's -fanalyzer,\n> this fixes a bug in 1678b81ecce (pull: teach git pull about --rebase,\n> 2015-06-18).\n>\n> In cmd_pull() we could go through the function without initializing\n> the \"repo\" argument (the -fanalyzer output shows how exactly), we'd\n> then call get_rebase_fork_point (), which would in turn call\n> get_tracking_branch() with that \"NULL\" repo argument.\n>\n> Let's avoid this potential issue by returning NULL in this case, which\n> will have get_rebase_fork_point() return -1 in turn.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> builtin/pull.c | 14 ++++++++------\n> 1 file changed, 8 insertions(+), 6 deletions(-)\n>\n> diff --git a/builtin/pull.c b/builtin/pull.c\n> index 01155ba67b2..ed8df004028 100644\n> --- a/builtin/pull.c\n> +++ b/builtin/pull.c\n> @@ -756,14 +756,16 @@ static const char *get_tracking_branch(const char *remote, const char *refspec)\n> \t\tstarts_with(spec_src, \"remotes/\"))\n> \t\tspec_src = \"\";\n>\n> -\tif (*spec_src) {\n> -\t\tif (!strcmp(remote, \".\"))\n> -\t\t\tmerge_branch = mkpath(\"refs/heads/%s\", spec_src);\n> -\t\telse\n> -\t\t\tmerge_branch = mkpath(\"refs/remotes/%s/%s\", remote, spec_src);\n> -\t} else\n> +\tif ((*spec_src && !remote) || !*spec_src) {\n\nThis is equivalent to:\n\n+\tif (!*spec_src || !remote) {\n\nIt's easier to read without the redundant term.\n\nThe patch would be even easier to understand as a one-liner that just adds\nthe missing check, i.e.:\n\n-\tif (*spec_src) {\n+\tif (*spec_src && remote) {\n\n> \t\tmerge_branch = NULL;\n> +\t\tgoto cleanup;\n> +\t}\n>\n> +\tif (!strcmp(remote, \".\"))\n> +\t\tmerge_branch = mkpath(\"refs/heads/%s\", spec_src);\n> +\telse\n> +\t\tmerge_branch = mkpath(\"refs/remotes/%s/%s\", remote, spec_src);\n> +cleanup:\n> \trefspec_item_clear(&spec);\n> \treturn merge_branch;\n> }\n"},{"id":"456638","messageId":"xmqqilphcu8p.fsf@gitster.g","threadId":"57952","inReplyTo":"0c296c0e-bbd4-57a6-1cab-3b98c3bc8927@web.de","subject":"Re: [RFC PATCH 01/15] remote.c: don't dereference NULL in freeing loop","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-03T21:28:06Z","receivedAt":"2022-06-03T21:28:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n>> -\tfor (i = 0; i < remote->pushurl_nr; i++) {\n>> +\tfor (i = 0; i < remote->pushurl_nr; i++)\n>> \t\tfree((char *)remote->pushurl[i]);\n>> -\t}\n>> \tFREE_AND_NULL(remote->pushurl);\n>\n> Why set pushurl to NULL after release? This results in an invalid state\n> unless pushurl_nr und pushurl_alloc are reset to zero. Same goes for\n> the url array above -- either a simple free(3) call suffices or url_nr\n> and url_alloc need to be cleared as well.\n\nWe probably should give a huge warning next to FREE_AND_NULL() about\nthis. It also is an effective way to hide an existing bug under the\nrug. diff_options.pathspec might be freed prematurely which may be\nnoticed by a use-after-free if left to use free(), but FREE_AND_NULL()\nwill mislead the use-after-free caller into thinkig that \"ah there is\nno pathspec to be used\" and produce nonsense result without crashing.\n\nThanks.\n"},{"id":"456641","messageId":"693ed3c3-535e-9eae-9fd7-ca612ebf6943@web.de","threadId":"57952","inReplyTo":"RFC-patch-03.15-0b570d112fc-20220603T183608Z-avarab@gmail.com","subject":"Re: [RFC PATCH 03/15] reftable: don't memset() a NULL from failed malloc()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-06-03T22:22:33Z","receivedAt":"2022-06-03T22:23:06Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 03.06.22 um 20:37 schrieb Ævar Arnfjörð Bjarmason:\n> Return NULL instead of possibly feeding a NULL to memset() in\n> reftable_calloc(). This issue was noted by GCC 12's -fanalyzer:\n>\n> \treftable/publicbasics.c: In function ‘reftable_calloc’:\n> \treftable/publicbasics.c:43:9: error: use of possibly-NULL ‘p’ where non-null expected [CWE-690] [-Werror=analyzer-possible-null-argument]\n> \t 43 | memset(p, 0, sz);\n> \t | ^~~~~~~~~~~~~~~~\n> \t[...]\n>\n> This bug has been with us ever since this code was added in\n> ef8a6c62687 (reftable: utility functions, 2021-10-07).\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> reftable/publicbasics.c | 2 ++\n> 1 file changed, 2 insertions(+)\n>\n> diff --git a/reftable/publicbasics.c b/reftable/publicbasics.c\n> index 0ad7d5c0ff2..a18167f5ab7 100644\n> --- a/reftable/publicbasics.c\n> +++ b/reftable/publicbasics.c\n> @@ -40,6 +40,8 @@ void reftable_free(void *p)\n> void *reftable_calloc(size_t sz)\n> {\n> \tvoid *p = reftable_malloc(sz);\n> +\tif (!p)\n> +\t\treturn NULL;\n> \tmemset(p, 0, sz);\n> \treturn p;\n> }\n\nWe discussed this before, in\nhttps://lore.kernel.org/git/RFC-patch-2.2-364d1194a95-20220415T101740Z-avarab@gmail.com/\n\nIf this code was actually used by Git and still not handling allocation\nfailures then I'd propose something like the below instead.\n\nNext I'd probably try to convert reftable_calloc() calls to a variant\nthat takes size and count separately -- like calloc(3) does -- to avoid\nunchecked multiplication.\n\n--- >8 ---\nSubject: [PATCH] reftable: remove reftable_set_alloc()\n\nUse our native allocation functions for the reftable code to provide\nconsistent handling of allocation failures. Remove the ability to\nchange the allocator, as we don't need it anymore after this change.\n\nSigned-off-by: René Scharfe \n---\n reftable/basics.h | 7 -------\n reftable/publicbasics.c | 43 --------------------------------------\n reftable/reftable-malloc.h | 18 ----------------\n reftable/system.h | 5 +++++\n 4 files changed, 5 insertions(+), 68 deletions(-)\n delete mode 100644 reftable/reftable-malloc.h\n\ndiff --git a/reftable/basics.h b/reftable/basics.h\nindex 096b36862b..3386ff6484 100644\n--- a/reftable/basics.h\n+++ b/reftable/basics.h\n@@ -46,13 +46,6 @@ int names_equal(char **a, char **b);\n /* returns the array size of a NULL-terminated array of strings. */\n int names_length(char **names);\n\n-/* Allocation routines; they invoke the functions set through\n- * reftable_set_alloc() */\n-void *reftable_malloc(size_t sz);\n-void *reftable_realloc(void *p, size_t sz);\n-void reftable_free(void *p);\n-void *reftable_calloc(size_t sz);\n-\n /* Find the longest shared prefix size of `a` and `b` */\n struct strbuf;\n int common_prefix_size(struct strbuf *a, struct strbuf *b);\ndiff --git a/reftable/publicbasics.c b/reftable/publicbasics.c\nindex 0ad7d5c0ff..4dd4204ce6 100644\n--- a/reftable/publicbasics.c\n+++ b/reftable/publicbasics.c\n@@ -6,52 +6,9 @@ license that can be found in the LICENSE file or at\n https://developers.google.com/open-source/licenses/bsd\n */\n\n-#include \"reftable-malloc.h\"\n-\n #include \"basics.h\"\n #include \"system.h\"\n\n-static void *(*reftable_malloc_ptr)(size_t sz);\n-static void *(*reftable_realloc_ptr)(void *, size_t);\n-static void (*reftable_free_ptr)(void *);\n-\n-void *reftable_malloc(size_t sz)\n-{\n-\tif (reftable_malloc_ptr)\n-\t\treturn (*reftable_malloc_ptr)(sz);\n-\treturn malloc(sz);\n-}\n-\n-void *reftable_realloc(void *p, size_t sz)\n-{\n-\tif (reftable_realloc_ptr)\n-\t\treturn (*reftable_realloc_ptr)(p, sz);\n-\treturn realloc(p, sz);\n-}\n-\n-void reftable_free(void *p)\n-{\n-\tif (reftable_free_ptr)\n-\t\treftable_free_ptr(p);\n-\telse\n-\t\tfree(p);\n-}\n-\n-void *reftable_calloc(size_t sz)\n-{\n-\tvoid *p = reftable_malloc(sz);\n-\tmemset(p, 0, sz);\n-\treturn p;\n-}\n-\n-void reftable_set_alloc(void *(*malloc)(size_t),\n-\t\t\tvoid *(*realloc)(void *, size_t), void (*free)(void *))\n-{\n-\treftable_malloc_ptr = malloc;\n-\treftable_realloc_ptr = realloc;\n-\treftable_free_ptr = free;\n-}\n-\n int hash_size(uint32_t id)\n {\n \tswitch (id) {\ndiff --git a/reftable/reftable-malloc.h b/reftable/reftable-malloc.h\ndeleted file mode 100644\nindex 5f2185f1f3..0000000000\n--- a/reftable/reftable-malloc.h\n+++ /dev/null\n@@ -1,18 +0,0 @@\n-/*\n-Copyright 2020 Google LLC\n-\n-Use of this source code is governed by a BSD-style\n-license that can be found in the LICENSE file or at\n-https://developers.google.com/open-source/licenses/bsd\n-*/\n-\n-#ifndef REFTABLE_H\n-#define REFTABLE_H\n-\n-#include \n-\n-/* Overrides the functions to use for memory management. */\n-void reftable_set_alloc(void *(*malloc)(size_t),\n-\t\t\tvoid *(*realloc)(void *, size_t), void (*free)(void *));\n-\n-#endif\ndiff --git a/reftable/system.h b/reftable/system.h\nindex 18f9207dfe..d7c7c20115 100644\n--- a/reftable/system.h\n+++ b/reftable/system.h\n@@ -16,6 +16,11 @@ license that can be found in the LICENSE file or at\n #include \"hash.h\" /* hash ID, sizes.*/\n #include \"dir.h\" /* remove_dir_recursively, for tests.*/\n\n+#define reftable_malloc(x) xmalloc(x)\n+#define reftable_realloc(x, y) xrealloc((x), (y))\n+#define reftable_free(x) free(x)\n+#define reftable_calloc(x) xcalloc(1, (x))\n+\n int hash_size(uint32_t id);\n\n #endif\n--\n2.35.3\n"},{"id":"456642","messageId":"kl6l35gle5u9.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"57952","inReplyTo":"0c296c0e-bbd4-57a6-1cab-3b98c3bc8927@web.de","subject":"Re: [RFC PATCH 01/15] remote.c: don't dereference NULL in freeing loop","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-06-03T22:32:14Z","receivedAt":"2022-06-03T22:32:27Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"René Scharfe writes:\n\n> Am 03.06.22 um 20:37 schrieb Ævar Arnfjörð Bjarmason:\n>>\n>> Signed-off-by: Ævar Arnfjörð Bjarmason \n>> ---\n>> remote.c | 8 ++------\n>> 1 file changed, 2 insertions(+), 6 deletions(-)\n>>\n>> diff --git a/remote.c b/remote.c\n>> index 930fdc9c2f6..61240562df1 100644\n>> --- a/remote.c\n>> +++ b/remote.c\n>> @@ -144,14 +144,10 @@ static void remote_clear(struct remote *remote)\n>> \tfree((char *)remote->name);\n>> \tfree((char *)remote->foreign_vcs);\n>>\n>> -\tfor (i = 0; i < remote->url_nr; i++) {\n>> +\tfor (i = 0; i < remote->url_nr; i++)\n>> \t\tfree((char *)remote->url[i]);\n>> -\t}\n>> -\tFREE_AND_NULL(remote->pushurl);\n>\n> So you remove the redundant release of the pushurl array, but the url\n> array, which should be freed here after its elements are released, still\n> leaks. The duplicate FREE_AND_NULL perhaps resulted from a copy&paste\n> error.\n\nThis seems like the most likely explanation (even I don't recall how I\nended up introducing this bug..). I probably meant to change pushurl ->\nurl, but forgot somehow.\n\n>> -\n>> -\tfor (i = 0; i < remote->pushurl_nr; i++) {\n>> +\tfor (i = 0; i < remote->pushurl_nr; i++)\n>> \t\tfree((char *)remote->pushurl[i]);\n>> -\t}\n>> \tFREE_AND_NULL(remote->pushurl);\n>\n> Why set pushurl to NULL after release? This results in an invalid state\n> unless pushurl_nr und pushurl_alloc are reset to zero. Same goes for\n> the url array above -- either a simple free(3) call suffices or url_nr\n> and url_alloc need to be cleared as well.\n\nTrue, thanks for catching that. We'd probably never notice it because we\nnever reuse the clear-ed `struct remote` (since we only ever call this\nwhen we clean up `struct repository`), but it's better to do the 'right\nthing'.\n\nI didn't consider that when I chose to use FREE_AND_NULL() instead of\nfree(), and there wasn't any particular reason why I chose one over the\nother.\n"},{"id":"456643","messageId":"6fec4a06-ac02-3de0-4517-6aa9314653a6@web.de","threadId":"57952","inReplyTo":"RFC-patch-04.15-3a287c19d7e-20220603T183608Z-avarab@gmail.com","subject":"Re: [RFC PATCH 04/15] diff-lib.c: don't dereference NULL in oneway_diff()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-06-03T22:48:24Z","receivedAt":"2022-06-03T22:49:03Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 03.06.22 um 20:37 schrieb Ævar Arnfjörð Bjarmason:\n> Fix a control flow issue dating back to d1f2d7e8ca6 (Make\n> run_diff_index() use unpack_trees(), not read_tree(), 2008-01-19)\n> where we'd assume \"tree\" must be non-NULL if idx was NULL. As\n> -fanalyzer shows we'd end up dereferencing \"tree\" in that case in\n> ce_path_match():\n>\n> dir.h:541:41: warning: dereference of NULL ‘ce’ [CWE-476] [-Wanalyzer-null-dereference]\n> 541 | S_ISDIR(ce->ce_mode) || S_ISGITLINK(ce->ce_mode));\n> | ^\n> ‘oneway_diff’: events 1-2\n> |\n> |diff-lib.c:493:12:\n> | 493 | static int oneway_diff(const struct cache_entry * const *src,\n> | | ^~~~~~~~~~~\n> | | |\n> | | (1) entry to ‘oneway_diff’\n> |......\n> | 506 | if (tree == o->df_conflict_entry)\n> | | ~\n> | | |\n> | | (2) following ‘true’ branch...\n> |\n> ‘oneway_diff’: event 3\n> |\n> | 507 | tree = NULL;\n> | | ^\n> | | |\n> | | (3) ...to here\n> |\n> ‘oneway_diff’: events 4-8\n> |\n> | 507 | tree = NULL;\n> | | ^\n> | | |\n> | | (4) ‘tree’ is NULL\n> | 508 |\n> | 509 | if (ce_path_match(revs->diffopt.repo->index,\n> | | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n> | | |\n> | | (5) following ‘false’ branch (when ‘idx’ is NULL)...\n> | | (6) ...to here\n> | | (7) ‘tree’ is NULL\n> | | (8) calling ‘ce_path_match’ from ‘oneway_diff’\n> | 510 | idx ? idx : tree,\n> | | ~~~~~~~~~~~~~~~~~\n> | 511 | &revs->prune_data, NULL)) {\n> | | ~~~~~~~~~~~~~~~~~~~~~~~~\n> |\n> +--> ‘ce_path_match’: event 9\n> |\n> |dir.h:535:19:\n> | 535 | static inline int ce_path_match(struct index_state *istate,\n> | | ^~~~~~~~~~~~~\n> | | |\n> | | (9) entry to ‘ce_path_match’\n> |\n> ‘ce_path_match’: event 10\n> |\n> | 541 | S_ISDIR(ce->ce_mode) || S_ISGITLINK(ce->ce_mode));\n> | | ^\n> | | |\n> | | (10) dereference of NULL ‘ce\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> diff-lib.c | 3 +++\n> 1 file changed, 3 insertions(+)\n>\n> diff --git a/diff-lib.c b/diff-lib.c\n> index ca085a03efc..8373ad7e3ea 100644\n> --- a/diff-lib.c\n> +++ b/diff-lib.c\n> @@ -506,6 +506,9 @@ static int oneway_diff(const struct cache_entry * const *src,\n> \tif (tree == o->df_conflict_entry)\n> \t\ttree = NULL;\n\nSo here we have a D/F conflict in a oneway diff, i.e. a single tree.\nThat means if we discard the thing from the tree, then we still have\nthe conflicting thing from the index. Meaning idx and tree cannot both\nbe NULL in that D/F conflict scenario. Right?\n\n>\n> +\tif (!idx && !tree)\n> +\t\treturn 0;\n\nThat calms down the confused compiler, but would it perhaps be better to\nBUG out at this point? Or is there a valid state with both idx and tree\nbeing NULL?\n\n> +\n> \tif (ce_path_match(revs->diffopt.repo->index,\n> \t\t\t idx ? idx : tree,> \t\t\t &revs->prune_data, NULL)) {\n"},{"id":"456647","messageId":"5b3e0d7c-e507-2edc-0cbc-5dd54883a8b3@web.de","threadId":"57952","inReplyTo":"RFC-patch-05.15-46e0c307941-20220603T183608Z-avarab@gmail.com","subject":"Re: [RFC PATCH 05/15] refs/packed-backend.c: add a BUG() if iter is NULL","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-06-03T23:14:11Z","receivedAt":"2022-06-03T23:14:48Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 03.06.22 um 20:37 schrieb Ævar Arnfjörð Bjarmason:\n> Adjust code added in 2775d8724d7 (packed_ref_store: implement\n> reference transactions, 2017-09-08) to BUG() out in a case there GCC\n> v12's -fanalyzer flagged that the \"iter->oid\" seen in the context was\n> reachable where iter was NULL.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> refs/packed-backend.c | 2 ++\n> 1 file changed, 2 insertions(+)\n>\n> diff --git a/refs/packed-backend.c b/refs/packed-backend.c\n> index 97b68377673..65991bbcaf5 100644\n> --- a/refs/packed-backend.c\n> +++ b/refs/packed-backend.c\n> @@ -1226,6 +1226,8 @@ static int write_with_updates(struct packed_ref_store *refs,\n> \t\t\tstruct object_id peeled;\n> \t\t\tint peel_error = ref_iterator_peel(iter, &peeled);\n\nSome peel backends dereference iter, e.g. packed_ref_iterator_peel() from\nthe same file.\n\n>\n> +\t\t\tif (!iter)\n> +\t\t\t\tBUG(\"must have iter if cmp < 0\");\n\nSo if iter can be NULL at this point, then it must be checked before the\nref_iterator_peel() call above, no?\n\n*Can* it be NULL, though? The code in question is a bit complicated and\nit's late around here, but I cannot see how cmp < 0 and !iter can both\nbe true at this place in the code. An optimizing compiler will probably\noptimize out the added check and string. Mine does (clang 13); you can\ncheck if yours does the same using:\n\n $ make refs/packed-backend.o && grep \"must have\" refs/packed-backend.o\n\n> \t\t\tif (write_packed_entry(out, iter->refname,\n> \t\t\t\t\t iter->oid,\n> \t\t\t\t\t peel_error ? NULL : &peeled))\n"},{"id":"456651","messageId":"220604.86pmjpw8h0.gmgdl@evledraar.gmail.com","threadId":"57952","inReplyTo":"693ed3c3-535e-9eae-9fd7-ca612ebf6943@web.de","subject":"Re: [RFC PATCH 03/15] reftable: don't memset() a NULL from failed malloc()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-04T00:54:00Z","receivedAt":"2022-06-04T00:58:10Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Jun 04 2022, René Scharfe wrote:\n\n> Am 03.06.22 um 20:37 schrieb Ævar Arnfjörð Bjarmason:\n>> Return NULL instead of possibly feeding a NULL to memset() in\n>> reftable_calloc(). This issue was noted by GCC 12's -fanalyzer:\n>>\n>> \treftable/publicbasics.c: In function ‘reftable_calloc’:\n>> \treftable/publicbasics.c:43:9: error: use of possibly-NULL ‘p’ where non-null expected [CWE-690] [-Werror=analyzer-possible-null-argument]\n>> \t 43 | memset(p, 0, sz);\n>> \t | ^~~~~~~~~~~~~~~~\n>> \t[...]\n>>\n>> This bug has been with us ever since this code was added in\n>> ef8a6c62687 (reftable: utility functions, 2021-10-07).\n>>\n>> Signed-off-by: Ævar Arnfjörð Bjarmason \n>> ---\n>> reftable/publicbasics.c | 2 ++\n>> 1 file changed, 2 insertions(+)\n>>\n>> diff --git a/reftable/publicbasics.c b/reftable/publicbasics.c\n>> index 0ad7d5c0ff2..a18167f5ab7 100644\n>> --- a/reftable/publicbasics.c\n>> +++ b/reftable/publicbasics.c\n>> @@ -40,6 +40,8 @@ void reftable_free(void *p)\n>> void *reftable_calloc(size_t sz)\n>> {\n>> \tvoid *p = reftable_malloc(sz);\n>> +\tif (!p)\n>> +\t\treturn NULL;\n>> \tmemset(p, 0, sz);\n>> \treturn p;\n>> }\n>\n> We discussed this before, in\n> https://lore.kernel.org/git/RFC-patch-2.2-364d1194a95-20220415T101740Z-avarab@gmail.com/\n>\n> If this code was actually used by Git and still not handling allocation\n> failures then I'd propose something like the below instead.\n>\n> Next I'd probably try to convert reftable_calloc() calls to a variant\n> that takes size and count separately -- like calloc(3) does -- to avoid\n> unchecked multiplication.\n>\n> --- >8 ---\n> Subject: [PATCH] reftable: remove reftable_set_alloc()\n\nI think this is a much better direction than my more narrow fix, and\nwould be happy to see it queued up.\n\nTo your comment here & some others (e.g. FREE_AND_NULL()): I was really\ntrying to focus on narrowly addressing these -fanalyzer issues without\ndigressing into the larger topics \"what is this code *really* doing, and\ndoes it make sense?\". It was pretty unavoidable in 13/15 though.\n\nWhich isn't to say that I shouldn't fix some of it, e.g. your\ns/return/BUG()/ suggestion, but I think it's best to view these patches\nwith an eye towards us already having these issues, and in most cases\nmaking -fanalyzer happy is a small cost.\n\nAnd by doing so and getting a \"clean build\" we'll be able to turn it on\nin CI, and thus notice when we run into new -fanalyzer issues.\n"},{"id":"456674","messageId":"bb05e942-eccd-5f88-d3c7-de903464d866@web.de","threadId":"57952","inReplyTo":"RFC-patch-07.15-cf1a5f3ed0f-20220603T183608Z-avarab@gmail.com","subject":"Re: [RFC PATCH 07/15] strbuf.c: placate -fanalyzer in strbuf_grow()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-06-04T12:24:35Z","receivedAt":"2022-06-04T12:25:09Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 03.06.22 um 20:37 schrieb Ævar Arnfjörð Bjarmason:\n> Change the strbuf_grow() function so that GCC v12's -fanalyze doesn't\n> yell at us about sb->buf[0] dereferencing NULL, this also makes this\n> code easier to follow.\n>\n> This BUG() should be unreachable since the state of our \"sb->buf\" and\n> \"sb->alloc\" goes hand-in-hand, but -fanalyzer isn't smart enough to\n> know that, and adding the BUG() also makes it clearer to human readers\n> that that's what happens here.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> strbuf.c | 2 ++\n> 1 file changed, 2 insertions(+)\n>\n> diff --git a/strbuf.c b/strbuf.c\n> index dd9eb85527a..61c4630aeeb 100644\n> --- a/strbuf.c\n> +++ b/strbuf.c\n> @@ -97,6 +97,8 @@ void strbuf_grow(struct strbuf *sb, size_t extra)\n> \tif (new_buf)\n> \t\tsb->buf = NULL;\n> \tALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc);\n> +\tif (new_buf && !sb->buf)\n> +\t\tBUG(\"for a new buffer ALLOC_GROW() should always do work!\");\n\nnew_buf is !sb->alloc. ALLOC_GROW sets buf if sb->len + extra + 1 is\nbigger than sb->alloc, which is always true because the variables in the\nsum are unsigned, so we end up with at least 1 > 0. That's easy enough\nto see; I wonder why the compiler doesn't agree. Am I missing\nsomething?\n\nDoes setting the attribute returns_nonnull for xrealloc help?\nSpecifying it explicitly makes sense to me -- expecting the compiler to\ninfer it automatically across compilation units is probably a bit too\nmuch to ask for, or at least needlessly expensive.\n\n> \tif (new_buf)\n> \t\tsb->buf[0] = '\\0';\n> }\n"},{"id":"456675","messageId":"1de74bf7-0e75-8ae3-6ea7-62939b540061@web.de","threadId":"57952","inReplyTo":"220604.86pmjpw8h0.gmgdl@evledraar.gmail.com","subject":"Re: [RFC PATCH 03/15] reftable: don't memset() a NULL from failed malloc()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-06-04T12:24:30Z","receivedAt":"2022-06-04T12:25:11Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 04.06.22 um 02:54 schrieb Ævar Arnfjörð Bjarmason:\n>\n> To your comment here & some others (e.g. FREE_AND_NULL()): I was really\n> trying to focus on narrowly addressing these -fanalyzer issues without\n> digressing into the larger topics \"what is this code *really* doing, and\n> does it make sense?\". It was pretty unavoidable in 13/15 though.\n>\n> Which isn't to say that I shouldn't fix some of it, e.g. your\n> s/return/BUG()/ suggestion, but I think it's best to view these patches\n> with an eye towards us already having these issues, and in most cases\n> making -fanalyzer happy is a small cost.\n>\n> And by doing so and getting a \"clean build\" we'll be able to turn it on\n> in CI, and thus notice when we run into new -fanalyzer issues.\n\nFuture analyzer reports are likely of the same quality as the current\nones. If the goal is to shush them then we should just not use the\nanalyzer. If reports contain a helpful signal, e.g. pointing to a real\nbug or to overly complicated code, then we better address these issues.\n\nWe can think about automating the analyzer once we have a certain number\nof commits with improvements that would not have been made without it.\n\nRené\n"},{"id":"456676","messageId":"870925774.539833.1654346793623@apps.talktalk.co.uk","threadId":"57952","inReplyTo":"RFC-patch-07.15-cf1a5f3ed0f-20220603T183608Z-avarab@gmail.com","subject":"Re: [RFC PATCH 07/15] strbuf.c: placate -fanalyzer in strbuf_grow()","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2022-06-04T12:46:33Z","receivedAt":"2022-06-04T12:47:41Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Ævar\n\n[This is an old address that I only have webmail access to, please use phillip.wood@dunelm.org.uk when cc'ing me]\n\n> On 03 June 2022 at 19:37 Ævar Arnfjörð Bjarmason wrote:\n> \n> \n> Change the strbuf_grow() function so that GCC v12's -fanalyze doesn't\n> yell at us about sb->buf[0] dereferencing NULL, this also makes this\n> code easier to follow.\n> \n> This BUG() should be unreachable since the state of our \"sb->buf\" and\n> \"sb->alloc\" goes hand-in-hand, but -fanalyzer isn't smart enough to\n> know that, and adding the BUG() also makes it clearer to human readers\n> that that's what happens here.\n> \n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> strbuf.c | 2 ++\n> 1 file changed, 2 insertions(+)\n> \n> diff --git a/strbuf.c b/strbuf.c\n> index dd9eb85527a..61c4630aeeb 100644\n> --- a/strbuf.c\n> +++ b/strbuf.c\n> @@ -97,6 +97,8 @@ void strbuf_grow(struct strbuf *sb, size_t extra)\n> \tif (new_buf)\n> \t\tsb->buf = NULL;\n> \tALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc);\n> +\tif (new_buf && !sb->buf)\n> +\t\tBUG(\"for a new buffer ALLOC_GROW() should always do work!\");\n\nThis is a bit ugly, have you tried adding\n__attribute__((malloc (free), returns_nonnull))\nto xmalloc() and xrealloc() ?\n\nBest Wishes\n\nPhillip\n\n> \tif (new_buf)\n> \t\tsb->buf[0] = '\\0';\n> }\n> -- \n> 2.36.1.1124.g577fa9c2ebd\n>\n"},{"id":"456677","messageId":"2064938784.539963.1654347067250@apps.talktalk.co.uk","threadId":"57952","inReplyTo":"0c296c0e-bbd4-57a6-1cab-3b98c3bc8927@web.de","subject":"Re: [RFC PATCH 01/15] remote.c: don't dereference NULL in freeing loop","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2022-06-04T12:51:07Z","receivedAt":"2022-06-04T12:51:14Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"\n> On 03 June 2022 at 22:07 René Scharfe wrote:\n> \n> \n> Am 03.06.22 um 20:37 schrieb Ævar Arnfjörð Bjarmason:\n> > Fix a bug in fd3cb0501e1 (remote: move static variables into\n> > per-repository struct, 2021-11-17) where we'd free(remote->pushurl[i])\n> > after having NULL'd out remote->pushurl. itself.\n> >\n> > While we're at it let's get rid of the redundant braces per the\n> > CodingGuidelines, which also serves to show in the diff context that\n> > we were doing a FREE_AND_NULL(remote->pushurl) afterwards too, let's\n> > keep that one.\n> \n> The extended context is helping, but the brace removal makes this change\n> harder to read.\n\nIndeed, a small style fix in a larger change is one thing but here at least 80% of the changed lines are unrelated to the bugfix. I'm afraid my heart has started to sink when I see the phrase \"while we're at it\" in a commit message.\n\n> [...]\n> > \tFREE_AND_NULL(remote->pushurl);\n> \n> Why set pushurl to NULL after release? This results in an invalid state\n> unless pushurl_nr und pushurl_alloc are reset to zero. Same goes for\n> the url array above -- either a simple free(3) call suffices or url_nr\n> and url_alloc need to be cleared as well.\n\nThat's a good point\n\nBest Wishes\n\nPhillip\n"},{"id":"456678","messageId":"1848899554.540227.1654347881891@apps.talktalk.co.uk","threadId":"57952","inReplyTo":"RFC-patch-09.15-de0f7722608-20220603T183608Z-avarab@gmail.com","subject":"Re: [RFC PATCH 09/15] add-patch: assert parse_diff() expectations with BUG()","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2022-06-04T13:04:41Z","receivedAt":"2022-06-04T13:05:07Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Ævar\n\n> On 03 June 2022 at 19:37 Ævar Arnfjörð Bjarmason wrote:\n> \n> \n> Assert that this code added in [1], [2] and other related commits\n> expects that once we see a \"diff \" line we should have a non-NULL\n> \"file_diff\" and \"hunk\".\n> \n> In practice this would have always been the case, as we are parsing\n> our own \"diff\" output, but e.g. GCC v12's -fanalyzer doesn't know\n> that, and will alert us that in the \"else if\" and below in this\n> function we could be dereferencing NULL if we were processing anything\n> except our expected input.\n\nIf we're only doing this to keep -fanalyzer quiet then would it be better to use the macro you introduce at the end of this series instead?\n\nBest Wishes\n\nPhillip\n\n> 1. f6aa7ecc343 (built-in add -i: start implementing the `patch`\n> functionality in C, 2019-12-13)\n> 2. 80399aec5ab (built-in add -p: support multi-file diffs, 2019-12-13)\n> \n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> add-patch.c | 7 ++++++-\n> 1 file changed, 6 insertions(+), 1 deletion(-)\n> \n> diff --git a/add-patch.c b/add-patch.c\n> index 55d719f7845..087bf317b07 100644\n> --- a/add-patch.c\n> +++ b/add-patch.c\n> @@ -478,11 +478,16 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)\n> \twhile (p != pend) {\n> \t\tchar *eol = memchr(p, '\\n', pend - p);\n> \t\tconst char *deleted = NULL, *mode_change = NULL;\n> +\t\tconst char *const diff_l = \"diff \";\n> +\t\tint is_diff_line = starts_with(p, diff_l);\n> \n> \t\tif (!eol)\n> \t\t\teol = pend;\n> \n> -\t\tif (starts_with(p, \"diff \")) {\n> +\t\tif (!is_diff_line && (!file_diff || !hunk))\n> +\t\t\tBUG(\"expected '%s' line to follow a '%s' line\", p, diff_l);\n> +\n> +\t\tif (is_diff_line) {\n> \t\t\tcomplete_file(marker, hunk);\n> \t\t\tALLOC_GROW_BY(s->file_diff, s->file_diff_nr, 1,\n> \t\t\t\t file_diff_alloc);\n> -- \n> 2.36.1.1124.g577fa9c2ebd\n>\n"},{"id":"456679","messageId":"1340013130.540405.1654348338278@apps.talktalk.co.uk","threadId":"57952","inReplyTo":"RFC-patch-15.15-16bd2270b4c-20220603T183608Z-avarab@gmail.com","subject":"Re: [RFC PATCH 15/15] config.mak.dev: add and use ASSERT_FOR_FANALYZER() macro","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2022-06-04T13:12:18Z","receivedAt":"2022-06-04T13:12:24Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"\n> On 03 June 2022 at 19:37 Ævar Arnfjörð Bjarmason wrote:\n> \n> \n> Add an ASSERT_FOR_FANALYZER() macro to quiet those -fanalyzer issues\n\nIf the purpose of the macro is to silence -fanalyzer then it would perhaps be better to name it after that purpose rather than the way that it is implemented. SILENCE_FANALYZER_WARNING() or something like that.\n\n> that haven't been diagnosed yet to either change the code involved, or\n> to add an assert() or BUG() to it to placate GCC v12's -fanalyzer\n> mode.\n> \n> As in the preceding commit we could also use -Wno-error=* here, which\n> was the initial implementation in config.mak.dev, i.e. something like:\n> \n> \t## -Wno-error=analyzer-null-dereference\n> \t$(eval $(call fn_disable_analyzer, \\\n> \t\t-Wno-error=analyzer-null-dereference, \\\n> \t\tdir \\\n> \t\tgpg-interface \\\n> \t\tgraph \\\n> \t\trange-diff \\\n> \t\tutf8 \\\n> \t))\n> \t## -Wno-error=analyzer-null-dereference: pre-gcc12\n> \tifeq ($(filter gcc12,$(COMPILER_FEATURES)),)\n> \t$(eval $(call fn_disable_analyzer, \\\n> \t\t-Wno-error=analyzer-null-dereference, \\\n> \t\tmerge \\\n> \t\tbuiltin/name-rev \\\n> \t))\n> \tendif\n> \n> But any such approach will unfortunately quiet *all* in the relevant\n> files, including any new ones. Instead we want to quiet specific\n> issues involved with ASSERT_FOR_FANALYZER().\n\nI had a hard time understanding this, maybe\n\nBut any such approach will unfortunately miss any new warnings as it silences them for the whole file.\n\nBest Wishes\n\nPhillip\n\n> \n> A drawback of this overall approach is that this only works under\n> e.g. CFLAGS=-O0, but not CFLAGS=-O3. The compiler takes the liberty to\n> re-arrange our code to get around these assert()s, and other new\n> issues will also crop up. All of this is expected (-fanalyzer is\n> subject to optimization passes, like most other options), but let's\n> focus on -O0 for now.\n> \n> Commentary on specific cases:\n> \n> * builtin/name-rev.c: Since 45a14f578e1 (Revert \"name-rev: release\n> unused name strings\", 2022-04-22) GCC v12's -fanalyzer has complained\n> about this code, per René's analysis in [1] it's incorrect to do\n> so. So let's add an ASSERT_FOR_FANALYZER() to placate it.\n> \n> * graph.c: In 0f0f389f120 (graph: tidy up display of left-skewed\n> merges, 2019-10-15) a previous \"assert\" was removed, and another\n> unconditional deference of the return value of\n> first_interesting_parent() was added without checking it. Since it\n> can return NULL let's add ASSERT_FOR_FANALYZER()'s here to note for\n> -fanalyzer that we won't be dereferencing these in practice.\n> \n> 1. https://lore.kernel.org/git/dece627d-ccf8-2375-0c50-c59637e561d3@web.de/\n> \n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> builtin/name-rev.c | 1 +\n> config.mak.dev | 4 ++++\n> dir.c | 1 +\n> git-compat-util.h | 16 ++++++++++++++++\n> gpg-interface.c | 2 ++\n> graph.c | 2 ++\n> line-log.c | 2 ++\n> unpack-trees.c | 1 +\n> utf8.c | 1 +\n> 9 files changed, 30 insertions(+)\n> \n> diff --git a/builtin/name-rev.c b/builtin/name-rev.c\n> index 02ea9d16330..419159961b9 100644\n> --- a/builtin/name-rev.c\n> +++ b/builtin/name-rev.c\n> @@ -223,6 +223,7 @@ static void name_rev(struct commit *start_commit,\n> \t\t\tif (commit_is_before_cutoff(parent))\n> \t\t\t\tcontinue;\n> \n> +\t\t\tASSERT_FOR_FANALYZER(name);\n> \t\t\tif (parent_number > 1) {\n> \t\t\t\tgeneration = 0;\n> \t\t\t\tdistance = name->distance + MERGE_TRAVERSAL_WEIGHT;\n> diff --git a/config.mak.dev b/config.mak.dev\n> index d6f5be92297..1d47fc04163 100644\n> --- a/config.mak.dev\n> +++ b/config.mak.dev\n> @@ -83,6 +83,10 @@ endif\n> \n> DEVELOPER_CFLAGS += -fanalyzer\n> \n> +ifeq ($(filter no-suppress-analyzer,$(DEVOPTS)),)\n> +DEVELOPER_CFLAGS += -DSUPPRESS_FSANITIZER\n> +endif\n> +\n> ## -fanalyzer exists exists as of gcc10, but versions older than gcc12\n> ## have a lot of false positives.\n> ifeq ($(filter gcc12,$(COMPILER_FEATURES)),)\n> diff --git a/dir.c b/dir.c\n> index 3633c0852ee..c80b584b4a7 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -3781,6 +3781,7 @@ static void invalidate_one_directory(struct untracked_cache *uc,\n> \t\t\t\t struct untracked_cache_dir *ucd)\n> {\n> \tuc->dir_invalidated++;\n> +\tASSERT_FOR_FANALYZER(ucd);\n> \tucd->valid = 0;\n> \tucd->untracked_nr = 0;\n> }\n> diff --git a/git-compat-util.h b/git-compat-util.h\n> index 96293b6c43a..a553884c048 100644\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -984,6 +984,21 @@ static inline unsigned long cast_size_t_to_ulong(size_t a)\n> \treturn (unsigned long)a;\n> }\n> \n> +/**\n> + * Transitory helper macro to quiet currently known GCC -fsanitizer\n> + * issues.\n> + *\n> + * We lie to it and say that we're confident that the given \"expr\" to\n> + * ASSERT_FOR_FANALYZER() cannot be NULL (or 0). Those\n> + * grandfathered-in issues should be fixed, but at least we're\n> + * stemming the tide.\n> + */\n> +#ifdef SUPPRESS_FSANITIZER\n> +#define ASSERT_FOR_FANALYZER(expr) assert((expr))\n> +#else\n> +#define ASSERT_FOR_FANALYZER(expr) do {} while (0)\n> +#endif\n> +\n> #ifdef HAVE_ALLOCA_H\n> # include \n> # define xalloca(size) (alloca(size))\n> @@ -1037,6 +1052,7 @@ int xstrncmpz(const char *s, const char *t, size_t len);\n> \tBUILD_ASSERT_OR_ZERO(sizeof(*(dst)) == sizeof(*(src))))\n> static inline void copy_array(void *dst, const void *src, size_t n, size_t size)\n> {\n> +\tASSERT_FOR_FANALYZER(dst);\n> \tif (n)\n> \t\tmemcpy(dst, src, st_mult(size, n));\n> }\n> diff --git a/gpg-interface.c b/gpg-interface.c\n> index 280f1fa1a58..9cba3b86e45 100644\n> --- a/gpg-interface.c\n> +++ b/gpg-interface.c\n> @@ -242,6 +242,7 @@ static void parse_gpg_output(struct signature_check *sigc)\n> \t\t\t\t\tnext = strchrnul(line, ' ');\n> \t\t\t\t\treplace_cstring(&sigc->key, line, next);\n> \t\t\t\t\t/* Do we have signer information? */\n> +\t\t\t\t\tASSERT_FOR_FANALYZER(next);\n> \t\t\t\t\tif (*next && (sigcheck_gpg_status[i].flags & GPG_STATUS_UID)) {\n> \t\t\t\t\t\tline = next + 1;\n> \t\t\t\t\t\tnext = strchrnul(line, '\\n');\n> @@ -283,6 +284,7 @@ static void parse_gpg_output(struct signature_check *sigc)\n> \t\t\t\t\t */\n> \t\t\t\t\tlimit = strchrnul(line, '\\n');\n> \t\t\t\t\tfor (j = 9; j > 0; j--) {\n> +\t\t\t\t\t\tASSERT_FOR_FANALYZER(next);\n> \t\t\t\t\t\tif (!*next || limit <= next)\n> \t\t\t\t\t\t\tbreak;\n> \t\t\t\t\t\tline = next + 1;\n> diff --git a/graph.c b/graph.c\n> index 568b6e7cd41..39f7e95d4ab 100644\n> --- a/graph.c\n> +++ b/graph.c\n> @@ -1105,6 +1105,7 @@ static void graph_output_post_merge_line(struct git_graph *graph, struct graph_l\n> \t\t\tseen_this = 1;\n> \n> \t\t\tfor (j = 0; j < graph->num_parents; j++) {\n> +\t\t\t\tASSERT_FOR_FANALYZER(parents);\n> \t\t\t\tpar_column = graph_find_new_column_by_commit(graph, parents->item);\n> \t\t\t\tassert(par_column >= 0);\n> \n> @@ -1138,6 +1139,7 @@ static void graph_output_post_merge_line(struct git_graph *graph, struct graph_l\n> \t\t\t}\n> \t\t}\n> \n> +\t\tASSERT_FOR_FANALYZER(first_parent);\n> \t\tif (col_commit == first_parent->item)\n> \t\t\tparent_col = col;\n> \t}\n> diff --git a/line-log.c b/line-log.c\n> index 51d93310a4d..1295f46deaf 100644\n> --- a/line-log.c\n> +++ b/line-log.c\n> @@ -154,6 +154,7 @@ static void range_set_union(struct range_set *out,\n> \twhile (i < a->nr || j < b->nr) {\n> \t\tstruct range *new_range;\n> \t\tif (i < a->nr && j < b->nr) {\n> +\t\t\tASSERT_FOR_FANALYZER(rb);\n> \t\t\tif (ra[i].start < rb[j].start)\n> \t\t\t\tnew_range = &ra[i++];\n> \t\t\telse if (ra[i].start > rb[j].start)\n> @@ -166,6 +167,7 @@ static void range_set_union(struct range_set *out,\n> \t\t\tnew_range = &ra[i++];\n> \t\telse /* a exhausted */\n> \t\t\tnew_range = &rb[j++];\n> +\t\tASSERT_FOR_FANALYZER(new_range);\n> \t\tif (new_range->start == new_range->end)\n> \t\t\t; /* empty range */\n> \t\telse if (!out->nr || out->ranges[out->nr-1].end < new_range->start) {\n> diff --git a/unpack-trees.c b/unpack-trees.c\n> index a1d0ff3a4d3..b6e10b05a00 100644\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -2195,6 +2195,7 @@ static int verify_clean_subdirectory(const struct cache_entry *ce,\n> \tchar *pathbuf;\n> \tint cnt = 0;\n> \n> +\tASSERT_FOR_FANALYZER(ce);\n> \tif (S_ISGITLINK(ce->ce_mode)) {\n> \t\tstruct object_id oid;\n> \t\tint sub_head = resolve_gitlink_ref(ce->name, \"HEAD\", &oid);\n> diff --git a/utf8.c b/utf8.c\n> index de4ce5c0e68..3ffc0a97f3b 100644\n> --- a/utf8.c\n> +++ b/utf8.c\n> @@ -130,6 +130,7 @@ static ucs_char_t pick_one_utf8_char(const char **start, size_t *remainder_p)\n> \t */\n> \tremainder = (remainder_p ? *remainder_p : 999);\n> \n> +\tASSERT_FOR_FANALYZER(remainder < 1 || s);\n> \tif (remainder < 1) {\n> \t\tgoto invalid;\n> \t} else if (*s < 0x80) {\n> -- \n> 2.36.1.1124.g577fa9c2ebd\n>\n"},{"id":"456685","messageId":"220604.86h750wgap.gmgdl@evledraar.gmail.com","threadId":"57952","inReplyTo":"2064938784.539963.1654347067250@apps.talktalk.co.uk","subject":"Re: [RFC PATCH 01/15] remote.c: don't dereference NULL in freeing loop","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-04T16:20:54Z","receivedAt":"2022-06-04T16:21:25Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Jun 04 2022, Phillip Wood wrote:\n\n>> On 03 June 2022 at 22:07 René Scharfe wrote:\n>> \n>> \n>> Am 03.06.22 um 20:37 schrieb Ævar Arnfjörð Bjarmason:\n>> > Fix a bug in fd3cb0501e1 (remote: move static variables into\n>> > per-repository struct, 2021-11-17) where we'd free(remote->pushurl[i])\n>> > after having NULL'd out remote->pushurl. itself.\n>> >\n>> > While we're at it let's get rid of the redundant braces per the\n>> > CodingGuidelines, which also serves to show in the diff context that\n>> > we were doing a FREE_AND_NULL(remote->pushurl) afterwards too, let's\n>> > keep that one.\n>> \n>> The extended context is helping, but the brace removal makes this change\n>> harder to read.\n>\n> Indeed, a small style fix in a larger change is one thing but here at\n> least 80% of the changed lines are unrelated to the bugfix. I'm afraid\n> my heart has started to sink when I see the phrase \"while we're at it\"\n> in a commit message.\n\nI'm happy to omit it, I thought it was helpful to force the context to\nbe shown, will change that.\n"},{"id":"456686","messageId":"220604.86czfowg81.gmgdl@evledraar.gmail.com","threadId":"57952","inReplyTo":"870925774.539833.1654346793623@apps.talktalk.co.uk","subject":"Re: [RFC PATCH 07/15] strbuf.c: placate -fanalyzer in strbuf_grow()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-04T16:21:53Z","receivedAt":"2022-06-04T16:23:01Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Jun 04 2022, Phillip Wood wrote:\n\n> Hi Ævar\n>\n> [This is an old address that I only have webmail access to, please use\n> phillip.wood@dunelm.org.uk when cc'ing me]\n\nHrm, I just grabbed it from an old commit of yours I found. Consider\nsending in a patch to update .mailmap :)\n\n>> On 03 June 2022 at 19:37 Ævar Arnfjörð Bjarmason wrote:\n>> \n>> \n>> Change the strbuf_grow() function so that GCC v12's -fanalyze doesn't\n>> yell at us about sb->buf[0] dereferencing NULL, this also makes this\n>> code easier to follow.\n>> \n>> This BUG() should be unreachable since the state of our \"sb->buf\" and\n>> \"sb->alloc\" goes hand-in-hand, but -fanalyzer isn't smart enough to\n>> know that, and adding the BUG() also makes it clearer to human readers\n>> that that's what happens here.\n>> \n>> Signed-off-by: Ævar Arnfjörð Bjarmason \n>> ---\n>> strbuf.c | 2 ++\n>> 1 file changed, 2 insertions(+)\n>> \n>> diff --git a/strbuf.c b/strbuf.c\n>> index dd9eb85527a..61c4630aeeb 100644\n>> --- a/strbuf.c\n>> +++ b/strbuf.c\n>> @@ -97,6 +97,8 @@ void strbuf_grow(struct strbuf *sb, size_t extra)\n>> \tif (new_buf)\n>> \t\tsb->buf = NULL;\n>> \tALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc);\n>> +\tif (new_buf && !sb->buf)\n>> +\t\tBUG(\"for a new buffer ALLOC_GROW() should always do work!\");\n>\n> This is a bit ugly, have you tried adding\n> __attribute__((malloc (free), returns_nonnull))\n> to xmalloc() and xrealloc() ?\n>\n\nWill try to experiment with that, perhaps GCC can be massaged to grok\nthis somehow.\n\nI do vaguely remember (but couldn't track down where it was) that we\nhave some config for coverity for this function, due to it also having\ntrouble with it.\n"},{"id":"456687","messageId":"220604.868rqcwfnw.gmgdl@evledraar.gmail.com","threadId":"57952","inReplyTo":"1de74bf7-0e75-8ae3-6ea7-62939b540061@web.de","subject":"Re: [RFC PATCH 03/15] reftable: don't memset() a NULL from failed malloc()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-04T16:23:13Z","receivedAt":"2022-06-04T16:35:11Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Jun 04 2022, René Scharfe wrote:\n\n> Am 04.06.22 um 02:54 schrieb Ævar Arnfjörð Bjarmason:\n>>\n>> To your comment here & some others (e.g. FREE_AND_NULL()): I was really\n>> trying to focus on narrowly addressing these -fanalyzer issues without\n>> digressing into the larger topics \"what is this code *really* doing, and\n>> does it make sense?\". It was pretty unavoidable in 13/15 though.\n>>\n>> Which isn't to say that I shouldn't fix some of it, e.g. your\n>> s/return/BUG()/ suggestion, but I think it's best to view these patches\n>> with an eye towards us already having these issues, and in most cases\n>> making -fanalyzer happy is a small cost.\n>>\n>> And by doing so and getting a \"clean build\" we'll be able to turn it on\n>> in CI, and thus notice when we run into new -fanalyzer issues.\n>\n> Future analyzer reports are likely of the same quality as the current\n> ones. If the goal is to shush them then we should just not use the\n> analyzer. If reports contain a helpful signal, e.g. pointing to a real\n> bug or to overly complicated code, then we better address these issues.\n>\n> We can think about automating the analyzer once we have a certain number\n> of commits with improvements that would not have been made without it.\n\nWe might decide not to go with -fanalyzer in CI or whatever, but I\nreally think that your line of reasoning here is just the wrong way to\nevaluate the cost/benefit of -fanalyzer, a new warning or whatever.\n\nThere's ~15 commits in this series addressing things -fanalyzer brought\nup, and it would be ~20 if the remaining issues I punted on were\naddressed.\n\nThe question shouldn't be whether those things in particular were worth\nthe effort, but whether the added safety of getting the new diagnostic\ngoing forward is worth the one-time cost.\n\nSome of these commits are fixing issues going back to 2007-ish, $(git\nlog --no-merges --oneline -- '*.[ch]' | wc -l) is ~25k lines. And\nlooking at it like that 20/25K isn't that bad of a ratio :)\n\nFWIW I spotted a couple of bugs in my own unsubmitted code from running\nall of it through -fanalyzer, and that POV is also worth thinking about,\ni.e. it's not just about improving git's current code, or even commits\nthat might land in git.git in the future.\n\nBut also to provide a development aid so that when we're writing patches\nwe spot issues earlier, even if they're ones we might spot before we\nsend the patch, or in review before it gets applied.\n\nIt's also a much faster way of spotting certain issues, if you take into\naccount that we've already been spotting some of these with the likes of\nSANITIZE=address, valgrind runs, or coverity.\n\nI find the warning output from -fanalyzer to be *really useful*. It's\nscarily verbose at first, but it's basically doing most of the work for\nyou in terms of exhaustively describing how the control flow got to a\ngiven location. With e.g. SANITIZE=address and valgrind (to the extent\nthat they overlap) you might get a stacktrace or two, but you generally\nhave to chase all that down yourself.\n\n"},{"id":"456689","messageId":"6679c09e-e691-f360-ace8-aae17ecabdc4@web.de","threadId":"57952","inReplyTo":"RFC-patch-06.15-2d04035d7aa-20220603T183608Z-avarab@gmail.com","subject":"Re: [RFC PATCH 06/15] ref-filter.c: BUG() out on show_ref() with NULL refname","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-06-04T18:07:32Z","receivedAt":"2022-06-04T18:07:58Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 03.06.22 um 20:37 schrieb Ævar Arnfjörð Bjarmason:\n> Adjust code originally added in 5339bdad96a (ref-filter: introduce\n> remote_ref_atom_parser(), 2016-02-17) to BUG() out rather than\n> potentially segfault if we get a NULL refname here.\n>\n> As noted by GCC v12's -fanalyzer that will happen if this follows the\n> \"refname = NULL\" branch added in cc72385fe35 (for-each-ref: let\n> upstream/push optionally report the remote name, 2017-10-05).\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> ref-filter.c | 4 +++-\n> 1 file changed, 3 insertions(+), 1 deletion(-)\n>\n> diff --git a/ref-filter.c b/ref-filter.c\n> index 2413f889f48..91aa8e89268 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -1644,7 +1644,9 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname,\n> \t\t\t\t struct branch *branch, const char **s)\n> {\n> \tint num_ours, num_theirs;\n> -\tif (atom->u.remote_ref.option == RR_REF)\n> +\tif (atom->u.remote_ref.option == RR_REF && !refname)\n> +\t\tBUG(\"must get refname with [...]remote_ref.option == RR_REF\");\n> +\telse if (atom->u.remote_ref.option == RR_REF)\n\nThe related code not shown here looks tricky. IIUC refname cannot be\nNULL if option is RR_REF, because remote_ref_atom_parser() only sets\n.push_remote for other option values and only if that's set we can get a\nNULL refname passed to fill_remote_ref_details(). Right?\n\nDo we need to have two different ways to indicate remote-ness? Having\njust one like after the patch below should reduce the risk of confusion.\n\n> \t\t*s = show_ref(&atom->u.remote_ref.refname, refname);\n> \telse if (atom->u.remote_ref.option == RR_TRACK) {\n> \t\tif (stat_tracking_info(branch, &num_ours, &num_theirs,\n\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 2413f889f4..260257b92c 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -183,7 +183,7 @@ static struct used_atom {\n \t\t\t\tRR_REF, RR_TRACK, RR_TRACKSHORT, RR_REMOTE_NAME, RR_REMOTE_REF\n \t\t\t} option;\n \t\t\tstruct refname_atom refname;\n-\t\t\tunsigned int nobracket : 1, push : 1, push_remote : 1;\n+\t\t\tunsigned int nobracket : 1, push : 1;\n \t\t} remote_ref;\n \t\tstruct {\n \t\t\tenum { C_BARE, C_BODY, C_BODY_DEP, C_LENGTH, C_LINES,\n@@ -295,10 +295,8 @@ static int remote_ref_atom_parser(struct ref_format *format, struct used_atom *a\n \t\t\tatom->u.remote_ref.nobracket = 1;\n \t\telse if (!strcmp(s, \"remotename\")) {\n \t\t\tatom->u.remote_ref.option = RR_REMOTE_NAME;\n-\t\t\tatom->u.remote_ref.push_remote = 1;\n \t\t} else if (!strcmp(s, \"remoteref\")) {\n \t\t\tatom->u.remote_ref.option = RR_REMOTE_REF;\n-\t\t\tatom->u.remote_ref.push_remote = 1;\n \t\t} else {\n \t\t\tatom->u.remote_ref.option = RR_REF;\n \t\t\tif (refname_atom_parser_internal(&atom->u.remote_ref.refname,\n@@ -1891,7 +1889,8 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)\n \t\t\t\tcontinue;\n \t\t\tbranch = branch_get(branch_name);\n\n-\t\t\tif (atom->u.remote_ref.push_remote)\n+\t\t\tif (atom->u.remote_ref.option == RR_REMOTE_NAME ||\n+\t\t\t atom->u.remote_ref.option == RR_REMOTE_REF)\n \t\t\t\trefname = NULL;\n \t\t\telse {\n \t\t\t\trefname = branch_get_push(branch, NULL);\n"},{"id":"456693","messageId":"3d2b349b-81aa-fd29-552d-5719dd75003d@web.de","threadId":"57952","inReplyTo":"220604.868rqcwfnw.gmgdl@evledraar.gmail.com","subject":"Re: [RFC PATCH 03/15] reftable: don't memset() a NULL from failed malloc()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-06-04T20:31:17Z","receivedAt":"2022-06-04T20:31:56Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 04.06.22 um 18:23 schrieb Ævar Arnfjörð Bjarmason:\n>\n> On Sat, Jun 04 2022, René Scharfe wrote:\n>\n>> Am 04.06.22 um 02:54 schrieb Ævar Arnfjörð Bjarmason:\n>>>\n>>> To your comment here & some others (e.g. FREE_AND_NULL()): I was really\n>>> trying to focus on narrowly addressing these -fanalyzer issues without\n>>> digressing into the larger topics \"what is this code *really* doing, and\n>>> does it make sense?\". It was pretty unavoidable in 13/15 though.\n>>>\n>>> Which isn't to say that I shouldn't fix some of it, e.g. your\n>>> s/return/BUG()/ suggestion, but I think it's best to view these patches\n>>> with an eye towards us already having these issues, and in most cases\n>>> making -fanalyzer happy is a small cost.\n>>>\n>>> And by doing so and getting a \"clean build\" we'll be able to turn it on\n>>> in CI, and thus notice when we run into new -fanalyzer issues.\n>>\n>> Future analyzer reports are likely of the same quality as the current\n>> ones. If the goal is to shush them then we should just not use the\n>> analyzer. If reports contain a helpful signal, e.g. pointing to a real\n>> bug or to overly complicated code, then we better address these issues.\n>>\n>> We can think about automating the analyzer once we have a certain number\n>> of commits with improvements that would not have been made without it.\n>\n> We might decide not to go with -fanalyzer in CI or whatever, but I\n> really think that your line of reasoning here is just the wrong way to\n> evaluate the cost/benefit of -fanalyzer, a new warning or whatever.\n>\n> There's ~15 commits in this series addressing things -fanalyzer brought\n> up, and it would be ~20 if the remaining issues I punted on were\n> addressed.\n>\n> The question shouldn't be whether those things in particular were worth\n> the effort, but whether the added safety of getting the new diagnostic\n> going forward is worth the one-time cost.\n\nIs it pointing out real issues? Yes, the ones from patches 1 and 2 look\nlegit. Let's get them fixed and attribute -fanalyzer for finding them!\nThey are good selling points.\n\nIs it giving us false positives? Patches 4, 5 and 6 look like that to\nme. Perhaps we can still use this opportunity to simplify the code for\nboth human and machine, or at least add helpful asserts.\n\nPatch 3 is a bit surprising to me. It's a real issue, but the whole\nreftable code never bothers to check for allocation errors, so I would\nhave expected the analyzer to report lots more places.\n\nIs it worth automating the thing? Perhaps, but we better work through\nthe reports we already have first before getting new ones.\n\n> FWIW I spotted a couple of bugs in my own unsubmitted code from running\n> all of it through -fanalyzer, and that POV is also worth thinking about,\n> i.e. it's not just about improving git's current code, or even commits\n> that might land in git.git in the future.\n>\n> But also to provide a development aid so that when we're writing patches\n> we spot issues earlier, even if they're ones we might spot before we\n> send the patch, or in review before it gets applied.\n>\n> It's also a much faster way of spotting certain issues, if you take into\n> account that we've already been spotting some of these with the likes of\n> SANITIZE=address, valgrind runs, or coverity.\n>\n> I find the warning output from -fanalyzer to be *really useful*. It's\n> scarily verbose at first, but it's basically doing most of the work for\n> you in terms of exhaustively describing how the control flow got to a\n> given location. With e.g. SANITIZE=address and valgrind (to the extent\n> that they overlap) you might get a stacktrace or two, but you generally\n> have to chase all that down yourself.\n\nSounds good, makes me want to try it. And I actually can, to a certain\ndegree: GCC 13 seems to get confused when compiling compat/ code on my\nmachine (macOS). Not sure what's going on. Compiling e.g. strbuf.c\nworks, though, and gives me the same false positive. :-|\n\nRené\n"},{"id":"456694","messageId":"8a5cdd7a-13bf-b091-e947-657aeb094c38@web.de","threadId":"57952","inReplyTo":"220604.86czfowg81.gmgdl@evledraar.gmail.com","subject":"Re: [RFC PATCH 07/15] strbuf.c: placate -fanalyzer in strbuf_grow()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-06-04T20:37:36Z","receivedAt":"2022-06-04T20:38:05Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 04.06.22 um 18:21 schrieb Ævar Arnfjörð Bjarmason:\n>\n> On Sat, Jun 04 2022, Phillip Wood wrote:\n>\n>> Hi Ævar\n>>\n>> [This is an old address that I only have webmail access to, please use\n>> phillip.wood@dunelm.org.uk when cc'ing me]\n>\n> Hrm, I just grabbed it from an old commit of yours I found. Consider\n> sending in a patch to update .mailmap :)\n>\n>>> On 03 June 2022 at 19:37 Ævar Arnfjörð Bjarmason wrote:\n>>>\n>>>\n>>> Change the strbuf_grow() function so that GCC v12's -fanalyze doesn't\n>>> yell at us about sb->buf[0] dereferencing NULL, this also makes this\n>>> code easier to follow.\n>>>\n>>> This BUG() should be unreachable since the state of our \"sb->buf\" and\n>>> \"sb->alloc\" goes hand-in-hand, but -fanalyzer isn't smart enough to\n>>> know that, and adding the BUG() also makes it clearer to human readers\n>>> that that's what happens here.\n>>>\n>>> Signed-off-by: Ævar Arnfjörð Bjarmason \n>>> ---\n>>> strbuf.c | 2 ++\n>>> 1 file changed, 2 insertions(+)\n>>>\n>>> diff --git a/strbuf.c b/strbuf.c\n>>> index dd9eb85527a..61c4630aeeb 100644\n>>> --- a/strbuf.c\n>>> +++ b/strbuf.c\n>>> @@ -97,6 +97,8 @@ void strbuf_grow(struct strbuf *sb, size_t extra)\n>>> \tif (new_buf)\n>>> \t\tsb->buf = NULL;\n>>> \tALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc);\n>>> +\tif (new_buf && !sb->buf)\n>>> +\t\tBUG(\"for a new buffer ALLOC_GROW() should always do work!\");\n>>\n>> This is a bit ugly, have you tried adding\n>> __attribute__((malloc (free), returns_nonnull))\n>> to xmalloc() and xrealloc() ?\n>>\n>\n> Will try to experiment with that, perhaps GCC can be massaged to grok\n> this somehow.\n>\n> I do vaguely remember (but couldn't track down where it was) that we\n> have some config for coverity for this function, due to it also having\n> trouble with it.\n\nGood idea, but this attribute doesn't seem to help here.\n\nThe following helps, but I don't know why it would be needed -- if alloc\nis 0 then any strbuf_grow() call will give a nr of at least 1 (for the\nNUL character):\n\n\ndiff --git a/cache.h b/cache.h\nindex 595582becc..fe10789959 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -707,7 +707,7 @@ int daemonize(void);\n */\n #define ALLOC_GROW(x, nr, alloc) \\\n \tdo { \\\n-\t\tif ((nr) > alloc) { \\\n+\t\tif (!alloc || (nr) > alloc) { \\\n \t\t\tif (alloc_nr(alloc) < (nr)) \\\n \t\t\t\talloc = (nr); \\\n \t\t\telse \\\n"},{"id":"456695","messageId":"5db7bbf7-a6be-20c9-8233-92fbcb94a266@web.de","threadId":"57952","inReplyTo":"RFC-patch-08.15-2c4b7832144-20220603T183608Z-avarab@gmail.com","subject":"Re: [RFC PATCH 08/15] strbuf.c: use st_add3(), not unsigned_add_overflows()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-06-04T21:27:43Z","receivedAt":"2022-06-04T21:28:15Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 03.06.22 um 20:37 schrieb Ævar Arnfjörð Bjarmason:\n> Change the strbuf_grow() function to use st_add3() instead of doing\n> its own unsigned_add_overflows() checks. The overflow checking here\n> was originally added in b449f4cfc97 (Rework strbuf API and semantics.,\n> 2007-09-06) and adjusted in 1368f65002b (compat: helper for detecting\n> unsigned overflow, 2010-10-10). Instead we compute a \"sz\" with\n> st_add3().\n\nGood idea.\n\n> That was done at a time when the underlying xrealloc() in\n> git-compat-util.h didn't use st_mult() yet, that has been the case\n> since the later e7792a74bcf (harden REALLOC_ARRAY and xcalloc against\n> size_t overflow, 2016-02-22).\n\nHow is that relevant?\n\n> The only behavior change here should be the very obscure edge case\n> that we'd previously die() in cases where we strictly didn't need to,\n> as we'd check both \"extra + 1\" and \"sb->len + extra + 1\" for\n> overflow. If we overflowed only on the latter but were doing the\n> former we'd needlessly die() die.\n\nst_add3 does the same checks. What do you mean with \"doing the former\"?\n\n> I don't think that difference\n> mattered, but it's noted here for completeness.\n>\n> While we're at it add an inline comment about why we're adding 1 to\n> the values, that's also explained in the API documentation in\n> strbuf.h, but since we're using that magic constant here...\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> strbuf.c | 8 ++++----\n> 1 file changed, 4 insertions(+), 4 deletions(-)\n>\n> diff --git a/strbuf.c b/strbuf.c\n> index 61c4630aeeb..f0a74d2bfb1 100644\n> --- a/strbuf.c\n> +++ b/strbuf.c\n> @@ -91,12 +91,12 @@ void strbuf_attach(struct strbuf *sb, void *buf, size_t len, size_t alloc)\n> void strbuf_grow(struct strbuf *sb, size_t extra)\n> {\n> \tint new_buf = !sb->alloc;\n> -\tif (unsigned_add_overflows(extra, 1) ||\n> -\t unsigned_add_overflows(sb->len, extra + 1))\n> -\t\tdie(\"you want to use way too much memory\");\n> +\tconst size_t sz_buf = new_buf ? 0 : sb->len;\n\nThis is a change in behavior. The one you meant above? It ignores the\nlen value if alloc is zero. Any len value other than zero is invalid in\nthat case. Adding code to paper over this invalid state seems a waste.\nAnd if a brave caller fiddled with len before calling strbuf_grow(), the\nresulting allocation will now be shorter. Hopefully nobody does that,\nbut it's not totally impossible.\n\nWe could report the issue instead with something like the following:\n\n\tif (new_buf && sb->len)\n\t\tBUG(\"unallocated strbuf has invalid len: %\"PRIuMAX, (uintmax_t)sb->len);\n\nI'd rather keep the old behavior. But whatever we decide to do, I think\nthe st_add3 change deserves its own commit with no further change in\nbehavior.\n\n> +\tconst size_t sz = st_add3(sz_buf, extra, 1 /* for \\0 */);\n\nI don't mind the added comment.\n\n> +\n> \tif (new_buf)\n> \t\tsb->buf = NULL;\n> -\tALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc);\n> +\tALLOC_GROW(sb->buf, sz, sb->alloc);\n> \tif (new_buf && !sb->buf)\n> \t\tBUG(\"for a new buffer ALLOC_GROW() should always do work!\");\n> \tif (new_buf)\n"},{"id":"456696","messageId":"bcbdd8db-763e-b910-3fe7-a5f11d3ff39b@gmail.com","threadId":"57952","inReplyTo":"8a5cdd7a-13bf-b091-e947-657aeb094c38@web.de","subject":"Re: [RFC PATCH 07/15] strbuf.c: placate -fanalyzer in strbuf_grow()","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-06-05T10:20:37Z","receivedAt":"2022-06-05T10:20:47Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 04/06/2022 21:37, René Scharfe wrote:\n> Am 04.06.22 um 18:21 schrieb Ævar Arnfjörð Bjarmason:\n>>\n>> On Sat, Jun 04 2022, Phillip Wood wrote:\n>>\n>>> Hi Ævar\n>>>\n>>> [This is an old address that I only have webmail access to, please use\n>>> phillip.wood@dunelm.org.uk when cc'ing me]\n>>\n>> Hrm, I just grabbed it from an old commit of yours I found. Consider\n>> sending in a patch to update .mailmap :)\n>>\n>>>> On 03 June 2022 at 19:37 Ævar Arnfjörð Bjarmason wrote:\n>>>>\n>>>>\n>>>> Change the strbuf_grow() function so that GCC v12's -fanalyze doesn't\n>>>> yell at us about sb->buf[0] dereferencing NULL, this also makes this\n>>>> code easier to follow.\n>>>>\n>>>> This BUG() should be unreachable since the state of our \"sb->buf\" and\n>>>> \"sb->alloc\" goes hand-in-hand, but -fanalyzer isn't smart enough to\n>>>> know that, and adding the BUG() also makes it clearer to human readers\n>>>> that that's what happens here.\n>>>>\n>>>> Signed-off-by: Ævar Arnfjörð Bjarmason \n>>>> ---\n>>>> strbuf.c | 2 ++\n>>>> 1 file changed, 2 insertions(+)\n>>>>\n>>>> diff --git a/strbuf.c b/strbuf.c\n>>>> index dd9eb85527a..61c4630aeeb 100644\n>>>> --- a/strbuf.c\n>>>> +++ b/strbuf.c\n>>>> @@ -97,6 +97,8 @@ void strbuf_grow(struct strbuf *sb, size_t extra)\n>>>> \tif (new_buf)\n>>>> \t\tsb->buf = NULL;\n>>>> \tALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc);\n>>>> +\tif (new_buf && !sb->buf)\n>>>> +\t\tBUG(\"for a new buffer ALLOC_GROW() should always do work!\");\n>>>\n>>> This is a bit ugly, have you tried adding\n>>> __attribute__((malloc (free), returns_nonnull))\n>>> to xmalloc() and xrealloc() ?\n>>>\n>>\n>> Will try to experiment with that, perhaps GCC can be massaged to grok\n>> this somehow.\n>>\n>> I do vaguely remember (but couldn't track down where it was) that we\n>> have some config for coverity for this function, due to it also having\n>> trouble with it.\n> \n> Good idea, but this attribute doesn't seem to help here.\n\nIndeed, I was tricked by the misleading BUG message, the analyzer is not \ncomplaining that xrealloc() may return NULL, it is taking a path where \nit does not reallocate the buffer. I've copied the full output at the \nend of the message if anyone wants to see the whole thing but the \nrelevant part is\n\n |cache.h:727:20:\n | 727 | if ((nr) > alloc) { \\\n | | ^\n | | |\n | | (10) following ‘false’ branch...\nstrbuf.c:99:9: note: in expansion of macro ‘ALLOC_GROW’\n | 99 | ALLOC_GROW(sb->buf, sb->len + extra + 1, \nsb->alloc);\n | | ^~~~~~~~~~\n\n> The following helps, but I don't know why it would be needed -- if alloc\n> is 0 then any strbuf_grow() call will give a nr of at least 1 (for the\n> NUL character):\n\nYes it seems to be ignoring the result of our overflow checks and \nassuming that sb->len + extra + 1 can overflow to zero. Having seen the \nnumber of false positives I'm inclined to think we should take the bug \nfixes and punt the rest of these patches. Maybe after a couple more gcc \nreleases the false positive rate will be more manageable. (I tired \nrunning gcc 11 with -fanalyzer a few months ago and gave up looking at \nthe output before I found a single real bug so it has certainly improved \nin gcc 12)\n\nBest Wishes\n\nPhillip\n\nstrbuf.c: In function ‘strbuf_grow’:\nstrbuf.c:101:28: warning: dereference of NULL ‘0’ [CWE-476] \n[-Wanalyzer-null-dereference]\n 101 | sb->buf[0] = '\\0';\n | ~~~~~~~~~~~^~~~~~\n ‘strbuf_normalize_path’: events 1-2\n |\n | 1156 | int strbuf_normalize_path(struct strbuf *src)\n | | ^~~~~~~~~~~~~~~~~~~~~\n | | |\n | | (1) entry to ‘strbuf_normalize_path’\n |......\n | 1160 | strbuf_grow(&dst, src->len);\n | | ~~~~~~~~~~~~~~~~~~~~~~~~~~~\n | | |\n | | (2) calling ‘strbuf_grow’ from ‘strbuf_normalize_path’\n |\n +--> ‘strbuf_grow’: events 3-4\n |\n | 91 | void strbuf_grow(struct strbuf *sb, size_t extra)\n | | ^~~~~~~~~~~\n | | |\n | | (3) entry to ‘strbuf_grow’\n |......\n | 94 | if (unsigned_add_overflows(extra, 1) ||\n | | ~\n | | |\n | | (4) following ‘false’ branch (when \n‘extra != 18446744073709551615’)...\n |\n ‘strbuf_grow’: event 5\n |\n | 95 | unsigned_add_overflows(sb->len, extra + 1))\ngit-compat-util.h:128:7: note: in definition of macro \n‘unsigned_add_overflows’\n | 128 | ((b) > maximum_unsigned_value_of_type(a) - (a))\n | | ^\n |\n ‘strbuf_grow’: events 6-9\n |\n |strbuf.c:94:46:\n | 94 | if (unsigned_add_overflows(extra, 1) ||\n |......\n | 97 | if (new_buf)\n | | ~~ ~\n | | | |\n | | | (8) following ‘true’ branch...\n | | (7) ...to here\n | 98 | sb->buf = NULL;\n | | ~~\n | | |\n | | (9) ...to here\n |\n ‘strbuf_grow’: event 10\n |\n |cache.h:727:20:\n | 727 | if ((nr) > alloc) { \\\n | | ^\n | | |\n | | (10) following ‘false’ branch...\nstrbuf.c:99:9: note: in expansion of macro ‘ALLOC_GROW’\n | 99 | ALLOC_GROW(sb->buf, sb->len + extra + 1, \nsb->alloc);\n | | ^~~~~~~~~~\n |\n ‘strbuf_grow’: event 11\n |\n |cache.h:726:12:\n | 726 | do { \\\n | | ^\n | | |\n | | (11) ...to here\nstrbuf.c:99:9: note: in expansion of macro ‘ALLOC_GROW’\n | 99 | ALLOC_GROW(sb->buf, sb->len + extra + 1, \nsb->alloc);\n | | ^~~~~~~~~~\n |\n ‘strbuf_grow’: events 12-15\n |\n | 100 | if (new_buf)\n | | ^\n | | |\n | | (12) following ‘true’ branch...\n | 101 | sb->buf[0] = '\\0';\n | | ~~~~~~~~~~~~~~~~~\n | | | | |\n | | | | (15) dereference of NULL \n‘*sb.buf’\n | | | (14) ‘dst.buf’ is NULL\n | | (13) ...to here\n\n"},{"id":"456724","messageId":"xmqq7d5tag3s.fsf@gitster.g","threadId":"57952","inReplyTo":"220604.868rqcwfnw.gmgdl@evledraar.gmail.com","subject":"Re: [RFC PATCH 03/15] reftable: don't memset() a NULL from failed malloc()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-06T16:53:11Z","receivedAt":"2022-06-06T17:00:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n>> Future analyzer reports are likely of the same quality as the current\n>> ones. If the goal is to shush them then we should just not use the\n>> analyzer. If reports contain a helpful signal, e.g. pointing to a real\n>> bug or to overly complicated code, then we better address these issues.\n>>\n>> We can think about automating the analyzer once we have a certain number\n>> of commits with improvements that would not have been made without it.\n>\n> We might decide not to go with -fanalyzer in CI or whatever, but I\n> really think that your line of reasoning here is just the wrong way to\n> evaluate the cost/benefit of -fanalyzer, a new warning or whatever.\n\nTo me, your priorities look much less in line with this project's\nthan what Rt Réne gave us above.\n\n> The question shouldn't be whether those things in particular were worth\n> the effort, but whether the added safety of getting the new diagnostic\n> going forward is worth the one-time cost.\n\nWorkarounds for false positives are hardly one-time cost. They have\nto stay with us until the -fanalyzer gets corrected, somebody needs\nto remember to occasionally check if that happened, and revert the\nworkaround to bring the code into their more natural form. What has\nbeen good and readable for human programmers for a long time does\nnot need to be touched just to work around a false positive bug in a\nnew tool.\n\n> I find the warning output from -fanalyzer to be *really useful*.\n\nI do not mind if you add -fanalyzer during your build via your own\nconfig.mak file, and you would help them improve the analyzer by\nreporting false positive bugs while finding possible bugs in the\ncode we have (like you did in a few patches in this series) and the\ncode you are writing. You are capable enough to keep your own set\nof patches to work around their false positive bugs locally.\n\nBut if you have to send in 15 patches with more workaround changes\nthan real fix, then it is premature for us to bear the cost to have\nworkaround for the tool.\n\nThere are folks who use our codebase as a suitably-sized guinea pig\nto improve their tool, and we should not make it harder for them to\ndo so, but our priority is to improve the product of this project.\n\nCome to think of it, adding unnecessary workarounds is a hostile act\nto those who are trying to improve -fanalyzer, I guess, too. They\nmay want to fix problems in their tool, but workarounds hide them.\n\n"},{"id":"456730","messageId":"220606.864k0xwuyl.gmgdl@evledraar.gmail.com","threadId":"57952","inReplyTo":"xmqq7d5tag3s.fsf@gitster.g","subject":"Re: [RFC PATCH 03/15] reftable: don't memset() a NULL from failed malloc()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-06T17:38:07Z","receivedAt":"2022-06-06T17:41:33Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Jun 06 2022, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n> [...]\n>> The question shouldn't be whether those things in particular were worth\n>> the effort, but whether the added safety of getting the new diagnostic\n>> going forward is worth the one-time cost.\n>\n> Workarounds for false positives are hardly one-time cost. They have\n> to stay with us until the -fanalyzer gets corrected, somebody needs\n> to remember to occasionally check if that happened, and revert the\n> workaround to bring the code into their more natural form. What has\n> been good and readable for human programmers for a long time does\n> not need to be touched just to work around a false positive bug in a\n> new tool.\n\nYes, but I think in this case most of these are either 100% legitimate\nissues, or things where we'd like to e.g. add a BUG() assertion anyway,\ne.g. in the diff parsing case.\n\n>> I find the warning output from -fanalyzer to be *really useful*.\n>\n> I do not mind if you add -fanalyzer during your build via your own\n> config.mak file, and you would help them improve the analyzer by\n> reporting false positive bugs while finding possible bugs in the\n> code we have (like you did in a few patches in this series) and the\n> code you are writing. You are capable enough to keep your own set\n> of patches to work around their false positive bugs locally.\n\nOf course.\n\n> But if you have to send in 15 patches with more workaround changes\n> than real fix, then it is premature for us to bear the cost to have\n> workaround for the tool.\n\nThe idea here was to send an RFC showing what it would take to get it\ninto a state where it would be more useful to others.\n\nI.e. I've found it useful to run with it in my own builds and see if\nanything crops up that's not on the whitelist, I was aiming to give that\nto others with an easily tweakable knob.\n\n> There are folks who use our codebase as a suitably-sized guinea pig\n> to improve their tool, and we should not make it harder for them to\n> do so, but our priority is to improve the product of this project.\n\nThose people can still use CFLAGS=-fanalyzer\n\n> Come to think of it, adding unnecessary workarounds is a hostile act\n> to those who are trying to improve -fanalyzer, I guess, too. They\n> may want to fix problems in their tool, but workarounds hide them.\n\nI'm not proposing that we do anything we wouldn't otherwise do to\nappease -fanalyzer, but that we:\n\n 1. Fix things it alerts on because we'd find it worthwhile anyway\n 2. For the rest, have a macro to appease it.\n\nThat macro being similar to e.g. UNLEAK() in that we'd opt-in enable it\nif you enabled certain flags, but otherwise we'd ignore it.\n"},{"id":"456732","messageId":"xmqqh74x8z66.fsf@gitster.g","threadId":"57952","inReplyTo":"220606.864k0xwuyl.gmgdl@evledraar.gmail.com","subject":"Re: [RFC PATCH 03/15] reftable: don't memset() a NULL from failed malloc()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-06T17:44:17Z","receivedAt":"2022-06-06T17:44:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Yes, but I think in this case most of these are either 100% legitimate\n> issues, or things where we'd like to e.g. add a BUG() assertion anyway,\n> e.g. in the diff parsing case.\n\nWhere does that \"BUG() assertion anyway\" comes from? What are we\nprotecting against?\n\nI do not think such a BUG() added only for placating -fanalyzer\nfalls into \"legitimate\" category at all.\n\n"},{"id":"456735","messageId":"220606.86v8tdvg0f.gmgdl@evledraar.gmail.com","threadId":"57952","inReplyTo":"xmqqh74x8z66.fsf@gitster.g","subject":"Re: [RFC PATCH 03/15] reftable: don't memset() a NULL from failed malloc()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-06T17:46:44Z","receivedAt":"2022-06-06T17:49:45Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Jun 06 2022, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> Yes, but I think in this case most of these are either 100% legitimate\n>> issues, or things where we'd like to e.g. add a BUG() assertion anyway,\n>> e.g. in the diff parsing case.\n>\n> Where does that \"BUG() assertion anyway\" comes from? What are we\n> protecting against?\n>\n> I do not think such a BUG() added only for placating -fanalyzer\n> falls into \"legitimate\" category at all.\n\nNo, but e.g. in 09/15[1] we *would* have a segfault or NULL dereference if\nthat function was fed input that it wasn't expecting, which is output\nfrom another git command (or the same output created in-core).\n\nIn that case we can reason about it being impossible in the current\nstate, but annotating that with the equivalent of a BUG(\"unreachable\")\nseems prudent, i.e. it helps human readers to understand the code,\nwhatever -fanalyzer says.\n\nDitto the pack-related code in [2], where it's noting an aspect I'd\nnoted in an earlier code review (before I'd ever used\n-fanalyzer). I.e. the combination of certain flags could land us in a\nNULL dereference, even if we can carefully reason that we won't combine\nthe two having the code be less easily misunderstood seems better.\n\n1. https://lore.kernel.org/git/RFC-patch-09.15-de0f7722608-20220603T183608Z-avarab@gmail.com/\n2. https://lore.kernel.org/git/RFC-patch-13.15-63eeb66185a-20220603T183608Z-avarab@gmail.com/\n"},{"id":"456788","messageId":"cover-0.3-00000000000-20220607T154520Z-avarab@gmail.com","threadId":"57952","inReplyTo":"RFC-cover-00.15-00000000000-20220603T183608Z-avarab@gmail.com","subject":"[PATCH 0/3] remote API: fix -fanalyzer-spotted freeing issue","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-07T15:50:02Z","receivedAt":"2022-06-07T15:51:11Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This series is spun off from the recent RFC -fanalyzer series[1] and\nfixes a clear bug in remote_state_clear().\n\nWhile doing so remove the underlying source of the landmines in the\nAPI and use free() rather than FREE_AND_NULL() for structs that aren't\nbeing re-used.\n\nThen have the API free() the structure itself, rather than have\n*_new() allocate it, but the caller being responsible for calling both\nremote_state_clear() and free().\n\n1. https://lore.kernel.org/git/RFC-cover-00.15-00000000000-20220603T183608Z-avarab@gmail.com/\n\nÆvar Arnfjörð Bjarmason (3):\n remote.c: remove braces from one-statement \"for\"-loops\n remote.c: don't dereference NULL in freeing loop\n remote API: don't buggily FREE_AND_NULL(), free() instead\n\n remote.c | 23 ++++++++++-------------\n remote.h | 10 +++++++++-\n repository.c | 2 +-\n 3 files changed, 20 insertions(+), 15 deletions(-)\n\nRange-diff:\n 1: b3a678d934a ! 1: 1879ed2826e remote.c: don't dereference NULL in freeing loop\n @@ Metadata\n Author: Ævar Arnfjörð Bjarmason \n \n ## Commit message ##\n - remote.c: don't dereference NULL in freeing loop\n + remote.c: remove braces from one-statement \"for\"-loops\n \n - Fix a bug in fd3cb0501e1 (remote: move static variables into\n - per-repository struct, 2021-11-17) where we'd free(remote->pushurl[i])\n - after having NULL'd out remote->pushurl. itself.\n -\n - While we're at it let's get rid of the redundant braces per the\n - CodingGuidelines, which also serves to show in the diff context that\n - we were doing a FREE_AND_NULL(remote->pushurl) afterwards too, let's\n - keep that one.\n + Remove braces that don't follow the CodingGuidelines from code added\n + in fd3cb0501e1 (remote: move static variables into per-repository\n + struct, 2021-11-17). A subsequent commit will edit code adjacent to\n + this.\n \n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n @@ remote.c: static void remote_clear(struct remote *remote)\n +\tfor (i = 0; i < remote->url_nr; i++)\n \t\tfree((char *)remote->url[i]);\n -\t}\n --\tFREE_AND_NULL(remote->pushurl);\n --\n + \tFREE_AND_NULL(remote->pushurl);\n + \n -\tfor (i = 0; i < remote->pushurl_nr; i++) {\n +\tfor (i = 0; i < remote->pushurl_nr; i++)\n \t\tfree((char *)remote->pushurl[i]);\n @@ remote.c: static void remote_clear(struct remote *remote)\n \tFREE_AND_NULL(remote->pushurl);\n \tfree((char *)remote->receivepack);\n \tfree((char *)remote->uploadpack);\n +@@ remote.c: void remote_state_clear(struct remote_state *remote_state)\n + {\n + \tint i;\n + \n +-\tfor (i = 0; i < remote_state->remotes_nr; i++) {\n ++\tfor (i = 0; i < remote_state->remotes_nr; i++)\n + \t\tremote_clear(remote_state->remotes[i]);\n +-\t}\n + \tFREE_AND_NULL(remote_state->remotes);\n + \tremote_state->remotes_alloc = 0;\n + \tremote_state->remotes_nr = 0;\n 2: 4a055969ea5 < -: ----------- pull.c: don't feed NULL to strcmp() on get_rebase_fork_point() path\n 3: 0b570d112fc < -: ----------- reftable: don't memset() a NULL from failed malloc()\n 4: 3a287c19d7e < -: ----------- diff-lib.c: don't dereference NULL in oneway_diff()\n 5: 46e0c307941 < -: ----------- refs/packed-backend.c: add a BUG() if iter is NULL\n 6: 2d04035d7aa < -: ----------- ref-filter.c: BUG() out on show_ref() with NULL refname\n 7: cf1a5f3ed0f < -: ----------- strbuf.c: placate -fanalyzer in strbuf_grow()\n 8: 2c4b7832144 < -: ----------- strbuf.c: use st_add3(), not unsigned_add_overflows()\n 9: de0f7722608 < -: ----------- add-patch: assert parse_diff() expectations with BUG()\n10: b50558d3b24 < -: ----------- reftable: don't have reader_get_block() confuse -fanalyzer\n11: 66518467e1d < -: ----------- blame.c: clarify the state of \"final_commit\" for -fanalyzer\n12: 9f0f515ed3a < -: ----------- pack.h: wrap write_*file*() functions\n13: 63eeb66185a < -: ----------- pack-write API: pass down \"verify\" not arbitrary flags\n14: 9cf550688d4 < -: ----------- config.mak.dev: add a DEVOPTS=analyzer mode to use GCC's -fanalyzer\n15: 16bd2270b4c < -: ----------- config.mak.dev: add and use ASSERT_FOR_FANALYZER() macro\n -: ----------- > 2: 0e258c230f6 remote.c: don't dereference NULL in freeing loop\n -: ----------- > 3: 062fb3f454e remote API: don't buggily FREE_AND_NULL(), free() instead\n-- \n2.36.1.1178.g0c3594a0ba5\n\n"},{"id":"456789","messageId":"patch-1.3-1879ed2826e-20220607T154520Z-avarab@gmail.com","threadId":"57952","inReplyTo":"cover-0.3-00000000000-20220607T154520Z-avarab@gmail.com","subject":"[PATCH 1/3] remote.c: remove braces from one-statement \"for\"-loops","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-07T15:50:03Z","receivedAt":"2022-06-07T15:51:14Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Remove braces that don't follow the CodingGuidelines from code added\nin fd3cb0501e1 (remote: move static variables into per-repository\nstruct, 2021-11-17). A subsequent commit will edit code adjacent to\nthis.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n remote.c | 9 +++------\n 1 file changed, 3 insertions(+), 6 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex e98148ac227..3e75db7bb4f 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -145,14 +145,12 @@ static void remote_clear(struct remote *remote)\n \tfree((char *)remote->name);\n \tfree((char *)remote->foreign_vcs);\n \n-\tfor (i = 0; i < remote->url_nr; i++) {\n+\tfor (i = 0; i < remote->url_nr; i++)\n \t\tfree((char *)remote->url[i]);\n-\t}\n \tFREE_AND_NULL(remote->pushurl);\n \n-\tfor (i = 0; i < remote->pushurl_nr; i++) {\n+\tfor (i = 0; i < remote->pushurl_nr; i++)\n \t\tfree((char *)remote->pushurl[i]);\n-\t}\n \tFREE_AND_NULL(remote->pushurl);\n \tfree((char *)remote->receivepack);\n \tfree((char *)remote->uploadpack);\n@@ -2720,9 +2718,8 @@ void remote_state_clear(struct remote_state *remote_state)\n {\n \tint i;\n \n-\tfor (i = 0; i < remote_state->remotes_nr; i++) {\n+\tfor (i = 0; i < remote_state->remotes_nr; i++)\n \t\tremote_clear(remote_state->remotes[i]);\n-\t}\n \tFREE_AND_NULL(remote_state->remotes);\n \tremote_state->remotes_alloc = 0;\n \tremote_state->remotes_nr = 0;\n-- \n2.36.1.1178.g0c3594a0ba5\n\n"},{"id":"456790","messageId":"patch-2.3-0e258c230f6-20220607T154520Z-avarab@gmail.com","threadId":"57952","inReplyTo":"cover-0.3-00000000000-20220607T154520Z-avarab@gmail.com","subject":"[PATCH 2/3] remote.c: don't dereference NULL in freeing loop","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-07T15:50:04Z","receivedAt":"2022-06-07T15:51:14Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a bug in fd3cb0501e1 (remote: move static variables into\nper-repository struct, 2021-11-17) where we'd free(remote->pushurl[i])\nafter having NULL'd out remote->pushurl. itself. We free\n\"remote->pushurl\" in the next \"for\"-loop, so doing this appears to\nhave been a copy/paste error.\n\nBefore this change GCC 12's -fanalyzer would correctly note that we'd\ndereference NULL in this case, this change fixes that:\n\n\tremote.c: In function ‘remote_clear’:\n\tremote.c:153:17: error: dereference of NULL ‘*remote.pushurl’ [CWE-476] [-Werror=analyzer-null-dereference]\n\t 153 | free((char *)remote->pushurl[i]);\n\t | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n\t [...]\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n remote.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/remote.c b/remote.c\nindex 3e75db7bb4f..0b243b090d9 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -147,7 +147,7 @@ static void remote_clear(struct remote *remote)\n \n \tfor (i = 0; i < remote->url_nr; i++)\n \t\tfree((char *)remote->url[i]);\n-\tFREE_AND_NULL(remote->pushurl);\n+\tFREE_AND_NULL(remote->url);\n \n \tfor (i = 0; i < remote->pushurl_nr; i++)\n \t\tfree((char *)remote->pushurl[i]);\n-- \n2.36.1.1178.g0c3594a0ba5\n\n"},{"id":"456791","messageId":"patch-3.3-062fb3f454e-20220607T154520Z-avarab@gmail.com","threadId":"57952","inReplyTo":"cover-0.3-00000000000-20220607T154520Z-avarab@gmail.com","subject":"[PATCH 3/3] remote API: don't buggily FREE_AND_NULL(), free() instead","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-07T15:50:05Z","receivedAt":"2022-06-07T15:51:17Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the buggy \"remote_clear()\" function to stop pretending to to be\nable to zero out a \"struct remote\". Setting \"url\" and \"pushurl\" to\nNULL results in an invalid state unless the corresponding \"url_nr\" and\n\"pushurl_nr\" are also set to zero.\n\nIn this case however we do not use the \"struct remote\", so the\nFREE_AND_NULL() pattern added in fd3cb0501e1 (remote: move static\nvariables into per-repository struct, 2021-11-17) can be replaced with\nfree().\n\nThe API was also odd in that remote_state_new() would xmalloc() for us,\nbut the user had to free() it themselves, let's instead change the\nbehavior to have the destructor free() what we malloc() in the\nconstructer.\n\nIn this case this appears to have been done for consistency with\nrepo_clear(), let's instead have repo_clear() handle the NULL-ing of\nits \"remote_state\", and not attempt to reset the structure in remote.c\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n remote.c | 14 +++++++-------\n remote.h | 10 +++++++++-\n repository.c | 2 +-\n 3 files changed, 17 insertions(+), 9 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex 0b243b090d9..c6ce04dacb7 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -147,15 +147,15 @@ static void remote_clear(struct remote *remote)\n \n \tfor (i = 0; i < remote->url_nr; i++)\n \t\tfree((char *)remote->url[i]);\n-\tFREE_AND_NULL(remote->url);\n+\tfree(remote->url);\n \n \tfor (i = 0; i < remote->pushurl_nr; i++)\n \t\tfree((char *)remote->pushurl[i]);\n-\tFREE_AND_NULL(remote->pushurl);\n+\tfree(remote->pushurl);\n \tfree((char *)remote->receivepack);\n \tfree((char *)remote->uploadpack);\n-\tFREE_AND_NULL(remote->http_proxy);\n-\tFREE_AND_NULL(remote->http_proxy_authmethod);\n+\tfree(remote->http_proxy);\n+\tfree(remote->http_proxy_authmethod);\n }\n \n static void add_merge(struct branch *branch, const char *name)\n@@ -2720,12 +2720,12 @@ void remote_state_clear(struct remote_state *remote_state)\n \n \tfor (i = 0; i < remote_state->remotes_nr; i++)\n \t\tremote_clear(remote_state->remotes[i]);\n-\tFREE_AND_NULL(remote_state->remotes);\n-\tremote_state->remotes_alloc = 0;\n-\tremote_state->remotes_nr = 0;\n+\tfree(remote_state->remotes);\n \n \thashmap_clear_and_free(&remote_state->remotes_hash, struct remote, ent);\n \thashmap_clear_and_free(&remote_state->branches_hash, struct remote, ent);\n+\n+\tfree(remote_state);\n }\n \n /*\ndiff --git a/remote.h b/remote.h\nindex dd4402436f1..d91b2b29373 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -54,9 +54,17 @@ struct remote_state {\n \tint initialized;\n };\n \n-void remote_state_clear(struct remote_state *remote_state);\n+/**\n+ * xmalloc() a \"struct remote_state\" and initialize it. The resulting\n+ * data should be free'd with remote_state_clear().\n+ */\n struct remote_state *remote_state_new(void);\n \n+/**\n+ * free() the structure returned by remote_state_new().\n+ */\n+void remote_state_clear(struct remote_state *remote_state);\n+\n struct remote {\n \tstruct hashmap_entry ent;\n \ndiff --git a/repository.c b/repository.c\nindex 5d166b692c8..0a6df6937e4 100644\n--- a/repository.c\n+++ b/repository.c\n@@ -292,7 +292,7 @@ void repo_clear(struct repository *repo)\n \n \tif (repo->remote_state) {\n \t\tremote_state_clear(repo->remote_state);\n-\t\tFREE_AND_NULL(repo->remote_state);\n+\t\trepo->remote_state = NULL;\n \t}\n \n \trepo_clear_path_cache(&repo->cached_paths);\n-- \n2.36.1.1178.g0c3594a0ba5\n\n"},{"id":"456795","messageId":"kl6lv8tccspo.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"57952","inReplyTo":"patch-3.3-062fb3f454e-20220607T154520Z-avarab@gmail.com","subject":"Re: [PATCH 3/3] remote API: don't buggily FREE_AND_NULL(), free() instead","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-06-07T17:02:27Z","receivedAt":"2022-06-07T17:02:35Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"\nThanks for sending out this series :) I feel a little guilty leaving\nbehind this much junk. Coincidentally, I was already planning on\nrevisiting my previous work to clean up debt/mistakes, so this was a\ngood lesson in what to look out for.\n\nThe previous patches look good to me, and I have no comments on those.\nI have a style question on this patch (more for my own learning than a\nsuggestion on this patch), but it also looks good to me.\n\nÆvar Arnfjörð Bjarmason writes:\n\n> In this case however we do not use the \"struct remote\", so the\n> FREE_AND_NULL() pattern added in fd3cb0501e1 (remote: move static\n> variables into per-repository struct, 2021-11-17) can be replaced with\n> free().\n\nDid you mean *reuse* the \"struct remote\"?\n\n> The API was also odd in that remote_state_new() would xmalloc() for us,\n> but the user had to free() it themselves, let's instead change the\n> behavior to have the destructor free() what we malloc() in the\n> constructer.\n>\n> In this case this appears to have been done for consistency with\n> repo_clear(), let's instead have repo_clear() handle the NULL-ing of\n> its \"remote_state\", and not attempt to reset the structure in remote.c\n\nYes, this was specifically for consistency reasons. We'll have to read\non to see whether or not this patch remains consistent with\nrepo_clear()...\n\n> static void add_merge(struct branch *branch, const char *name)\n> @@ -2720,12 +2720,12 @@ void remote_state_clear(struct remote_state *remote_state)\n> \n> \tfor (i = 0; i < remote_state->remotes_nr; i++)\n> \t\tremote_clear(remote_state->remotes[i]);\n> -\tFREE_AND_NULL(remote_state->remotes);\n> -\tremote_state->remotes_alloc = 0;\n> -\tremote_state->remotes_nr = 0;\n> +\tfree(remote_state->remotes);\n> \n> \thashmap_clear_and_free(&remote_state->remotes_hash, struct remote, ent);\n> \thashmap_clear_and_free(&remote_state->branches_hash, struct remote, ent);\n> +\n> +\tfree(remote_state);\n> }\n\nNow that remote_state_clear() free()-s the \"struct remote_state\", should\nwe rename it to something like \"remote_state_free()\"? There's some\nprecedent for this in the other repo_clear() 'destructors'...\n\n> diff --git a/remote.h b/remote.h\n> index dd4402436f1..d91b2b29373 100644\n> --- a/remote.h\n> +++ b/remote.h\n> @@ -54,9 +54,17 @@ struct remote_state {\n> \tint initialized;\n> };\n> \n> -void remote_state_clear(struct remote_state *remote_state);\n> +/**\n> + * xmalloc() a \"struct remote_state\" and initialize it. The resulting\n> + * data should be free'd with remote_state_clear().\n> + */\n> struct remote_state *remote_state_new(void);\n> \n> +/**\n> + * free() the structure returned by remote_state_new().\n> + */\n> +void remote_state_clear(struct remote_state *remote_state);\n> +\n> struct remote {\n> \tstruct hashmap_entry ent;\n> \n> diff --git a/repository.c b/repository.c\n> index 5d166b692c8..0a6df6937e4 100644\n> --- a/repository.c\n> +++ b/repository.c\n> @@ -292,7 +292,7 @@ void repo_clear(struct repository *repo)\n> \n> \tif (repo->remote_state) {\n> \t\tremote_state_clear(repo->remote_state);\n> -\t\tFREE_AND_NULL(repo->remote_state);\n> +\t\trepo->remote_state = NULL;\n> \t}\n> \n> \trepo_clear_path_cache(&repo->cached_paths);\n\nI suppose the question of whether or not to free() in the 'destructor'\ndepends on whether we expect the struct to be reusable? I don't expect\nthat \"struct remote_state\" needs to be reused, so free()-ing it is ok to\nme.\n\nThe API is not _that_ odd though ;) As you noted, my initial use of\nFREE_AND_NULL() is for consistency reasons with the rest of\nrepo_clear(), which looks like this:\n\n\tif (repo->config) {\n\t\tgit_configset_clear(repo->config);\n\t\tFREE_AND_NULL(repo->config);\n\t}\n\n\tif (repo->submodule_cache) {\n\t\tsubmodule_cache_free(repo->submodule_cache);\n\t\trepo->submodule_cache = NULL;\n\t}\n\n\tif (repo->index) {\n\t\tdiscard_index(repo->index);\n\t\tif (repo->index != &the_index)\n\t\t\tFREE_AND_NULL(repo->index);\n\t}\n\n\tif (repo->promisor_remote_config) {\n\t\tpromisor_remote_clear(repo->promisor_remote_config);\n\t\tFREE_AND_NULL(repo->promisor_remote_config);\n\t}\n\n\tif (repo->remote_state) {\n\t\tremote_state_clear(repo->remote_state);\n -\tFREE_AND_NULL(repo->remote_state);\n +\trepo->remote_state = NULL;\n\t}\n\npromisor_remote_clear(), discard_index(), and git_configset_clear()\ndon't free() the struct, so it makes sense for them to use\nFREE_AND_NULL(). AFAICT, these structs are meant to be reused, so it\nmakes sense that we \"clear\" it without freeing the struct pointer\nitself.\n\nOn the other hand, submodule_cache_free() _does_ free() the struct, and\nso we just use \"= NULL\". I noticed that this uses the verb \"free\", and\nnot \"clear\".\n\nSo now that remote_state_clear() *does* free() the struct, it is\nperfectly fine to use \"= NULL\" here as well, though it uses the verb\n\"clear\".\n\nI'm not sure if we have a style around clear/free. Feel free to ignore\nif there isn't one.\n"},{"id":"456799","messageId":"xmqqpmjkxu9s.fsf@gitster.g","threadId":"57952","inReplyTo":"patch-2.3-0e258c230f6-20220607T154520Z-avarab@gmail.com","subject":"Re: [PATCH 2/3] remote.c: don't dereference NULL in freeing loop","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-07T17:23:11Z","receivedAt":"2022-06-07T17:27:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Fix a bug in fd3cb0501e1 (remote: move static variables into\n> per-repository struct, 2021-11-17) where we'd free(remote->pushurl[i])\n> after having NULL'd out remote->pushurl. itself. We free\n\nWow, that's a bad one. Why didn't anybody notice this at runtime, I\nhave to wonder.\n\n> \tfor (i = 0; i < remote->url_nr; i++)\n> \t\tfree((char *)remote->url[i]);\n> -\tFREE_AND_NULL(remote->pushurl);\n> +\tFREE_AND_NULL(remote->url);\n> \n> \tfor (i = 0; i < remote->pushurl_nr; i++)\n> \t\tfree((char *)remote->pushurl[i]);\n> \tFREE_AND_NULL(remote->pushurl);\n\nThanks.\n"},{"id":"456800","messageId":"xmqqee00xtyw.fsf@gitster.g","threadId":"57952","inReplyTo":"patch-3.3-062fb3f454e-20220607T154520Z-avarab@gmail.com","subject":"Re: [PATCH 3/3] remote API: don't buggily FREE_AND_NULL(), free() instead","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-07T17:29:43Z","receivedAt":"2022-06-07T17:32:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Change the buggy \"remote_clear()\" function to stop pretending to to be\n\n\"buggy\" -> \"\"; \"to to\" -> \"to\".\n\nOver-using FREE_AND_NULL() is indeed irritating, but as long as we\nare not reusing the struct whose member are getting freed, there is\nno \"bug\" per-se, no?\n\n> able to zero out a \"struct remote\". Setting \"url\" and \"pushurl\" to\n> NULL results in an invalid state unless the corresponding \"url_nr\" and\n> \"pushurl_nr\" are also set to zero.\n>\n> In this case however we do not use the \"struct remote\", so the\n\n\"use\" -> \"reuse\", with comma around \"however\" on both sides, probably.\n\n> FREE_AND_NULL() pattern added in fd3cb0501e1 (remote: move static\n> variables into per-repository struct, 2021-11-17) can be replaced with\n> free().\n\nYay.\n\n> The API was also odd in that remote_state_new() would xmalloc() for us,\n> but the user had to free() it themselves, let's instead change the\n> behavior to have the destructor free() what we malloc() in the\n> constructer.\n\nI am not sure if remote_clear() that does not free the remote goes\nwell with remote_state_clear() that does free remote_state. As long\nas it is clear whose responsibility to release resources is, we can\ngo either way, but helper functions with similar names having quite\ndifferent resource maintainance semantics looks like a potential\nsource of confusion to me.\n\n> In this case this appears to have been done for consistency with\n> repo_clear(), let's instead have repo_clear() handle the NULL-ing of\n> its \"remote_state\", and not attempt to reset the structure in remote.c\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> remote.c | 14 +++++++-------\n> remote.h | 10 +++++++++-\n> repository.c | 2 +-\n> 3 files changed, 17 insertions(+), 9 deletions(-)\n>\n> diff --git a/remote.c b/remote.c\n> index 0b243b090d9..c6ce04dacb7 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -147,15 +147,15 @@ static void remote_clear(struct remote *remote)\n> \n> \tfor (i = 0; i < remote->url_nr; i++)\n> \t\tfree((char *)remote->url[i]);\n> -\tFREE_AND_NULL(remote->url);\n> +\tfree(remote->url);\n> \n> \tfor (i = 0; i < remote->pushurl_nr; i++)\n> \t\tfree((char *)remote->pushurl[i]);\n> -\tFREE_AND_NULL(remote->pushurl);\n> +\tfree(remote->pushurl);\n> \tfree((char *)remote->receivepack);\n> \tfree((char *)remote->uploadpack);\n> -\tFREE_AND_NULL(remote->http_proxy);\n> -\tFREE_AND_NULL(remote->http_proxy_authmethod);\n> +\tfree(remote->http_proxy);\n> +\tfree(remote->http_proxy_authmethod);\n> }\n> \n> static void add_merge(struct branch *branch, const char *name)\n> @@ -2720,12 +2720,12 @@ void remote_state_clear(struct remote_state *remote_state)\n> \n> \tfor (i = 0; i < remote_state->remotes_nr; i++)\n> \t\tremote_clear(remote_state->remotes[i]);\n> -\tFREE_AND_NULL(remote_state->remotes);\n> -\tremote_state->remotes_alloc = 0;\n> -\tremote_state->remotes_nr = 0;\n> +\tfree(remote_state->remotes);\n> \n> \thashmap_clear_and_free(&remote_state->remotes_hash, struct remote, ent);\n> \thashmap_clear_and_free(&remote_state->branches_hash, struct remote, ent);\n> +\n> +\tfree(remote_state);\n> }\n> \n> /*\n> diff --git a/remote.h b/remote.h\n> index dd4402436f1..d91b2b29373 100644\n> --- a/remote.h\n> +++ b/remote.h\n> @@ -54,9 +54,17 @@ struct remote_state {\n> \tint initialized;\n> };\n> \n> -void remote_state_clear(struct remote_state *remote_state);\n> +/**\n> + * xmalloc() a \"struct remote_state\" and initialize it. The resulting\n> + * data should be free'd with remote_state_clear().\n> + */\n> struct remote_state *remote_state_new(void);\n> \n> +/**\n> + * free() the structure returned by remote_state_new().\n> + */\n> +void remote_state_clear(struct remote_state *remote_state);\n> +\n> struct remote {\n> \tstruct hashmap_entry ent;\n> \n> diff --git a/repository.c b/repository.c\n> index 5d166b692c8..0a6df6937e4 100644\n> --- a/repository.c\n> +++ b/repository.c\n> @@ -292,7 +292,7 @@ void repo_clear(struct repository *repo)\n> \n> \tif (repo->remote_state) {\n> \t\tremote_state_clear(repo->remote_state);\n> -\t\tFREE_AND_NULL(repo->remote_state);\n> +\t\trepo->remote_state = NULL;\n> \t}\n> \n> \trepo_clear_path_cache(&repo->cached_paths);\n"},{"id":"456801","messageId":"xmqq7d5sxttm.fsf@gitster.g","threadId":"57952","inReplyTo":"cover-0.3-00000000000-20220607T154520Z-avarab@gmail.com","subject":"Re: [PATCH 0/3] remote API: fix -fanalyzer-spotted freeing issue","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-07T17:32:53Z","receivedAt":"2022-06-07T17:52:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> This series is spun off from the recent RFC -fanalyzer series[1] and\n> fixes a clear bug in remote_state_clear().\n\n[1/3] is a pretty-much \"Meh\" (read: if it is already in the code, it\nis not worth a patch noise to go and fix it, even though we would\navoid newly introducing the construct in new code), and some part of\n[3/3] I am not so sure about (read: it is a mixed bag of changes),\nbut [2/3] is a bug that I find surprising that nobody noticed at\nruntime.\n\nThanks. Will queue.\n"},{"id":"456806","messageId":"xmqqsfogwdjq.fsf@gitster.g","threadId":"57952","inReplyTo":"kl6lv8tccspo.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH 3/3] remote API: don't buggily FREE_AND_NULL(), free() instead","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-07T18:09:45Z","receivedAt":"2022-06-07T19:28:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Glen Choo writes:\n\n> I suppose the question of whether or not to free() in the 'destructor'\n> depends on whether we expect the struct to be reusable? I don't expect\n> that \"struct remote_state\" needs to be reused, so free()-ing it is ok to\n> me.\n>\n> The API is not _that_ odd though ;) As you noted, my initial use of\n> FREE_AND_NULL() is for consistency reasons with the rest of\n> repo_clear(), which looks like this:\n>\n> \tif (repo->config) {\n> \t\tgit_configset_clear(repo->config);\n> \t\tFREE_AND_NULL(repo->config);\n\nSo git_configset_clear() does clear but does not free.\n\n> \t}\n>\n> \tif (repo->submodule_cache) {\n> \t\tsubmodule_cache_free(repo->submodule_cache);\n\nsubmodule_cache_free() does (probably clear and) free.\n\n> \t\trepo->submodule_cache = NULL;\n> \t}\n>\n> \tif (repo->index) {\n> \t\tdiscard_index(repo->index);\n\ndiscard_index() does not free.\n\n> \t\tif (repo->index != &the_index)\n> \t\t\tFREE_AND_NULL(repo->index);\n> \t}\n>\n> \tif (repo->promisor_remote_config) {\n> \t\tpromisor_remote_clear(repo->promisor_remote_config);\n\npromisor_remote_clear() does not free.\n\n> \t\tFREE_AND_NULL(repo->promisor_remote_config);\n> \t}\n>\n> \tif (repo->remote_state) {\n> \t\tremote_state_clear(repo->remote_state);\n> -\tFREE_AND_NULL(repo->remote_state);\n> +\trepo->remote_state = NULL;\n> \t}\n>\n> promisor_remote_clear(), discard_index(), and git_configset_clear()\n> don't free() the struct, so it makes sense for them to use\n> FREE_AND_NULL(). AFAICT, these structs are meant to be reused, so it\n> makes sense that we \"clear\" it without freeing the struct pointer\n> itself.\n>\n> On the other hand, submodule_cache_free() _does_ free() the struct, and\n> so we just use \"= NULL\". I noticed that this uses the verb \"free\", and\n> not \"clear\".\n>\n> So now that remote_state_clear() *does* free() the struct, it is\n> perfectly fine to use \"= NULL\" here as well, though it uses the verb\n> \"clear\".\n>\n> I'm not sure if we have a style around clear/free. Feel free to ignore\n> if there isn't one.\n\nIt does bother me. Changing _clear() that did not free the\ncontainer resource to free it, without changing the name to free or\nrelease or whatever, smells like leaving a source of confusion for\nfuture developers.\n\n\n"}]}