{"thread":{"id":"56150","subject":"[PATCH] pack-bitmap: check pack validity when opening bitmap","startedAt":"2021-07-23T09:29:41Z","lastAt":"2021-07-23T09:29:41Z","messageCount":1,"participants":["Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"431008","messageId":"YPqL/pZt6hNYN4hB@coredump.intra.peff.net","threadId":"56150","inReplyTo":null,"subject":"[PATCH] pack-bitmap: check pack validity when opening bitmap","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-07-23T09:29:34Z","receivedAt":"2021-07-23T09:29:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When pack-objects adds an entry to its list of objects to pack, it may\nmark the packfile and offset that contains the file, which we can later\nuse to output the object verbatim.  If the packfile is deleted while we\nare running (e.g., by another process running \"git repack\"), we may die\nin use_pack() if the pack file cannot be opened.\n\nWe worked around this in 4c08018204 (pack-objects: protect against\ndisappearing packs, 2011-10-14) by making sure we can open the pack\nbefore recording it as a source. This detects a pack which has already\ndisappeared while generating the packing list, and because we keep the\npack's file descriptor (or an mmap window) open, it means we can access\nit later (unless you exceed core.packedgitlimit).\n\nThe bitmap code that was added later does not do this; it adds entries\nto the packlist without checking that the packfile is still valid, and\nis vulnerable to this race. It needs the same treatment as 4c08018204.\n\nHowever, rather than add it in just that one spot, it makes more sense\nto simply open and check the packfile when we open the bitmap.\nTechnically you can use the .bitmap without even looking in the .pack\nfile (e.g., if you are just printing a list of objects without accessing\nthem), but it's much simpler to do it early. That covers all later\ndirect uses of the pack (due to the cached descriptor) without having to\ncheck each one directly. For example, in pack-objects we need to protect\nthe packlist entries, but we also access the pack directly as part of\nthe reuse_partial_pack_from_bitmap() feature. This patch covers both\ncases.\n\nThere's no test here, because the problem is inherently racy. I\nreproduced and verified the fix with this script:\n\n  rm -rf parent.git push.git fetch.git\n\n  push() {\n    (\n      cd push.git &&\n      echo content >>file &&\n      git add file &&\n      git commit -qm \"change $1\" &&\n      git push -q origin HEAD &&\n      echo \"push $1...\"\n    ) &&\n    (\n      cd parent.git &&\n      git repack -ad -q &&\n      echo \"repack $1...\"\n    )\n  }\n\n  fetch() {\n    rm -rf fetch.git &&\n    git clone -q file://$PWD/parent.git fetch.git &&\n    echo \"fetch $1...\"\n  }\n\n  git init --bare parent.git &&\n  git --git-dir=parent.git config transfer.unpacklimit 1 &&\n  git clone parent.git push.git &&\n  (for i in `seq 1 1000`; do push $i || break; done) &\n  pusher=$!\n  (for i in `seq 1 1000`; do fetch $i || break; done) &\n  fetcher=$!\n  wait $fetcher\n  kill $pusher\n\nThat simulates a race between a client cloning and a push triggering a\nrepack on the server. Without this patch, it generally fails within a\ncouple hundred iterations with:\n\n  remote: fatal: packfile ./objects/pack/.tmp-1377349-pack-498afdec371232bdb99d1757872f5569331da61e.pack cannot be accessed\n  error: git upload-pack: git-pack-objects died with error.\n  fatal: git upload-pack: aborting due to possible repository corruption on the remote side.\n  remote: aborting due to possible repository corruption on the remote side.\n  fatal: early EOF\n  fatal: fetch-pack: invalid index-pack output\n\nWith this patch, it reliably runs through all thousand attempts.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI wrote this patch in 2015 after seeing this race in production, and\nwe've been running with it at GitHub ever since. I think I didn't\nupstream it back then because it was an incomplete fix without\nfurther changes to the pack-reuse code. And then of course I forgot all\nabout it. Those pack-reuse changes came upstream in a14aebeac3 (Merge\nbranch 'jk/packfile-reuse-cleanup', 2020-02-14), so this is a complete\nfix now (and recipe above is almost certainly triggering the problem in\nthe reuse code path, since we'd generally be sending the whole pack).\n\n pack-bitmap.c | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex bfc10148f5..c3a2c03b59 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -298,6 +298,11 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git\n \t\treturn -1;\n \t}\n \n+\tif (!is_pack_valid(packfile)) {\n+\t\tclose(fd);\n+\t\treturn -1;\n+\t}\n+\n \tbitmap_git->pack = packfile;\n \tbitmap_git->map_size = xsize_t(st.st_size);\n \tbitmap_git->map = xmmap(NULL, bitmap_git->map_size, PROT_READ, MAP_PRIVATE, fd, 0);\n-- \n2.32.0.784.g92e169d3d7\n"}]}