{"thread":{"id":"28669","subject":"[PATCH] pack-objects: protect against disappearing packs","startedAt":"2011-10-14T01:23:20Z","lastAt":"2011-10-14T18:04:16Z","messageCount":10,"participants":["Jeff King","Nicolas Pitre","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"177600","messageId":"20111014012320.GA4395@sigill.intra.peff.net","threadId":"28669","inReplyTo":null,"subject":"[PATCH] pack-objects: protect against disappearing packs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-10-14T01:23:20Z","receivedAt":"2011-10-14T01:23:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"It's possible that while pack-objects is running, a\nsimultaneously running prune process might delete a pack\nthat we are interested in. Because we load the pack indices\nearly on, we know that the pack contains our item, but by\nthe time we try to open and map it, it is gone.\n\nSince c715f78, we already protect against this in the normal\nobject access code path, but pack-objects accesses the packs\nat a lower level.  In the normal access path, we call\nfind_pack_entry, which will call find_pack_entry_one on each\npack index, which does the actual lookup. If it gets a hit,\nwe will actually open and verify the validity of the\nmatching packfile (using c715f78's is_pack_valid). If we\ncan't open it, we'll issue a warning and pretend that we\ndidn't find it, causing us to go on to the next pack (or on\nto loose objects).\n\nFurthermore, we will cache the descriptor to the opened\npackfile. Which means that later, when we actually try to\naccess the object, we are likely to still have that packfile\nopened, and won't care if it has been unlinked from the\nfilesystem.\n\nNotice the \"likely\" above. If there is another pack access\nin the interim, and we run out of descriptors, we could\nclose the pack. And then a later attempt to access the\nclosed pack could fail (we'll try to re-open it, of course,\nbut it may have been deleted). In practice, this doesn't\nhappen because we tend to look up items and then access them\nimmediately.\n\nPack-objects does not follow this code path. Instead, it\naccesses the packs at a much lower level, using\nfind_pack_entry_one directly. This means we skip the\nis_pack_valid check, and may end up with the name of a\npackfile, but no open descriptor.\n\nWe can add the same is_pack_valid check here. Unfortunately,\nthe access patterns of pack-objects are not quite as nice\nfor keeping lookup and object access together. We look up\neach object as we find out about it, and the only later when\nwriting the packfile do we necessarily access it. Which\nmeans that the opened packfile may be closed in the interim.\n\nIn practice, however, adding this check still has value, for\nthree reasons.\n\n  1. If you have a reasonable number of packs and/or a\n     reasonable file descriptor limit, you can keep all of\n     your packs open simultaneously. If this is the case,\n     then the race is impossible to trigger.\n\n  2. Even if you can't keep all packs open at once, you\n     may end up keeping the deleted one open (i.e., you may\n     get lucky).\n\n  3. The race window is shortened. You may notice early that\n     the pack is gone, and not try to access it. Triggering\n     the problem without this check means deleting the pack\n     any time after we read the list of index files, but\n     before we access the looked-up objects.  Triggering it\n     with this check means deleting the pack means deleting\n     the pack after we do a lookup (and successfully access\n     the packfile), but before we access the object. Which\n     is a smaller window.\n---\nWe're seeing this at GitHub because we prune pretty\naggressively. We let pushes go into individual repositories,\nbut then we kick off a job to move the resulting objects\ninto the repository's \"network\" repo, which is basically a\nbig alternates repository for related repos.\n\nYou can reproduce locally with:\n\n    push() {\n      (cd child &&\n       echo content >>file &&\n       git add file &&\n       git commit -q -m \"change `wc -l <file`\" &&\n       git push -q origin HEAD\n      ) &&\n      (cd network.git &&\n       git fetch -q ../parent refs/*:refs/* &&\n       git gc --auto\n      ) &&\n      (cd parent.git &&\n       git repack -qadl\n      )\n    }\n\n    fetch() {\n      rm -rf clone$1\n      git.compile clone -q file://$PWD/parent.git clone$1 2>>output$1 ||\n      {\n        echo >&2 FAIL FAIL FAIL\n        exit 1\n      }\n    }\n\n    git init --bare network.git &&\n    git --git-dir=network.git config transfer.unpacklimit 1 &&\n    git init --bare parent.git &&\n    git --git-dir=parent.git config transfer.unpacklimit 1 &&\n    echo \"$PWD/network.git/objects\" >parent.git/objects/info/alternates\n    git clone parent.git child &&\n    (for i in `seq 1 1000`; do push; done) &\n    (for i in `seq 1 1000`; do fetch 1; done) &\n    (for i in `seq 1 1000`; do fetch 2; done) &\n    wait\n\nOne process repeatedly pushes new packs, then migrates the\nobjects over to the network repo. Simultaneously, two\nprocesses are constantly cloning. After 10-20 seconds, I'll\nusually get unlucky and a clone will fail (because the\nremote pack-objects complains about the inaccessible\npackfile).\n\nAnother option would be to open the pack at point of use, and if that\nfails, try to find the object elsewhere. This throws off the rest of the\ncode a bit, though, as we were expecting to get it from a pack, which\nwent into our decision about what we should delta.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/pack-objects.c |    4 ++++\n cache.h                |    1 +\n sha1_file.c            |    2 +-\n 3 files changed, 6 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 2b18de5..8681ccd 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -804,6 +804,10 @@ static int add_object_entry(const unsigned char *sha1, enum object_type type,\n \t\toff_t offset = find_pack_entry_one(sha1, p);\n \t\tif (offset) {\n \t\t\tif (!found_pack) {\n+\t\t\t\tif (!is_pack_valid(p)) {\n+\t\t\t\t\terror(\"packfile %s cannot be accessed\", p->pack_name);\n+\t\t\t\t\tcontinue;\n+\t\t\t\t}\n \t\t\t\tfound_offset = offset;\n \t\t\t\tfound_pack = p;\n \t\t\t}\ndiff --git a/cache.h b/cache.h\nindex e39e160..495b468 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1055,6 +1055,7 @@ struct extra_have_objects {\n extern const unsigned char *nth_packed_object_sha1(struct packed_git *, uint32_t);\n extern off_t nth_packed_object_offset(const struct packed_git *, uint32_t);\n extern off_t find_pack_entry_one(const unsigned char *, struct packed_git *);\n+extern int is_pack_valid(struct packed_git *);\n extern void *unpack_entry(struct packed_git *, off_t, enum object_type *, unsigned long *);\n extern unsigned long unpack_object_header_buffer(const unsigned char *buf, unsigned long len, enum object_type *type, unsigned long *sizep);\n extern unsigned long get_size_from_delta(struct packed_git *, struct pack_window **, off_t);\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 3401301..a22c5b4 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1987,7 +1987,7 @@ off_t find_pack_entry_one(const unsigned char *sha1,\n \treturn 0;\n }\n \n-static int is_pack_valid(struct packed_git *p)\n+int is_pack_valid(struct packed_git *p)\n {\n \t/* An already open pack is known to be valid. */\n \tif (p->pack_fd != -1)\n-- \n1.7.6.4.37.g43b58b\n"},{"id":"177601","messageId":"20111014013130.GA7163@sigill.intra.peff.net","threadId":"28669","inReplyTo":"20111014012320.GA4395@sigill.intra.peff.net","subject":"Re: [PATCH] pack-objects: protect against disappearing packs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-10-14T01:31:30Z","receivedAt":"2011-10-14T01:31:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 13, 2011 at 09:23:20PM -0400, Jeff King wrote:\n\n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index 2b18de5..8681ccd 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -804,6 +804,10 @@ static int add_object_entry(const unsigned char *sha1, enum object_type type,\n>  \t\toff_t offset = find_pack_entry_one(sha1, p);\n>  \t\tif (offset) {\n>  \t\t\tif (!found_pack) {\n> +\t\t\t\tif (!is_pack_valid(p)) {\n> +\t\t\t\t\terror(\"packfile %s cannot be accessed\", p->pack_name);\n> +\t\t\t\t\tcontinue;\n> +\t\t\t\t}\n\nThis message is modeled after the one in find_pack_entry. However,\nthey're not really errors, since we will try to find the object\nelsewhere (and generally succeed). So the messages could just go away.\nThough they can also alert you to something fishy going on (like a\npackfile with bad permissions). But perhaps we should downgrade them\nlike this:\n\n-- >8 --\nSubject: [PATCH] downgrade \"packfile cannot be accessed\" errors to warnings\n\nThese can happen if another process simultaneously prunes a\npack. But that is not usually an error condition, because a\nproperly-running prune should have repacked the object into\na new pack. So we will notice that the pack has disappeared\nunexpectedly, print a message, try other packs (possibly\nafter re-scanning the list of packs), and find it in the new\npack.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/pack-objects.c |    2 +-\n sha1_file.c            |    2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 8681ccd..ba3705d 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -805,7 +805,7 @@ static int add_object_entry(const unsigned char *sha1, enum object_type type,\n \t\tif (offset) {\n \t\t\tif (!found_pack) {\n \t\t\t\tif (!is_pack_valid(p)) {\n-\t\t\t\t\terror(\"packfile %s cannot be accessed\", p->pack_name);\n+\t\t\t\t\twarning(\"packfile %s cannot be accessed\", p->pack_name);\n \t\t\t\t\tcontinue;\n \t\t\t\t}\n \t\t\t\tfound_offset = offset;\ndiff --git a/sha1_file.c b/sha1_file.c\nindex a22c5b4..27f3b9b 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2038,7 +2038,7 @@ static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)\n \t\t\t * was loaded!\n \t\t\t */\n \t\t\tif (!is_pack_valid(p)) {\n-\t\t\t\terror(\"packfile %s cannot be accessed\", p->pack_name);\n+\t\t\t\twarning(\"packfile %s cannot be accessed\", p->pack_name);\n \t\t\t\tgoto next;\n \t\t\t}\n \t\t\te->offset = offset;\n-- \n1.7.6.4.37.g43b58b\n"},{"id":"177605","messageId":"alpine.LFD.2.02.1110132234210.17040@xanadu.home","threadId":"28669","inReplyTo":"20111014012320.GA4395@sigill.intra.peff.net","subject":"Re: [PATCH] pack-objects: protect against disappearing packs","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2011-10-14T02:42:28Z","receivedAt":"2011-10-14T02:42:28Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Thu, 13 Oct 2011, Jeff King wrote:\n\n> It's possible that while pack-objects is running, a\n> simultaneously running prune process might delete a pack\n> that we are interested in. Because we load the pack indices\n> early on, we know that the pack contains our item, but by\n> the time we try to open and map it, it is gone.\n> \n> Since c715f78, we already protect against this in the normal\n> object access code path, but pack-objects accesses the packs\n> at a lower level.  In the normal access path, we call\n> find_pack_entry, which will call find_pack_entry_one on each\n> pack index, which does the actual lookup. If it gets a hit,\n> we will actually open and verify the validity of the\n> matching packfile (using c715f78's is_pack_valid). If we\n> can't open it, we'll issue a warning and pretend that we\n> didn't find it, causing us to go on to the next pack (or on\n> to loose objects).\n> \n> Furthermore, we will cache the descriptor to the opened\n> packfile. Which means that later, when we actually try to\n> access the object, we are likely to still have that packfile\n> opened, and won't care if it has been unlinked from the\n> filesystem.\n> \n> Notice the \"likely\" above. If there is another pack access\n> in the interim, and we run out of descriptors, we could\n> close the pack. And then a later attempt to access the\n> closed pack could fail (we'll try to re-open it, of course,\n> but it may have been deleted). In practice, this doesn't\n> happen because we tend to look up items and then access them\n> immediately.\n> \n> Pack-objects does not follow this code path. Instead, it\n> accesses the packs at a much lower level, using\n> find_pack_entry_one directly. This means we skip the\n> is_pack_valid check, and may end up with the name of a\n> packfile, but no open descriptor.\n> \n> We can add the same is_pack_valid check here. Unfortunately,\n> the access patterns of pack-objects are not quite as nice\n> for keeping lookup and object access together. We look up\n> each object as we find out about it, and the only later when\n> writing the packfile do we necessarily access it. Which\n> means that the opened packfile may be closed in the interim.\n> \n> In practice, however, adding this check still has value, for\n> three reasons.\n> \n>   1. If you have a reasonable number of packs and/or a\n>      reasonable file descriptor limit, you can keep all of\n>      your packs open simultaneously. If this is the case,\n>      then the race is impossible to trigger.\n> \n>   2. Even if you can't keep all packs open at once, you\n>      may end up keeping the deleted one open (i.e., you may\n>      get lucky).\n> \n>   3. The race window is shortened. You may notice early that\n>      the pack is gone, and not try to access it. Triggering\n>      the problem without this check means deleting the pack\n>      any time after we read the list of index files, but\n>      before we access the looked-up objects.  Triggering it\n>      with this check means deleting the pack means deleting\n>      the pack after we do a lookup (and successfully access\n>      the packfile), but before we access the object. Which\n>      is a smaller window.\n> ---\n\nyou should put your SOB above that line I would think.\n\nAcked-by: Nicolas Pitre <nico@fluxnic.net>\n\n> We're seeing this at GitHub because we prune pretty\n> aggressively. We let pushes go into individual repositories,\n> but then we kick off a job to move the resulting objects\n> into the repository's \"network\" repo, which is basically a\n> big alternates repository for related repos.\n\nWhile this patch certainly has value, it doesn't provide 100% \nreliability for that use case.  Maybe the github infrastructure should \nsimply skip any auto-repack on push if some other object maintenance \noperation is ongoing, possibly via the pre-auto-gc hook.\n\n\nNicolas\n"},{"id":"177606","messageId":"alpine.LFD.2.02.1110132242540.17040@xanadu.home","threadId":"28669","inReplyTo":"20111014013130.GA7163@sigill.intra.peff.net","subject":"Re: [PATCH] pack-objects: protect against disappearing packs","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2011-10-14T02:43:54Z","receivedAt":"2011-10-14T02:43:54Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Thu, 13 Oct 2011, Jeff King wrote:\n\n> On Thu, Oct 13, 2011 at 09:23:20PM -0400, Jeff King wrote:\n> \n> > diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> > index 2b18de5..8681ccd 100644\n> > --- a/builtin/pack-objects.c\n> > +++ b/builtin/pack-objects.c\n> > @@ -804,6 +804,10 @@ static int add_object_entry(const unsigned char *sha1, enum object_type type,\n> >  \t\toff_t offset = find_pack_entry_one(sha1, p);\n> >  \t\tif (offset) {\n> >  \t\t\tif (!found_pack) {\n> > +\t\t\t\tif (!is_pack_valid(p)) {\n> > +\t\t\t\t\terror(\"packfile %s cannot be accessed\", p->pack_name);\n> > +\t\t\t\t\tcontinue;\n> > +\t\t\t\t}\n> \n> This message is modeled after the one in find_pack_entry. However,\n> they're not really errors, since we will try to find the object\n> elsewhere (and generally succeed). So the messages could just go away.\n> Though they can also alert you to something fishy going on (like a\n> packfile with bad permissions). But perhaps we should downgrade them\n> like this:\n> \n> -- >8 --\n> Subject: [PATCH] downgrade \"packfile cannot be accessed\" errors to warnings\n> \n> These can happen if another process simultaneously prunes a\n> pack. But that is not usually an error condition, because a\n> properly-running prune should have repacked the object into\n> a new pack. So we will notice that the pack has disappeared\n> unexpectedly, print a message, try other packs (possibly\n> after re-scanning the list of packs), and find it in the new\n> pack.\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n\nAcked-by: Nicolas Pitre <nico@fluxnic.net>\n\n\n> ---\n>  builtin/pack-objects.c |    2 +-\n>  sha1_file.c            |    2 +-\n>  2 files changed, 2 insertions(+), 2 deletions(-)\n> \n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index 8681ccd..ba3705d 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -805,7 +805,7 @@ static int add_object_entry(const unsigned char *sha1, enum object_type type,\n>  \t\tif (offset) {\n>  \t\t\tif (!found_pack) {\n>  \t\t\t\tif (!is_pack_valid(p)) {\n> -\t\t\t\t\terror(\"packfile %s cannot be accessed\", p->pack_name);\n> +\t\t\t\t\twarning(\"packfile %s cannot be accessed\", p->pack_name);\n>  \t\t\t\t\tcontinue;\n>  \t\t\t\t}\n>  \t\t\t\tfound_offset = offset;\n> diff --git a/sha1_file.c b/sha1_file.c\n> index a22c5b4..27f3b9b 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -2038,7 +2038,7 @@ static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)\n>  \t\t\t * was loaded!\n>  \t\t\t */\n>  \t\t\tif (!is_pack_valid(p)) {\n> -\t\t\t\terror(\"packfile %s cannot be accessed\", p->pack_name);\n> +\t\t\t\twarning(\"packfile %s cannot be accessed\", p->pack_name);\n>  \t\t\t\tgoto next;\n>  \t\t\t}\n>  \t\t\te->offset = offset;\n> -- \n> 1.7.6.4.37.g43b58b\n> \n"},{"id":"177613","messageId":"4E97DF63.104@viscovery.net","threadId":"28669","inReplyTo":"20111014012320.GA4395@sigill.intra.peff.net","subject":"Re: [PATCH] pack-objects: protect against disappearing packs","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2011-10-14T07:06:11Z","receivedAt":"2011-10-14T07:06:11Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 10/14/2011 3:23, schrieb Jeff King:\n> In practice, however, adding this check still has value, for\n> three reasons.\n> \n>   1. If you have a reasonable number of packs and/or a\n>      reasonable file descriptor limit, you can keep all of\n>      your packs open simultaneously. If this is the case,\n>      then the race is impossible to trigger.\n\nOn Windows, we cannot remove files that are open. If I understand\ncorrectly, this patch keeps more files open for a longer time. Is there\nany chance that packfiles remain now open until an unlink() call?\n\nI am not worried about parallel processes (we already have a problem\nthere), but that this can now happen within a single process, i.e., that a\nsingle git-repack -a -d -f would now try to unlink a pack file that it\nopened itself and did not close timely.\n\nI'll test your patch later this weekend to see whether the test suite\nfinds something. But perhaps you know the answer already?\n\n-- Hannes\n"},{"id":"177640","messageId":"20111014130247.GA7808@sigill.intra.peff.net","threadId":"28669","inReplyTo":"alpine.LFD.2.02.1110132234210.17040@xanadu.home","subject":"Re: [PATCH] pack-objects: protect against disappearing packs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-10-14T13:02:48Z","receivedAt":"2011-10-14T13:02:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 13, 2011 at 10:42:28PM -0400, Nicolas Pitre wrote:\n\n> > ---\n> \n> you should put your SOB above that line I would think.\n\nThanks. I cheated and wrote my \"---\" cover letter in the commit message\nlocally, knowing that it would get included by format-patch but stripped\nby am on Junio's end.  Which does work, except that \"format-patch -s\"\nputs the SOB in the wrong place. :)\n\n> Acked-by: Nicolas Pitre <nico@fluxnic.net>\n\nThanks for reviewing.\n\n> > We're seeing this at GitHub because we prune pretty\n> > aggressively. We let pushes go into individual repositories,\n> > but then we kick off a job to move the resulting objects\n> > into the repository's \"network\" repo, which is basically a\n> > big alternates repository for related repos.\n> \n> While this patch certainly has value, it doesn't provide 100% \n> reliability for that use case.  Maybe the github infrastructure should \n> simply skip any auto-repack on push if some other object maintenance \n> operation is ongoing, possibly via the pre-auto-gc hook.\n\nI'm not sure I understand the problem.  We already serialize the\nre-packing jobs in a queue, so you won't have two repacks going at once.\nThe problematic pack-objects is the one started by upload-pack when\nsomebody fetches. Or do you mean turning off receive.autogc? I'd have to\ncheck if we do that, but we definitely should; it's useless to us\n(though it would be unlikely to trigger anyway, because we are manually\nrepacking so frequently).\n\n-Peff\n"},{"id":"177641","messageId":"20111014130703.GB7808@sigill.intra.peff.net","threadId":"28669","inReplyTo":"4E97DF63.104@viscovery.net","subject":"Re: [PATCH] pack-objects: protect against disappearing packs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-10-14T13:07:03Z","receivedAt":"2011-10-14T13:07:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 14, 2011 at 09:06:11AM +0200, Johannes Sixt wrote:\n\n> Am 10/14/2011 3:23, schrieb Jeff King:\n> > In practice, however, adding this check still has value, for\n> > three reasons.\n> > \n> >   1. If you have a reasonable number of packs and/or a\n> >      reasonable file descriptor limit, you can keep all of\n> >      your packs open simultaneously. If this is the case,\n> >      then the race is impossible to trigger.\n> \n> On Windows, we cannot remove files that are open. If I understand\n> correctly, this patch keeps more files open for a longer time. Is there\n> any chance that packfiles remain now open until an unlink() call?\n> \n> I am not worried about parallel processes (we already have a problem\n> there), but that this can now happen within a single process, i.e., that a\n> single git-repack -a -d -f would now try to unlink a pack file that it\n> opened itself and did not close timely.\n> \n> I'll test your patch later this weekend to see whether the test suite\n> finds something. But perhaps you know the answer already?\n\nWith two parallel processes, this will definitely increase the\nlikelihood of a deleted file being open. That is the point. :)\n\nWithin a single process, I don't think so. This change impacts only\npack-objects, which always runs as a separate process, and never deletes\npacks itself. The most likely problematic code path would be \"git\nrepack -d\", but it waits for pack-objects to complete successfully\nbefore removing any packs.\n\n-Peff\n"},{"id":"177649","messageId":"4E9848AB.3010802@viscovery.net","threadId":"28669","inReplyTo":"20111014130703.GB7808@sigill.intra.peff.net","subject":"Re: [PATCH] pack-objects: protect against disappearing packs","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2011-10-14T14:35:23Z","receivedAt":"2011-10-14T14:35:23Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 10/14/2011 15:07, schrieb Jeff King:\n> Within a single process, I don't think so. This change impacts only\n> pack-objects, which always runs as a separate process, and never deletes\n> packs itself. The most likely problematic code path would be \"git\n> repack -d\", but it waits for pack-objects to complete successfully\n> before removing any packs.\n\nThanks. The test suite didn't find any issues on Windows.\n\n-- Hannes\n"},{"id":"177659","messageId":"20111014180348.GA17412@sigill.intra.peff.net","threadId":"28669","inReplyTo":"20111014012320.GA4395@sigill.intra.peff.net","subject":"[PATCH 1/2] pack-objects: protect against disappearing packs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-10-14T18:03:48Z","receivedAt":"2011-10-14T18:03:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"It's possible that while pack-objects is running, a\nsimultaneously running prune process might delete a pack\nthat we are interested in. Because we load the pack indices\nearly on, we know that the pack contains our item, but by\nthe time we try to open and map it, it is gone.\n\nSince c715f78, we already protect against this in the normal\nobject access code path, but pack-objects accesses the packs\nat a lower level.  In the normal access path, we call\nfind_pack_entry, which will call find_pack_entry_one on each\npack index, which does the actual lookup. If it gets a hit,\nwe will actually open and verify the validity of the\nmatching packfile (using c715f78's is_pack_valid). If we\ncan't open it, we'll issue a warning and pretend that we\ndidn't find it, causing us to go on to the next pack (or on\nto loose objects).\n\nFurthermore, we will cache the descriptor to the opened\npackfile. Which means that later, when we actually try to\naccess the object, we are likely to still have that packfile\nopened, and won't care if it has been unlinked from the\nfilesystem.\n\nNotice the \"likely\" above. If there is another pack access\nin the interim, and we run out of descriptors, we could\nclose the pack. And then a later attempt to access the\nclosed pack could fail (we'll try to re-open it, of course,\nbut it may have been deleted). In practice, this doesn't\nhappen because we tend to look up items and then access them\nimmediately.\n\nPack-objects does not follow this code path. Instead, it\naccesses the packs at a much lower level, using\nfind_pack_entry_one directly. This means we skip the\nis_pack_valid check, and may end up with the name of a\npackfile, but no open descriptor.\n\nWe can add the same is_pack_valid check here. Unfortunately,\nthe access patterns of pack-objects are not quite as nice\nfor keeping lookup and object access together. We look up\neach object as we find out about it, and the only later when\nwriting the packfile do we necessarily access it. Which\nmeans that the opened packfile may be closed in the interim.\n\nIn practice, however, adding this check still has value, for\nthree reasons.\n\n  1. If you have a reasonable number of packs and/or a\n     reasonable file descriptor limit, you can keep all of\n     your packs open simultaneously. If this is the case,\n     then the race is impossible to trigger.\n\n  2. Even if you can't keep all packs open at once, you\n     may end up keeping the deleted one open (i.e., you may\n     get lucky).\n\n  3. The race window is shortened. You may notice early that\n     the pack is gone, and not try to access it. Triggering\n     the problem without this check means deleting the pack\n     any time after we read the list of index files, but\n     before we access the looked-up objects.  Triggering it\n     with this check means deleting the pack means deleting\n     the pack after we do a lookup (and successfully access\n     the packfile), but before we access the object. Which\n     is a smaller window.\n\nAcked-by: Nicolas Pitre <nico@fluxnic.net>\nSigned-off-by: Jeff King <peff@peff.net>\n---\nRe-post with ack from Nicolas and my SOB fixed. No changes from earlier\nversion in this thread.\n\n builtin/pack-objects.c |    4 ++++\n cache.h                |    1 +\n sha1_file.c            |    2 +-\n 3 files changed, 6 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 2b18de5..8681ccd 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -804,6 +804,10 @@ static int add_object_entry(const unsigned char *sha1, enum object_type type,\n \t\toff_t offset = find_pack_entry_one(sha1, p);\n \t\tif (offset) {\n \t\t\tif (!found_pack) {\n+\t\t\t\tif (!is_pack_valid(p)) {\n+\t\t\t\t\terror(\"packfile %s cannot be accessed\", p->pack_name);\n+\t\t\t\t\tcontinue;\n+\t\t\t\t}\n \t\t\t\tfound_offset = offset;\n \t\t\t\tfound_pack = p;\n \t\t\t}\ndiff --git a/cache.h b/cache.h\nindex e39e160..495b468 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1055,6 +1055,7 @@ struct extra_have_objects {\n extern const unsigned char *nth_packed_object_sha1(struct packed_git *, uint32_t);\n extern off_t nth_packed_object_offset(const struct packed_git *, uint32_t);\n extern off_t find_pack_entry_one(const unsigned char *, struct packed_git *);\n+extern int is_pack_valid(struct packed_git *);\n extern void *unpack_entry(struct packed_git *, off_t, enum object_type *, unsigned long *);\n extern unsigned long unpack_object_header_buffer(const unsigned char *buf, unsigned long len, enum object_type *type, unsigned long *sizep);\n extern unsigned long get_size_from_delta(struct packed_git *, struct pack_window **, off_t);\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 3401301..a22c5b4 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1987,7 +1987,7 @@ off_t find_pack_entry_one(const unsigned char *sha1,\n \treturn 0;\n }\n \n-static int is_pack_valid(struct packed_git *p)\n+int is_pack_valid(struct packed_git *p)\n {\n \t/* An already open pack is known to be valid. */\n \tif (p->pack_fd != -1)\n-- \n1.7.6.4.37.g43b58b\n"},{"id":"177660","messageId":"20111014180415.GB17412@sigill.intra.peff.net","threadId":"28669","inReplyTo":"20111014012320.GA4395@sigill.intra.peff.net","subject":"[PATCH 2/2] downgrade \"packfile cannot be accessed\" errors to warnings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-10-14T18:04:16Z","receivedAt":"2011-10-14T18:04:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"These can happen if another process simultaneously prunes a\npack. But that is not usually an error condition, because a\nproperly-running prune should have repacked the object into\na new pack. So we will notice that the pack has disappeared\nunexpectedly, print a message, try other packs (possibly\nafter re-scanning the list of packs), and find it in the new\npack.\n\nAcked-by: Nicolas Pitre <nico@fluxnic.net>\nSigned-off-by: Jeff King <peff@peff.net>\n---\nRepost with ack from Nicolas, and more obvious subject line. No changes\nfrom earlier version in this thread.\n\n builtin/pack-objects.c |    2 +-\n sha1_file.c            |    2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 8681ccd..ba3705d 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -805,7 +805,7 @@ static int add_object_entry(const unsigned char *sha1, enum object_type type,\n \t\tif (offset) {\n \t\t\tif (!found_pack) {\n \t\t\t\tif (!is_pack_valid(p)) {\n-\t\t\t\t\terror(\"packfile %s cannot be accessed\", p->pack_name);\n+\t\t\t\t\twarning(\"packfile %s cannot be accessed\", p->pack_name);\n \t\t\t\t\tcontinue;\n \t\t\t\t}\n \t\t\t\tfound_offset = offset;\ndiff --git a/sha1_file.c b/sha1_file.c\nindex a22c5b4..27f3b9b 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2038,7 +2038,7 @@ static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)\n \t\t\t * was loaded!\n \t\t\t */\n \t\t\tif (!is_pack_valid(p)) {\n-\t\t\t\terror(\"packfile %s cannot be accessed\", p->pack_name);\n+\t\t\t\twarning(\"packfile %s cannot be accessed\", p->pack_name);\n \t\t\t\tgoto next;\n \t\t\t}\n \t\t\te->offset = offset;\n-- \n1.7.6.4.37.g43b58b\n"}]}