{"thread":{"id":"44129","subject":"[PATCH] verify_packfile: check pack validity before accessing data","startedAt":"2016-09-22T03:49:41Z","lastAt":"2016-09-22T18:21:30Z","messageCount":3,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"302353","messageId":"20160922034904.dm5okldfmgmin5d7@sigill.intra.peff.net","threadId":"44129","inReplyTo":null,"subject":"[PATCH] verify_packfile: check pack validity before accessing data","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-22T03:49:05Z","receivedAt":"2016-09-22T03:49:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The verify_packfile() does not explicitly open the packfile;\ninstead, it starts with a sha1 checksum over the whole pack,\nand relies on use_pack() to open the packfile as a side\neffect.\n\nIf the pack cannot be opened for whatever reason (either\nbecause its header information is corrupted, or perhaps\nbecause a simultaneous repack deleted it), then use_pack()\nwill die(), as it has no way to return an error. This is not\nideal, as verify_packfile() otherwise tries to gently return\nan error (this lets programs like git-fsck go on to check\nother packs).\n\nInstead, let's check is_pack_valid() up front, and return an\nerror if it fails. This will open the pack as a side effect,\nand then use_pack() will later rely on our cached\ndescriptor, and avoid calling die().\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n pack-check.c | 7 ++-----\n 1 file changed, 2 insertions(+), 5 deletions(-)\n\ndiff --git a/pack-check.c b/pack-check.c\nindex d123846..c5c7763 100644\n--- a/pack-check.c\n+++ b/pack-check.c\n@@ -57,11 +57,8 @@ static int verify_packfile(struct packed_git *p,\n \tint err = 0;\n \tstruct idx_entry *entries;\n \n-\t/* Note that the pack header checks are actually performed by\n-\t * use_pack when it first opens the pack file.  If anything\n-\t * goes wrong during those checks then the call will die out\n-\t * immediately.\n-\t */\n+\tif (!is_pack_valid(p))\n+\t\treturn error(\"packfile %s cannot be accessed\", p->pack_name);\n \n \tgit_SHA1_Init(&ctx);\n \tdo {\n-- \n2.10.0.482.gae5a597\n"},{"id":"302354","messageId":"20160922040523.x3pihbs7f5iudyfz@sigill.intra.peff.net","threadId":"44129","inReplyTo":"20160922034904.dm5okldfmgmin5d7@sigill.intra.peff.net","subject":"Re: [PATCH] verify_packfile: check pack validity before accessing data","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-22T04:05:24Z","receivedAt":"2016-09-22T04:07:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 21, 2016 at 11:49:05PM -0400, Jeff King wrote:\n\n> The verify_packfile() does not explicitly open the packfile;\n> instead, it starts with a sha1 checksum over the whole pack,\n> and relies on use_pack() to open the packfile as a side\n> effect.\n> \n> If the pack cannot be opened for whatever reason (either\n> because its header information is corrupted, or perhaps\n> because a simultaneous repack deleted it), then use_pack()\n> will die(), as it has no way to return an error. This is not\n> ideal, as verify_packfile() otherwise tries to gently return\n> an error (this lets programs like git-fsck go on to check\n> other packs).\n> \n> Instead, let's check is_pack_valid() up front, and return an\n> error if it fails. This will open the pack as a side effect,\n> and then use_pack() will later rely on our cached\n> descriptor, and avoid calling die().\n\nI actually had an ulterior motive, but it didn't pan out. I\nthink this change is an improvement on its own, which is why I posted\nit. But here's my ulterior motive, for reference.\n\nThe die() in question happens when use_pack() is asked to lazily open a\npack, but it fails. If this happens then the code in question is racy\nwith respect to somebody else running a repack, because the pack we are\nlooking for might go away, but we could find the object in another pack.\nSince we cannot handle the retry at this level, callers of use_pack()\nshould generally use is_pack_valid() early to cache the descriptor (and\nat that early stage, they can still bail to another pack if necessary).\nSee the comment in fill_pack_entry(), for example, or the discussion in \n4c08018 (pack-objects: protect against disappearing packs, 2011-10-14).\n\nSo I wanted to know whether there were any code paths that failed to do\nso, and just blindly rely on the lazy-open. Finding the races is\ninherently hard, because you only catch them when somebody else is doing\na repack. But if we just _remove_ the lazy-load, then it becomes easy to\ncatch anybody relying on it. Like:\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex b9c1fa3..f3d7615 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1122,8 +1122,8 @@ unsigned char *use_pack(struct packed_git *p,\n \t * hash, and the in_window function above wouldn't match\n \t * don't allow an offset too close to the end of the file.\n \t */\n-\tif (!p->pack_size && p->pack_fd == -1 && open_packed_git(p))\n-\t\tdie(\"packfile %s cannot be accessed\", p->pack_name);\n+\tif (!p->pack_size && p->pack_fd == -1)\n+\t\tdie(\"BUG: use_pack() called on unopened '%s'\", p->pack_name);\n \tif (offset > (p->pack_size - 20))\n \t\tdie(\"offset beyond end of packfile (truncated pack?)\");\n \tif (offset < 0)\n@@ -1140,8 +1140,8 @@ unsigned char *use_pack(struct packed_git *p,\n \t\t\tsize_t window_align = packed_git_window_size / 2;\n \t\t\toff_t len;\n \n-\t\t\tif (p->pack_fd == -1 && open_packed_git(p))\n-\t\t\t\tdie(\"packfile %s cannot be accessed\", p->pack_name);\n+\t\t\tif (p->pack_fd == -1)\n+\t\t\t\tdie(\"BUG: use_pack() called on unopened '%s'\", p->pack_name);\n \n \t\t\twin = xcalloc(1, sizeof(*win));\n \t\t\twin->offset = (offset / window_align) * window_align;\n\n\nRunning the test suite with the patch above revealed the issue in\nverify_packfile (and with my patch, the test suite now passes, even with\nthis).\n\nSo I was hoping that we could convert these into assertions as above.\nBut the test suite passing does not quite tell the whole story. We might\nstill close a pack in the middle of an operation if we need to open\nanother one and are running against the system file descriptor limits.\nThat would only trigger in a repository with a large number of packs (or\na very low descriptor limit).\n\nIn such a case, we are relying on the lazy-load (and we _are_ racy!).\nBut the patch above would punish people on low-descriptor systems. It's\nbetter to have an unlikely race and complete the request than to fail\nconsistently. :-/\n\nFor people who are running high-traffic servers, they just need to make\nsure their file descriptor limit is reasonably high to avoid the race.\n\n-Peff\n"},{"id":"302382","messageId":"xmqq37kr6bj5.fsf@gitster.mtv.corp.google.com","threadId":"44129","inReplyTo":"20160922040523.x3pihbs7f5iudyfz@sigill.intra.peff.net","subject":"Re: [PATCH] verify_packfile: check pack validity before accessing data","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-22T18:21:18Z","receivedAt":"2016-09-22T18:21:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> So I wanted to know whether there were any code paths that failed to do\n> so, and just blindly rely on the lazy-open. Finding the races is\n> inherently hard, because you only catch them when somebody else is doing\n> a repack. But if we just _remove_ the lazy-load, then it becomes easy to\n> catch anybody relying on it. Like:\n> ...\n\nClever; I like it.\n\n> In such a case, we are relying on the lazy-load (and we _are_ racy!).\n> But the patch above would punish people on low-descriptor systems. It's\n> better to have an unlikely race and complete the request than to fail\n> consistently. :-/\n>\n> For people who are running high-traffic servers, they just need to make\n> sure their file descriptor limit is reasonably high to avoid the race.\n\nThanks for an illuminating backstory for the patch.\n\n"}]}