{"thread":{"id":"40290","subject":"Bug: git-upload-pack will return successfully even when it can't read all references","startedAt":"2015-09-07T12:11:15Z","lastAt":"2015-09-08T22:06:11Z","messageCount":4,"participants":["Ævar Arnfjörð Bjarmason","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"269555","messageId":"CACBZZX6ZYDEPrQorg=pVh734ua+x55SYoKKvSZ_h0GQaR=m+8w@mail.gmail.com","threadId":"40290","inReplyTo":null,"subject":"Bug: git-upload-pack will return successfully even when it can't read all references","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2015-09-07T12:11:15Z","receivedAt":"2015-09-07T12:11:15Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"We have a process to back up our Git repositories at work, this\nstarted alerting because it wasn't getting the same refs as the\nremote.\n\nThis turned out to be a pretty trivial filesystem error.\nrefs/heads/master wasn't readable by the backup process, but some\nother stuff in refs/heads and objects/* was.\n\nBut I think it's a bug that if we ssh to the remote end, and\ngit-upload-pack can't read certain refs in refs/heads/ that we don't\nreturn an error.\n\nThis simple shellscript reproduces the issue:\n\n    rm -rf /tmp/repo /tmp/repo-checkout\n    git init /tmp/repo\n    cd /tmp/repo\n    touch foo\n    git add foo\n    git commit -m\"foo\"\n    git checkout -b branch\n    git checkout master\n    git show-ref\n    chmod 000 .git/refs/heads/master\n    git show-ref\n    cd /tmp\n    git clone repo repo-checkout\n    echo \"Status code of clone: $?\"\n    cd repo-checkout\n    git show-ref\n\nAfter running this you get:\n\n    $ (cd /tmp/repo-checkout && echo -n | strace\n/tmp/avar/bin/git-upload-pack /tmp/repo 2>&1 | grep -e EACCES)\n    open(\"refs/heads/master\", O_RDONLY)     = -1 EACCES (Permission denied)\n    open(\"refs/heads/master\", O_RDONLY)     = -1 EACCES (Permission denied)\n    open(\"refs/heads/master\", O_RDONLY)     = -1 EACCES (Permission denied)\n\nAnd \"git fetch\" will return 0.\n\nWe fail to call get refs/heads/master in head_ref_namespaced() called\nby upload_pack(). I was going to see if I could patch it to return an\nerror, but that code seems very far removed from any error checking.\n\nThis isn't only an issue with git-upload-pack, e.g. show-ref itself\nhas the same issue:\n\n    $ chmod 600 .git/refs/heads/master\n    $ git show-ref; echo $?\n    e7255c8fcabc6e15f57cd984f9f117870052c1a0 refs/heads/branch\n    e7255c8fcabc6e15f57cd984f9f117870052c1a0 refs/heads/master\n    0\n    $ chmod 000 .git/refs/heads/master\n    $ git show-ref; echo $?\n    e7255c8fcabc6e15f57cd984f9f117870052c1a0 refs/heads/branch\n    0\n\nI wanted to check if this was a regression and got as far back as\nv1.4.3 with the same behavior before the commands wouldn't work\nanymore due to changes in the git config parsing code.\n"},{"id":"269597","messageId":"20150908065347.GG26331@sigill.intra.peff.net","threadId":"40290","inReplyTo":"CACBZZX6ZYDEPrQorg=pVh734ua+x55SYoKKvSZ_h0GQaR=m+8w@mail.gmail.com","subject":"Re: Bug: git-upload-pack will return successfully even when it can't read all references","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-09-08T06:53:47Z","receivedAt":"2015-09-08T06:53:47Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 07, 2015 at 02:11:15PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> This turned out to be a pretty trivial filesystem error.\n> refs/heads/master wasn't readable by the backup process, but some\n> other stuff in refs/heads and objects/* was.\n>\n> [...]\n>\n> I wanted to check if this was a regression and got as far back as\n> v1.4.3 with the same behavior before the commands wouldn't work\n> anymore due to changes in the git config parsing code.\n\nRight, it has basically always been this way. for_each_ref() silently\neats oddities or errors while reading refs. Calling for_each_rawref()\nwill include them, but we don't do it in most places; it would make\nnon-critical operations on a corrupted repo barf.  And it is difficult\nto know what is \"critical\" inside the code. You might be calling\n\"upload-pack\" to salvage what you can from a corrupted repo, or to make\na backup where you want to know what is corrupted and what is not.\n\nCommit 49672f2 introduced a \"ref paranoia\" environment variable to let\nyou specify this (and robust backups was definitely one of the use cases\nI had in mind). It's a little tricky to use with upload-pack because you\nmay be crossing an ssh boundary, but:\n\n  git clone -u 'GIT_REF_PARANOIA=1 git-upload-pack' ...\n\nshould work.\n\nWith your case:\n\n  $ git clone --no-local -u 'GIT_REF_PARANOIA=1 git-upload-pack' repo repo-checkout\n  Cloning into 'repo-checkout'...\n  fatal: git upload-pack: not our ref 0000000000000000000000000000000000000000\n  fatal: The remote end hung up unexpectedly\n\nWithout \"--no-local\" it behaves weirdly, but I would not recommend local\nclones in general if you are trying to be careful. They optimize out a\nlot of the safety checks, and we do things like copy the packed-refs\nfile wholesale.\n\nAnd certainly the error message is not the greatest. upload-pack is not\nchecking for the REF_ISBROKEN flag, so it just dumps:\n\n  0000000000000000000000000000000000000000 refs/heads/master\n\nin the advertisement, and the client happily requests that object.\nREF_PARANOIA is really just a band-aid to feed the broken refs to the\nnormal code paths, which typically barf on their own. :)\n\nSomething like this:\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 89e832b..3c621a5 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -731,6 +731,9 @@ static int send_ref(const char *refname, const struct object_id *oid,\n \tif (mark_our_ref(refname, oid))\n \t\treturn 0;\n \n+\tif (flag & REF_ISBROKEN)\n+\t\twarning(\"remote ref '%s' is broken\", refname);\n+\n \tif (capabilities) {\n \t\tstruct strbuf symref_info = STRBUF_INIT;\n \nkind of helps, but the advertisement is too early for us to send\nsideband messages. So it makes it to the user if the transport is local\nor ssh, but not over git:// or http.\n\nThat's something we could do better with protocol v2 (we'll negotiate\ncapabilities before the advertisement).\n\n-Peff\n"},{"id":"269606","messageId":"CACBZZX6Ag5pjx_AhS_aN=rvJBcy+Nh+PXfdeEqxFdgxvL3NMbw@mail.gmail.com","threadId":"40290","inReplyTo":"20150908065347.GG26331@sigill.intra.peff.net","subject":"Re: Bug: git-upload-pack will return successfully even when it can't read all references","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2015-09-08T12:16:03Z","receivedAt":"2015-09-08T12:16:03Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Tue, Sep 8, 2015 at 8:53 AM, Jeff King <peff@peff.net> wrote:\n> On Mon, Sep 07, 2015 at 02:11:15PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n>> This turned out to be a pretty trivial filesystem error.\n>> refs/heads/master wasn't readable by the backup process, but some\n>> other stuff in refs/heads and objects/* was.\n>>\n>> [...]\n>>\n>> I wanted to check if this was a regression and got as far back as\n>> v1.4.3 with the same behavior before the commands wouldn't work\n>> anymore due to changes in the git config parsing code.\n>\n> Right, it has basically always been this way. for_each_ref() silently\n> eats oddities or errors while reading refs. Calling for_each_rawref()\n> will include them, but we don't do it in most places; it would make\n> non-critical operations on a corrupted repo barf.  And it is difficult\n> to know what is \"critical\" inside the code. You might be calling\n> \"upload-pack\" to salvage what you can from a corrupted repo, or to make\n> a backup where you want to know what is corrupted and what is not.\n>\n> Commit 49672f2 introduced a \"ref paranoia\" environment variable to let\n> you specify this (and robust backups was definitely one of the use cases\n> I had in mind). It's a little tricky to use with upload-pack because you\n> may be crossing an ssh boundary, but:\n>\n>   git clone -u 'GIT_REF_PARANOIA=1 git-upload-pack' ...\n>\n> should work.\n>\n> With your case:\n>\n>   $ git clone --no-local -u 'GIT_REF_PARANOIA=1 git-upload-pack' repo repo-checkout\n>   Cloning into 'repo-checkout'...\n>   fatal: git upload-pack: not our ref 0000000000000000000000000000000000000000\n>   fatal: The remote end hung up unexpectedly\n>\n> Without \"--no-local\" it behaves weirdly, but I would not recommend local\n> clones in general if you are trying to be careful. They optimize out a\n> lot of the safety checks, and we do things like copy the packed-refs\n> file wholesale.\n>\n> And certainly the error message is not the greatest. upload-pack is not\n> checking for the REF_ISBROKEN flag, so it just dumps:\n>\n>   0000000000000000000000000000000000000000 refs/heads/master\n>\n> in the advertisement, and the client happily requests that object.\n> REF_PARANOIA is really just a band-aid to feed the broken refs to the\n> normal code paths, which typically barf on their own. :)\n>\n> Something like this:\n>\n> diff --git a/upload-pack.c b/upload-pack.c\n> index 89e832b..3c621a5 100644\n> --- a/upload-pack.c\n> +++ b/upload-pack.c\n> @@ -731,6 +731,9 @@ static int send_ref(const char *refname, const struct object_id *oid,\n>         if (mark_our_ref(refname, oid))\n>                 return 0;\n>\n> +       if (flag & REF_ISBROKEN)\n> +               warning(\"remote ref '%s' is broken\", refname);\n> +\n>         if (capabilities) {\n>                 struct strbuf symref_info = STRBUF_INIT;\n>\n> kind of helps, but the advertisement is too early for us to send\n> sideband messages. So it makes it to the user if the transport is local\n> or ssh, but not over git:// or http.\n>\n> That's something we could do better with protocol v2 (we'll negotiate\n> capabilities before the advertisement).\n\nFantastic. REF_PARANOIA does exactly what I need, i.e. stall the fetch\nprocess so permissions can be manually repaired.\n\nI think it makes sense to keep the default at \"let's try to copy over\nwhat we can\", for salvage purposes. I think the bug is that we still\nreturn success in that case, and should return non-zero, but as you\npoint out this is easier said than done due to needing to deal with\nthe case where the remote transport sends us the 0000... ref.\n\nI wonder if --upload-pack=\"GIT_REF_PARANOIA=1 git-upload-pack\" should\nbe the default when running fetch if you have --prune enabled. There's\na particularly bad edge case now where if you have permission errors\non the master repository and run --prune on your backup along with a\n--mirror clone to mirror the refs, then when you have permission\nissues you'll prune everything from the backup.\n\nBut yeah, a proper fix needs protocol v2. Because among other things\nthat --upload-pack hack will only work for ssh, not http.\n"},{"id":"269632","messageId":"20150908220611.GE24159@sigill.intra.peff.net","threadId":"40290","inReplyTo":"CACBZZX6Ag5pjx_AhS_aN=rvJBcy+Nh+PXfdeEqxFdgxvL3NMbw@mail.gmail.com","subject":"Re: Bug: git-upload-pack will return successfully even when it can't read all references","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-09-08T22:06:11Z","receivedAt":"2015-09-08T22:06:11Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 08, 2015 at 02:16:03PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> I wonder if --upload-pack=\"GIT_REF_PARANOIA=1 git-upload-pack\" should\n> be the default when running fetch if you have --prune enabled. There's\n> a particularly bad edge case now where if you have permission errors\n> on the master repository and run --prune on your backup along with a\n> --mirror clone to mirror the refs, then when you have permission\n> issues you'll prune everything from the backup.\n> \n> But yeah, a proper fix needs protocol v2. Because among other things\n> that --upload-pack hack will only work for ssh, not http.\n\nRight. There is no real way to flip the flag from the client side,\nbecause we cannot reliably communicate it. Once we have such a\nmechanism, it might actually make sense to _always_ flip on paranoia.\nThat is, we would rather an operation fail and be retried with in a\n\"loose\" mode explicitly. That's safer. It's less convenient when you\nhave a broken repo, but surely that's the exception, right?\n\n-Peff\n"}]}