{"thread":{"id":"54713","subject":"[PATCH 0/2] midx: prevent against racily disappearing packs","startedAt":"2020-11-25T17:17:28Z","lastAt":"2020-11-26T01:27:46Z","messageCount":9,"participants":["Taylor Blau","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"410784","messageId":"cover.1606324509.git.me@ttaylorr.com","threadId":"54713","inReplyTo":null,"subject":"[PATCH 0/2] midx: prevent against racily disappearing packs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-11-25T17:17:22Z","receivedAt":"2020-11-25T17:17:28Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Here are a couple of patches that Peff and I wrote after noticing a\nproblem where racily disappearing .idx files can cause a segfault. While\ninvestigating, we fixed a related issue which is described and fixed in\nthe second patch.\n\n(I'm a little behind in responding to review on the reachability\ngeneration improvements patches, but had these in my backlog and they\nseemed relatively easy to send and remove from my TODO list. So, here\nthey are :-).)\n\nThanks,\nTaylor\n\nTaylor Blau (2):\n  packfile.c: protect against disappearing indexes\n  midx.c: protect against disappearing packs\n\n midx.c                      |  2 +-\n packfile.c                  | 19 ++-----------------\n t/t5319-multi-pack-index.sh | 30 ++++++++++++++++++++++++++++--\n 3 files changed, 31 insertions(+), 20 deletions(-)\n\n--\n2.29.2.368.ge1806d1bdc\n"},{"id":"410785","messageId":"cee25cc1ca5bc3371e32099980f28b623c1349d5.1606324509.git.me@ttaylorr.com","threadId":"54713","inReplyTo":"cover.1606324509.git.me@ttaylorr.com","subject":"[PATCH 1/2] packfile.c: protect against disappearing indexes","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-11-25T17:17:28Z","receivedAt":"2020-11-25T17:17:34Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In 17c35c8969 (packfile: skip loading index if in multi-pack-index,\n2018-07-12) we stopped loading the .idx file for packs that are\ncontained within a multi-pack index.\n\nThis saves us the effort of loading an .idx and doing some lightweight\nvalidity checks by way of 'packfile.c:load_idx()', but introduces a race\nbetween processes that need to load the index (e.g., to generate a\nreverse index) and processes that can delete the index.\n\nFor example, running the following in your shell:\n\n    $ git init repo && cd repo\n    $ git commit --allow-empty -m 'base'\n    $ git repack -ad && git multi-pack-index write\n\nfollowed by:\n\n    $ rm -f .git/objects/pack/pack-*.idx\n    $ git rev-parse HEAD | git cat-file --batch-check='%(objectsize:disk)'\n\nwill result in a segfault prior to this patch. What's happening here is\nthat we notice that the pack is in the multi-pack index, and so don't\ncheck that it still has a .idx. When we then try and load that index to\ngenerate a reverse index, we don't have it, so the call to\n'find_pack_revindex()' in 'packfile.c:packed_object_info()' returns\nNULL, and then dereferencing it causes a segfault.\n\nOf course, we don't ever expect someone to remove the index file by\nhand, or to be in a state where we never wrote it to begin with (yet\nfind that pack in the multi-pack-index). But, this can happen in a\ntiming race with 'git repack -ad', which removes all existing packs\nafter writing a new pack containing all of their objects.\n\nAvoid this by reverting the hunk of 17c35c8969 which stops loading the\nindex when the pack is contained in a MIDX. This makes the latter half\nof 17c35c8969 useless, since we'll always have a non-NULL\n'p->index_data', in which case that if statement isn't guarding\nanything.\n\nThese two together effectively revert 17c35c8969, and avoid the race\nexplained above.\n\nCo-authored-by: Jeff King <peff@peff.net>\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n packfile.c                  | 19 ++-----------------\n t/t5319-multi-pack-index.sh | 24 ++++++++++++++++++++++--\n 2 files changed, 24 insertions(+), 19 deletions(-)\n\ndiff --git a/packfile.c b/packfile.c\nindex 0929ebe4fc..8d7f37a5f6 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -514,19 +514,8 @@ static int open_packed_git_1(struct packed_git *p)\n \tssize_t read_result;\n \tconst unsigned hashsz = the_hash_algo->rawsz;\n \n-\tif (!p->index_data) {\n-\t\tstruct multi_pack_index *m;\n-\t\tconst char *pack_name = pack_basename(p);\n-\n-\t\tfor (m = the_repository->objects->multi_pack_index;\n-\t\t     m; m = m->next) {\n-\t\t\tif (midx_contains_pack(m, pack_name))\n-\t\t\t\tbreak;\n-\t\t}\n-\n-\t\tif (!m && open_pack_index(p))\n-\t\t\treturn error(\"packfile %s index unavailable\", p->pack_name);\n-\t}\n+\tif (open_pack_index(p))\n+\t\treturn error(\"packfile %s index unavailable\", p->pack_name);\n \n \tif (!pack_max_fds) {\n \t\tunsigned int max_fds = get_max_fd_limit();\n@@ -567,10 +556,6 @@ static int open_packed_git_1(struct packed_git *p)\n \t\t\t\" supported (try upgrading GIT to a newer version)\",\n \t\t\tp->pack_name, ntohl(hdr.hdr_version));\n \n-\t/* Skip index checking if in multi-pack-index */\n-\tif (!p->index_data)\n-\t\treturn 0;\n-\n \t/* Verify the pack matches its index. */\n \tif (p->num_objects != ntohl(hdr.hdr_entries))\n \t\treturn error(\"packfile %s claims to have %\"PRIu32\" objects\"\ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex ace469c95c..d4607daec1 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -138,7 +138,7 @@ test_expect_success 'write midx with one v2 pack' '\n \n compare_results_with_midx \"one v2 pack\"\n \n-test_expect_success 'corrupt idx not opened' '\n+test_expect_success 'corrupt idx reports errors' '\n \tidx=$(test-tool read-midx $objdir | grep \"\\.idx\\$\") &&\n \tmv $objdir/pack/$idx backup-$idx &&\n \ttest_when_finished \"mv backup-\\$idx \\$objdir/pack/\\$idx\" &&\n@@ -149,7 +149,7 @@ test_expect_success 'corrupt idx not opened' '\n \ttest_copy_bytes 1064 <backup-$idx >$objdir/pack/$idx &&\n \n \tgit -c core.multiPackIndex=true rev-list --objects --all 2>err &&\n-\ttest_must_be_empty err\n+\tgrep \"index unavailable\" err\n '\n \n test_expect_success 'add more objects' '\n@@ -755,4 +755,24 @@ test_expect_success 'repack --batch-size=<large> repacks everything' '\n \t)\n '\n \n+test_expect_success 'load reverse index when missing .idx' '\n+\tgit init repo &&\n+\ttest_when_finished \"rm -fr repo\" &&\n+\t(\n+\t\tcd repo &&\n+\n+\t\tgit config core.multiPackIndex true &&\n+\n+\t\ttest_commit base &&\n+\t\tgit repack -ad &&\n+\t\tgit multi-pack-index write &&\n+\n+\t\tgit rev-parse HEAD >tip &&\n+\t\tidx=$(ls .git/objects/pack/pack-*.idx) &&\n+\n+\t\tmv $idx $idx.bak &&\n+\t\tgit cat-file --batch-check=\"%(objectsize:disk)\" <tip\n+\t)\n+'\n+\n test_done\n-- \n2.29.2.368.ge1806d1bdc\n\n"},{"id":"410786","messageId":"e1806d1bdc0da8061c78608e56138424ad24bed0.1606324509.git.me@ttaylorr.com","threadId":"54713","inReplyTo":"cover.1606324509.git.me@ttaylorr.com","subject":"[PATCH 2/2] midx.c: protect against disappearing packs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-11-25T17:17:33Z","receivedAt":"2020-11-25T17:17:38Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When a packed object is stored in a multi-pack index, but that pack has\nracily gone away, the MIDX code simply calls die(), when it could be\nreturning an error to the caller, which would in turn lead to\nre-scanning the pack directory.\n\nA pack can racily disappear, for example, due to a simultaneous 'git\nrepack -ad',\n\nYou can also reproduce this with two terminals, where one is running:\n\n    git init\n    while true; do\n      git commit -q --allow-empty -m foo\n      git repack -ad\n      git multi-pack-index write\n    done\n\n(in effect, constantly writing new MIDXs), and the other is running:\n\n    obj=$(git rev-parse HEAD)\n    while true; do\n      echo $obj | git cat-file --batch-check='%(objectsize:disk)' || break\n    done\n\nThat will sometimes hit the error preparing packfile from\nmulti-pack-index message, which this patch fixes.\n\nRight now, that path to discovering a missing pack looks something like\n'find_pack_entry()' calling 'fill_midx_entry()' and eventually making\nits way to call 'nth_midxed_pack_entry()'.\n\n'nth_midxed_pack_entry()' already checks 'is_pack_valid()' and\npropagates an error if the pack is invalid. So, this works if the pack\nhas gone away between calling 'prepare_midx_pack()' and before calling\n'is_pack_valid()', but not if it disappears before then.\n\nCatch the case where the pack has already disappeared before\n'prepare_midx_pack()' by returning an error in that case, too.\n\nCo-authored-by: Jeff King <peff@peff.net>\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n midx.c                      | 2 +-\n t/t5319-multi-pack-index.sh | 8 +++++++-\n 2 files changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex d233b54ac7..1d2179a61f 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -298,7 +298,7 @@ static int nth_midxed_pack_entry(struct repository *r,\n \tpack_int_id = nth_midxed_pack_int_id(m, pos);\n \n \tif (prepare_midx_pack(r, m, pack_int_id))\n-\t\tdie(_(\"error preparing packfile from multi-pack-index\"));\n+\t\treturn 0;\n \tp = m->packs[pack_int_id];\n \n \t/*\ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex d4607daec1..297de502a9 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -755,7 +755,7 @@ test_expect_success 'repack --batch-size=<large> repacks everything' '\n \t)\n '\n \n-test_expect_success 'load reverse index when missing .idx' '\n+test_expect_success 'load reverse index when missing .idx, .pack' '\n \tgit init repo &&\n \ttest_when_finished \"rm -fr repo\" &&\n \t(\n@@ -768,9 +768,15 @@ test_expect_success 'load reverse index when missing .idx' '\n \t\tgit multi-pack-index write &&\n \n \t\tgit rev-parse HEAD >tip &&\n+\t\tpack=$(ls .git/objects/pack/pack-*.pack) &&\n \t\tidx=$(ls .git/objects/pack/pack-*.idx) &&\n \n \t\tmv $idx $idx.bak &&\n+\t\tgit cat-file --batch-check=\"%(objectsize:disk)\" <tip &&\n+\n+\t\tmv $idx.bak $idx &&\n+\n+\t\tmv $pack $pack.bak &&\n \t\tgit cat-file --batch-check=\"%(objectsize:disk)\" <tip\n \t)\n '\n-- \n2.29.2.368.ge1806d1bdc\n"},{"id":"410802","messageId":"xmqqblfldts4.fsf@gitster.c.googlers.com","threadId":"54713","inReplyTo":"cover.1606324509.git.me@ttaylorr.com","subject":"Re: [PATCH 0/2] midx: prevent against racily disappearing packs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-25T21:13:31Z","receivedAt":"2020-11-25T21:13:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> Here are a couple of patches that Peff and I wrote after noticing a\n> problem where racily disappearing .idx files can cause a segfault. While\n> investigating, we fixed a related issue which is described and fixed in\n> the second patch.\n\nIn the cover letter it won't affect the end result, but when talking\nabout \"race\", it always is a good idea to explicitly mention both\nsides of the race.  It is clear what one side is in the above\n(i.e. somebody who removes .idx file that is still in use), but it\nis not so clear who gets hit and segfaults.\n\nI am guessing that the other party is the user of .pack file(s)\nbypassing the corresponding .idx file(s) because the necessary data\nis mostly in .midx?\n\nThanks.\n"},{"id":"410803","messageId":"xmqq7dq9dtpg.fsf@gitster.c.googlers.com","threadId":"54713","inReplyTo":"cee25cc1ca5bc3371e32099980f28b623c1349d5.1606324509.git.me@ttaylorr.com","subject":"Re: [PATCH 1/2] packfile.c: protect against disappearing indexes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-25T21:15:07Z","receivedAt":"2020-11-25T21:15:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> In 17c35c8969 (packfile: skip loading index if in multi-pack-index,\n> 2018-07-12) we stopped loading the .idx file for packs that are\n> contained within a multi-pack index.\n>\n> This saves us the effort of loading an .idx and doing some lightweight\n> validity checks by way of 'packfile.c:load_idx()', but introduces a race\n> between processes that need to load the index (e.g., to generate a\n> reverse index) and processes that can delete the index.\n\nAh, OK.  And demonstration using %(objectsize:disk) makes perfect\nsense given the above explanation.\n\n> These two together effectively revert 17c35c8969, and avoid the race\n> explained above.\n>\n> Co-authored-by: Jeff King <peff@peff.net>\n> Signed-off-by: Taylor Blau <me@ttaylorr.com>\n> ---\n>  packfile.c                  | 19 ++-----------------\n>  t/t5319-multi-pack-index.sh | 24 ++++++++++++++++++++++--\n>  2 files changed, 24 insertions(+), 19 deletions(-)\n\nMakes sense.  Thanks.\n\n> diff --git a/packfile.c b/packfile.c\n> index 0929ebe4fc..8d7f37a5f6 100644\n> --- a/packfile.c\n> +++ b/packfile.c\n> @@ -514,19 +514,8 @@ static int open_packed_git_1(struct packed_git *p)\n>  \tssize_t read_result;\n>  \tconst unsigned hashsz = the_hash_algo->rawsz;\n>  \n> -\tif (!p->index_data) {\n> -\t\tstruct multi_pack_index *m;\n> -\t\tconst char *pack_name = pack_basename(p);\n> -\n> -\t\tfor (m = the_repository->objects->multi_pack_index;\n> -\t\t     m; m = m->next) {\n> -\t\t\tif (midx_contains_pack(m, pack_name))\n> -\t\t\t\tbreak;\n> -\t\t}\n> -\n> -\t\tif (!m && open_pack_index(p))\n> -\t\t\treturn error(\"packfile %s index unavailable\", p->pack_name);\n> -\t}\n> +\tif (open_pack_index(p))\n> +\t\treturn error(\"packfile %s index unavailable\", p->pack_name);\n>  \n>  \tif (!pack_max_fds) {\n>  \t\tunsigned int max_fds = get_max_fd_limit();\n> @@ -567,10 +556,6 @@ static int open_packed_git_1(struct packed_git *p)\n>  \t\t\t\" supported (try upgrading GIT to a newer version)\",\n>  \t\t\tp->pack_name, ntohl(hdr.hdr_version));\n>  \n> -\t/* Skip index checking if in multi-pack-index */\n> -\tif (!p->index_data)\n> -\t\treturn 0;\n> -\n>  \t/* Verify the pack matches its index. */\n>  \tif (p->num_objects != ntohl(hdr.hdr_entries))\n>  \t\treturn error(\"packfile %s claims to have %\"PRIu32\" objects\"\n> diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\n> index ace469c95c..d4607daec1 100755\n> --- a/t/t5319-multi-pack-index.sh\n> +++ b/t/t5319-multi-pack-index.sh\n> @@ -138,7 +138,7 @@ test_expect_success 'write midx with one v2 pack' '\n>  \n>  compare_results_with_midx \"one v2 pack\"\n>  \n> -test_expect_success 'corrupt idx not opened' '\n> +test_expect_success 'corrupt idx reports errors' '\n>  \tidx=$(test-tool read-midx $objdir | grep \"\\.idx\\$\") &&\n>  \tmv $objdir/pack/$idx backup-$idx &&\n>  \ttest_when_finished \"mv backup-\\$idx \\$objdir/pack/\\$idx\" &&\n> @@ -149,7 +149,7 @@ test_expect_success 'corrupt idx not opened' '\n>  \ttest_copy_bytes 1064 <backup-$idx >$objdir/pack/$idx &&\n>  \n>  \tgit -c core.multiPackIndex=true rev-list --objects --all 2>err &&\n> -\ttest_must_be_empty err\n> +\tgrep \"index unavailable\" err\n>  '\n>  \n>  test_expect_success 'add more objects' '\n> @@ -755,4 +755,24 @@ test_expect_success 'repack --batch-size=<large> repacks everything' '\n>  \t)\n>  '\n>  \n> +test_expect_success 'load reverse index when missing .idx' '\n> +\tgit init repo &&\n> +\ttest_when_finished \"rm -fr repo\" &&\n> +\t(\n> +\t\tcd repo &&\n> +\n> +\t\tgit config core.multiPackIndex true &&\n> +\n> +\t\ttest_commit base &&\n> +\t\tgit repack -ad &&\n> +\t\tgit multi-pack-index write &&\n> +\n> +\t\tgit rev-parse HEAD >tip &&\n> +\t\tidx=$(ls .git/objects/pack/pack-*.idx) &&\n> +\n> +\t\tmv $idx $idx.bak &&\n> +\t\tgit cat-file --batch-check=\"%(objectsize:disk)\" <tip\n> +\t)\n> +'\n> +\n>  test_done\n"},{"id":"410804","messageId":"xmqq360xdtdw.fsf@gitster.c.googlers.com","threadId":"54713","inReplyTo":"e1806d1bdc0da8061c78608e56138424ad24bed0.1606324509.git.me@ttaylorr.com","subject":"Re: [PATCH 2/2] midx.c: protect against disappearing packs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-25T21:22:03Z","receivedAt":"2020-11-25T21:22:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> When a packed object is stored in a multi-pack index, but that pack has\n> racily gone away, the MIDX code simply calls die(), when it could be\n> returning an error to the caller, which would in turn lead to\n> re-scanning the pack directory.\n\nMakes sense.  Will queue.\n\nThanks.\n"},{"id":"410843","messageId":"X777dl3pqioME7uM@coredump.intra.peff.net","threadId":"54713","inReplyTo":"xmqqblfldts4.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 0/2] midx: prevent against racily disappearing packs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-26T00:48:54Z","receivedAt":"2020-11-26T00:49:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 25, 2020 at 01:13:31PM -0800, Junio C Hamano wrote:\n\n> Taylor Blau <me@ttaylorr.com> writes:\n> \n> > Here are a couple of patches that Peff and I wrote after noticing a\n> > problem where racily disappearing .idx files can cause a segfault. While\n> > investigating, we fixed a related issue which is described and fixed in\n> > the second patch.\n> \n> In the cover letter it won't affect the end result, but when talking\n> about \"race\", it always is a good idea to explicitly mention both\n> sides of the race.  It is clear what one side is in the above\n> (i.e. somebody who removes .idx file that is still in use), but it\n> is not so clear who gets hit and segfaults.\n> \n> I am guessing that the other party is the user of .pack file(s)\n> bypassing the corresponding .idx file(s) because the necessary data\n> is mostly in .midx?\n\nYeah, the race reproduction in the second commit message can actually\nreproduce the segfault as well (it depends on the exact timing which\nerror you get). So the segfault is in the reader, who is not checking\nthe result of find_revindex_entry().\n\nArguably every call there should be checking for NULL, but in practice\nI think it would always be a bug:\n\n  - we were somehow unable to open the index in order to generate the\n    revindex (which is what happened here). But I think we are better\n    off making sure that we can always do so, which is what this series\n    does.\n\n  - the caller asked about an object at a position beyond the number of\n    objects in the packfile. This is a bug in the caller.\n\nSo we could perhaps BUG() in find_revindex_entry() instead of returning\nNULL. A quick segfault accomplishes mostly the same thing, though the\nBUG() could distinguish the two cases more clearly.\n\n-Peff\n"},{"id":"410847","messageId":"X77/LEwWlz9fjbYM@nand.local","threadId":"54713","inReplyTo":"X777dl3pqioME7uM@coredump.intra.peff.net","subject":"Re: [PATCH 0/2] midx: prevent against racily disappearing packs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-11-26T01:04:44Z","receivedAt":"2020-11-26T01:04:49Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Nov 25, 2020 at 07:48:54PM -0500, Jeff King wrote:\n> Yeah, the race reproduction in the second commit message can actually\n> reproduce the segfault as well (it depends on the exact timing which\n> error you get). So the segfault is in the reader, who is not checking\n> the result of find_revindex_entry().\n>\n> Arguably every call there should be checking for NULL, but in practice\n> I think it would always be a bug:\n>\n>   - we were somehow unable to open the index in order to generate the\n>     revindex (which is what happened here). But I think we are better\n>     off making sure that we can always do so, which is what this series\n>     does.\n>\n>   - the caller asked about an object at a position beyond the number of\n>     objects in the packfile. This is a bug in the caller.\n>\n> So we could perhaps BUG() in find_revindex_entry() instead of returning\n> NULL. A quick segfault accomplishes mostly the same thing, though the\n> BUG() could distinguish the two cases more clearly.\n\nYeah, a find_revindex_entry() that returns NULL means that the caller is\nprobably dead in the water.\n\nFWIW, this function gets touched by a series that I'm working on here:\n[1]. There, I think \"returning NULL\" is equivalent to \"returning -1\",\nand the problem exists there, too.\n\nWe could return a different negative number, call BUG(), or do nothing\nother than what's written. I don't have any strong feelings, though.\n\n> -Peff\n\nThanks,\nTaylor\n\n[1]: https://github.com/ttaylorr/git/blob/tb/on-disk-revindex-part-one/pack-revindex.c#L177-L201\n"},{"id":"410850","messageId":"X78EgOPAVg1vQRo9@coredump.intra.peff.net","threadId":"54713","inReplyTo":"X77/LEwWlz9fjbYM@nand.local","subject":"Re: [PATCH 0/2] midx: prevent against racily disappearing packs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-26T01:27:28Z","receivedAt":"2020-11-26T01:27:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 25, 2020 at 08:04:44PM -0500, Taylor Blau wrote:\n\n> > So we could perhaps BUG() in find_revindex_entry() instead of returning\n> > NULL. A quick segfault accomplishes mostly the same thing, though the\n> > BUG() could distinguish the two cases more clearly.\n> \n> Yeah, a find_revindex_entry() that returns NULL means that the caller is\n> probably dead in the water.\n> \n> FWIW, this function gets touched by a series that I'm working on here:\n> [1]. There, I think \"returning NULL\" is equivalent to \"returning -1\",\n> and the problem exists there, too.\n> \n> We could return a different negative number, call BUG(), or do nothing\n> other than what's written. I don't have any strong feelings, though.\n\nYeah, I was suggesting _never_ returning a failure, and just hitting a\nBUG() within the function. So it does not matter then how you represent\nthe error return type, because there isn't one. :)\n\nLooking over the callers, there are actually a few that check the return\nvalue and handle it sanely. I suspect they can never trigger in\npractice, given that the other callers would all segfault, and we have\nnot seen any reports in the 15 years during which that has been the\ncase. But perhaps some of them can be triggered by bogus pack data that\nnobody has ever run into in the real world.\n\nAt any rate, I am content to ignore it until somebody feels like digging\ninto each caller. A BUG() is only marginally better than an immediate\nsegfault anyway, and I'd prefer not to disrupt more substantive\nimprovements in the area.\n\n-Peff\n"}]}