{"thread":{"id":"45324","subject":"More about blob reachability (while fetching arbitrary blobs)","startedAt":"2017-03-09T00:36:34Z","lastAt":"2017-03-09T00:36:34Z","messageCount":1,"participants":["Jonathan Tan"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"313584","messageId":"20170309003547.6930-1-jonathantanmy@google.com","threadId":"45324","inReplyTo":null,"subject":"More about blob reachability (while fetching arbitrary blobs)","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-03-09T00:35:47Z","receivedAt":"2017-03-09T00:36:34Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"There has been some talk about fetching blobs from repos with missing\nobjects [1] [2]. I took a further look at the issue of blob\nreachability.\n\nI have written earlier that there is a bug in rev-list when used to\ncompute reachability [3], but someone pointed me to bitmaps, and I found\nout that \"--use-bitmap-index\" makes rev-list use an alternative code\npath in the presence of a bitmap (whether fully covering or partially\ncovering the relevant objects), and that code doesn't contain that bug.\nIf reachability checks are desired, bitmaps are probably required for\nperformance anyway [4].\n\nWe should implement things so that users will be strongly recommended to\ndo one of the following:\n 1) configure explicitly (instead of it being the default) the\n    equivalent of allowanysha1inwant to disable reachability checks, or\n 2) use a bitmap (and it is recommended to periodically repack).\n\n(This strong recommendation is currently a requirement due to the fact\nthat the 3rd option, \"do nothing\", is not possible due to the bug in\nrev-list described above.)\n\nI have included a small patch (below) to do that for upload-pack. (For\nthe case of upload-pack, which until now does not really need to process\nblob wants, a possible alternative is to forbid trees and blobs from\nbeing processed by upload-pack at all.)\n\n[1] <20170304191901.9622-1-markbt@efaref.net>\n[2] <1488999039-37631-1-git-send-email-git@jeffhostetler.com>\n[3] <cover.1487984670.git.jonathantanmy@google.com>\n[4] I checked, and got 0m0.281s for \"time git rev-list --objects\n    --use-bitmap-index HEAD^{tree} --not --all\" on a fully repacked\n    git.git. (Not sure whether that's OK performance.)\n\n-- 8< --\nSubject: [PATCH] upload-pack: forbid fetching unreachable blobs\n\nIf allowreachablesha1inwant is set, upload-pack will provide a blob to a\nuser, provided its hash, regardless of whether the blob is reachable or\nnot.\n\nPartially fix this by (i) including the \"--objects\" argument, if\nnecessary, when invoking \"rev-list\", and (ii) including the\n\"--use-bitmap-index\" argument.\n\n(i) is so that rev-list operates on the correct granularity, depending\non the type of objects checked.\n\n(ii) needs more explanation:\n\n  Our invocation of \"rev-list\" is supposed to return `A - B`, where `A`\n  is the set of all objects reachable from the objects that we want to\n  check the reachability of, and `B` is the set of all objects reachable\n  from any of our refs. If all our objects are reachable, then the\n  resulting set is empty.  However, the non-bitmap-using part of the\n  code wrongly excludes some trees and blobs from `B`, making the\n  resulting set sometimes non-empty and thus wrongly concluding that an\n  object is unreachable when it is, in fact, reachable.  (However, the\n  error does not run the other way - an object will not be indicated as\n  reachable when it is unreachable.)\n\n  The \"--use-bitmap-index\" argument partially solves this problem by\n  activating another code path when the serving repository has a bitmap.\n  This other code path does not contain the bug described above.\n\n(\"--use-bitmap-index\" has other benefits besides correctness, but has\nnot yet been included likely because bitmaps were introduced in commit\nfff4275 (\"pack-bitmap: add support for bitmap indexes\", 2013-12-21),\nlater than the last time this part of the code was touched in commit\n051e400 (\"helping smart-http/stateless-rpc fetch race\", 2011-08-05)).\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n t/t5500-fetch-pack.sh | 31 +++++++++++++++++++++++++++++++\n upload-pack.c         | 17 ++++++++++++++++-\n 2 files changed, 47 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex 505e1b4a7..1568aed82 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -547,6 +547,37 @@ test_expect_success 'fetch-pack can fetch a raw sha1' '\n \tgit fetch-pack hidden $(git -C hidden rev-parse refs/hidden/one)\n '\n \n+test_expect_success 'setup for tests that fetch blobs by hash' '\n+\tgit init blobserver &&\n+\ttest_commit -C blobserver 1 &&\n+\tgit -C blobserver repack -a -d --write-bitmap-index &&\n+\ttest_commit -C blobserver 2 &&\n+\ttest_commit -C blobserver 3 &&\n+\tblob1=$(echo 1 | git hash-object --stdin) &&\n+\tblob2=$(echo 2 | git hash-object --stdin) &&\n+\tblob3=$(echo 3 | git hash-object --stdin) &&\n+\n+\tunreachable=$(echo 4 | git -C blobserver hash-object -w --stdin) &&\n+\tgit -C blobserver cat-file -e \"$unreachable\"\n+'\n+\n+test_expect_success 'fetch-pack can fetch reachable blobs by hash' '\n+\ttest_config -C blobserver uploadpack.allowreachablesha1inwant 1 &&\n+\n+\tgit init reachabletest &&\n+\tgit -C reachabletest fetch-pack ../blobserver \"$blob1\" \"$blob2\" &&\n+\tgit -C reachabletest cat-file -e \"$blob1\" &&\n+\tgit -C reachabletest cat-file -e \"$blob2\" &&\n+\ttest_must_fail git -C reachabletest cat-file -e \"$blob3\"\n+'\n+\n+test_expect_success 'fetch-pack cannot fetch unreachable blobs' '\n+\ttest_config -C blobserver uploadpack.allowreachablesha1inwant 1 &&\n+\n+\tgit init unreachabletest &&\n+\ttest_must_fail git -C unreachabletest fetch-pack ../blobserver \"$blob1\" \"$unreachable\"\n+'\n+\n check_prot_path () {\n \tcat >expected <<-EOF &&\n \tDiag: url=$1\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 7597ba340..a00f0034c 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -469,7 +469,10 @@ static int do_reachable_revlist(struct child_process *cmd,\n \t\t\t\tstruct object_array *reachable)\n {\n \tstatic const char *argv[] = {\n-\t\t\"rev-list\", \"--stdin\", NULL,\n+\t\t\"rev-list\", \"--use-bitmap-index\", \"--stdin\", NULL,\n+\t};\n+\tstatic const char *argv_with_objects[] = {\n+\t\t\"rev-list\", \"--use-bitmap-index\", \"--objects\", \"--stdin\", NULL,\n \t};\n \tstruct object *o;\n \tchar namebuf[42]; /* ^ + SHA-1 + LF */\n@@ -488,6 +491,18 @@ static int do_reachable_revlist(struct child_process *cmd,\n \t */\n \tsigchain_push(SIGPIPE, SIG_IGN);\n \n+\t/*\n+\t * If we are testing reachability of a tree or blob, rev-list needs to\n+\t * operate more granularly.\n+\t */\n+\tfor (i = 0; i < src->nr; i++) {\n+\t\to = src->objects[i].item;\n+\t\tif (o->type == OBJ_TREE || o->type == OBJ_BLOB) {\n+\t\t\tcmd->argv = argv_with_objects;\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+\n \tif (start_command(cmd))\n \t\tgoto error;\n \n-- \n2.12.0.246.ga2ecc84866-goog\n\n"}]}