{"thread":{"id":"37405","subject":"[PATCH 1/5] git-prompt: do not look for refs/stash in $GIT_DIR","startedAt":"2014-08-23T05:23:34Z","lastAt":"2014-08-25T18:56:20Z","messageCount":14,"participants":["Jeff King","Michael Haggerty","Ronnie Sahlberg"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"248171","messageId":"20140823052334.GA17813@peff.net","threadId":"37405","inReplyTo":null,"subject":"[PATCH 0/5] fix pack-refs pruning of refs/foo","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-08-23T05:23:34Z","receivedAt":"2014-08-23T05:23:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I noticed that \"git pack-refs --all\" will pack a top-level ref like\n\"refs/foo\", but will not actually prune \"$GIT_DIR/refs/foo\". I do not\nsee the point in having a policy not to pack \"refs/foo\" if \"--all\" is\ngiven. But even if we did have such a policy, this seems broken; we\nshould either pack and prune, or leave them untouched. I don't see any\nindication that the existing behavior was intentional.\n\nThe problem is that pack-refs's prune_ref calls lock_ref_sha1, which\nenforces this \"no toplevel\" behavior. I am not sure there is any real\npoint to this, given that most callers use lock_any_ref_for_update,\nwhich is exactly equivalent but without the toplevel check.\n\nThe first two patches deal with this by switching pack-refs to use\nlock_any_ref_for_update. This will conflict slightly with Ronnie's\nref-transaction work, as he gets rid of lock_ref_sha1 entirely, and\nmoves the code directly into prune_ref. This can be trivially resolved\nin favor of my patch, I think.\n\nThe third patch is a cleanup I noticed while looking around, and I think\nshould not conflict with anyone (and is a good thing to do).\n\nThe last two are trickier. I wondered if we could get rid of\nlock_ref_sha1 entirely. After pack-refs, there are two callers:\nfast-import.c and walker.c. After converting the first, it occurred to\nme that Ronnie might be touching the same areas, and I see that yes,\nindeed, there's quite a bit of conflict (and he reaches the same end\ngoal of dropping it entirely).\n\nSo in that sense I do not mind dropping the final two patches. Ronnie's\nendpoint is much better, moving to a ref_transaction. However, there is\nactually a buffer overflow in the existing code. Ronnie's series fixes\nit in a similar way (moving to a strbuf), and I'm fine with that\nendpoint. But given that the ref transaction code is not yet merged (and\nwould certainly never be maint-track), I think it is worth applying the\nbuffer overflow fix separately.\n\nI think the final patch can probably be dropped, then. It is a clean-up,\nbut one that we can just get when Ronnie's series is merged.\n\n  [1/5]: git-prompt: do not look for refs/stash in $GIT_DIR\n  [2/5]: pack-refs: prune top-level refs like \"refs/foo\"\n  [3/5]: fast-import: clean up pack_data pointer in end_packfile\n  [4/5]: fast-import: fix buffer overflow in dump_tags\n  [5/5]: fast-import: stop using lock_ref_sha1\n\n-Peff\n"},{"id":"248170","messageId":"20140823052650.GA18075@peff.net","threadId":"37405","inReplyTo":"20140823052334.GA17813@peff.net","subject":"[PATCH 1/5] git-prompt: do not look for refs/stash in $GIT_DIR","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-08-23T05:26:51Z","receivedAt":"2014-08-23T05:26:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Since dd0b72c (bash prompt: use bash builtins to check stash\nstate, 2011-04-01), git-prompt checks whether we have a\nstash by looking for $GIT_DIR/refs/stash. Generally external\nprograms should never do this, because they would miss\npacked-refs.\n\nThat commit claims that packed-refs does not pack\nrefs/stash, but that is not quite true. It does pack the\nref, but due to a bug, fails to prune the ref. When we fix\nthat bug, we would want to be doing the right thing here.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI know we are pretty sensitive to forks in the prompt code (after all,\nthat was the point of dd0b72c). This patch is essentially a reversion of\nthis hunk of dd0b72c, and is definitely safe.\n\nI think we could probably get by with:\n\n  [ -r \"$g/logs/ref/stash\" ]\n\nsince reflogs are always in the filesystem. However, that seems somewhat\nshort-sighted, as the work Ronnie is doing is moving in the direction of\nmore abstraction here. I hope a day will come soon when reflogs do not\nhave to be stored in $GIT_DIR/logs, and then we would end up updating\nthis code again.\n\n contrib/completion/git-prompt.sh | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh\nindex 9d684b1..c5473dc 100644\n--- a/contrib/completion/git-prompt.sh\n+++ b/contrib/completion/git-prompt.sh\n@@ -468,7 +468,8 @@ __git_ps1 ()\n \t\t\tfi\n \t\tfi\n \t\tif [ -n \"${GIT_PS1_SHOWSTASHSTATE-}\" ] &&\n-\t\t   [ -r \"$g/refs/stash\" ]; then\n+\t\t   git rev-parse --verify --quiet refs/stash >/dev/null\n+\t\tthen\n \t\t\ts=\"$\"\n \t\tfi\n \n-- \n2.1.0.346.ga0367b9\n"},{"id":"248172","messageId":"20140823052707.GB18075@peff.net","threadId":"37405","inReplyTo":"20140823052334.GA17813@peff.net","subject":"[PATCH 2/5] pack-refs: prune top-level refs like \"refs/foo\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-08-23T05:27:07Z","receivedAt":"2014-08-23T05:27:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"After we have packed all refs, we prune any loose refs that\ncorrespond to what we packed. We do so by first taking a\nlock with lock_ref_sha1, and then deleting the loose ref\nfile.\n\nHowever, lock_ref_sha1 will refuse to take a lock on any\nrefs that exist at the top-level of the \"refs/\" directory,\nand we skip pruning the ref.  This is almost certainly not\nwhat we want to happen here. The criteria to be pruned\nshould not differ from that to be packed; if a ref makes it\nto prune_ref, it's because we want it both packed and\npruned (if there are refs you do not want to be packed, they\nshould be omitted much earlier by pack_ref_is_possible,\nwhich we do in this case if --all is not given).\n\nWe can fix this by switching to lock_any_ref_for_update.\nThis behaves exactly the same with the exception of this\ntop-level check.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n refs.c               | 3 ++-\n t/t3210-pack-refs.sh | 7 +++++++\n 2 files changed, 9 insertions(+), 1 deletion(-)\n\ndiff --git a/refs.c b/refs.c\nindex 27927f2..82e5b1b 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2387,7 +2387,8 @@ static void try_remove_empty_parents(char *name)\n /* make sure nobody touched the ref, and unlink */\n static void prune_ref(struct ref_to_prune *r)\n {\n-\tstruct ref_lock *lock = lock_ref_sha1(r->name + 5, r->sha1);\n+\tstruct ref_lock *lock = lock_any_ref_for_update(r->name, r->sha1,\n+\t\t\t\t\t\t\t0, NULL);\n \n \tif (lock) {\n \t\tunlink_or_warn(git_path(\"%s\", r->name));\ndiff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh\nindex 1a2080e..3a017bf 100755\n--- a/t/t3210-pack-refs.sh\n+++ b/t/t3210-pack-refs.sh\n@@ -151,4 +151,11 @@ test_expect_success 'delete ref while another dangling packed ref' '\n \ttest_cmp /dev/null result\n '\n \n+test_expect_success 'pack ref directly below refs/' '\n+\tgit update-ref refs/top HEAD &&\n+\tgit pack-refs --all --prune &&\n+\tgrep refs/top .git/packed-refs &&\n+\ttest_path_is_missing .git/refs/top\n+'\n+\n test_done\n-- \n2.1.0.346.ga0367b9\n"},{"id":"248173","messageId":"20140823052741.GC18075@peff.net","threadId":"37405","inReplyTo":"20140823052334.GA17813@peff.net","subject":"[PATCH 3/5] fast-import: clean up pack_data pointer in end_packfile","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-08-23T05:27:41Z","receivedAt":"2014-08-23T05:27:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We have a global pointer pack_data pointing to the current\npack we have open. Inside end_packfile we have two new\npointers, old_p and new_p. The latter points to pack_data,\nand the former points to the new \"installed\" version of the\npackfile we get when we hand the file off to the regular\nsha1_file machinery. When then free old_p.\n\nPresumably the extra old_p pointer was there so that we\ncould overwrite pack_data with new_p and still free old_p,\nbut we don't do that. We just leave pack_data pointing to\nbogus memory, and don't overwrite it until we call\nstart_packfile again (if ever).\n\nThis can cause problems for our die routine, which calls\nend_packfile to clean things up. If we die at the wrong\nmoment, we can end up looking at invalid memory in\npack_data left after the last end_packfile().\n\nInstead, let's make sure we set pack_data to NULL after we\nfree it, and make calling endfile() again with a NULL\npack_data a noop (there is nothing to end).\n\nWe can further make things less confusing by dropping old_p\nentirely, and moving new_p closer to its point of use.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nNoticed while running fast-import under valgrind to debug the next\ncommit. :)\n\n fast-import.c | 11 +++++++----\n 1 file changed, 7 insertions(+), 4 deletions(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex d73f58c..f25a4ae 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -946,10 +946,12 @@ static void unkeep_all_packs(void)\n \n static void end_packfile(void)\n {\n-\tstruct packed_git *old_p = pack_data, *new_p;\n+\tif (!pack_data)\n+\t\treturn;\n \n \tclear_delta_base_cache();\n \tif (object_count) {\n+\t\tstruct packed_git *new_p;\n \t\tunsigned char cur_pack_sha1[20];\n \t\tchar *idx_name;\n \t\tint i;\n@@ -991,10 +993,11 @@ static void end_packfile(void)\n \t\tpack_id++;\n \t}\n \telse {\n-\t\tclose(old_p->pack_fd);\n-\t\tunlink_or_warn(old_p->pack_name);\n+\t\tclose(pack_data->pack_fd);\n+\t\tunlink_or_warn(pack_data->pack_name);\n \t}\n-\tfree(old_p);\n+\tfree(pack_data);\n+\tpack_data = NULL;\n \n \t/* We can't carry a delta across packfiles. */\n \tstrbuf_release(&last_blob.data);\n-- \n2.1.0.346.ga0367b9\n"},{"id":"248174","messageId":"20140823053237.GD18075@peff.net","threadId":"37405","inReplyTo":"20140823052334.GA17813@peff.net","subject":"[PATCH 4/5] fast-import: fix buffer overflow in dump_tags","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-08-23T05:32:37Z","receivedAt":"2014-08-23T05:32:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When creating a new annotated tag, we sprintf the refname\ninto a static-sized buffer. If we have an absurdly long\ntagname, like:\n\n  git init repo &&\n  cd repo &&\n  git commit --allow-empty -m foo &&\n  git tag -m message mytag &&\n  git fast-export mytag |\n  perl -lpe '/^tag/ and s/mytag/\"a\" x 8192/e' |\n  git fast-import <input\n\nwe'll overflow the buffer. We can fix it by using a strbuf.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI'm not sure how easily exploitable this is. The buffer is on the stack,\nand we definitely demolish the return address. But we never actually\nreturn from the function, since lock_ref_sha1 will fail in such a case\nand die (it cannot succeed because the name is longer than PATH_MAX,\nwhich we check when concatenating it with $GIT_DIR).\n\nStill, there is no limit to the size of buffer you can feed it, so it's\nentirely possible you can overwrite something else and cause some\nmischief. So I wouldn't call it trivially exploitable, but I would not\nbet my servers that it is not (and of course it is easy to trigger if\nyou can convince somebody to run fast-import a stream, so any remote\nhelpers produce a potentially vulnerable situation).\n\n fast-import.c | 10 ++++++----\n 1 file changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex f25a4ae..a1479e9 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1734,14 +1734,16 @@ static void dump_tags(void)\n \tstatic const char *msg = \"fast-import\";\n \tstruct tag *t;\n \tstruct ref_lock *lock;\n-\tchar ref_name[PATH_MAX];\n+\tstruct strbuf ref_name = STRBUF_INIT;\n \n \tfor (t = first_tag; t; t = t->next_tag) {\n-\t\tsprintf(ref_name, \"tags/%s\", t->name);\n-\t\tlock = lock_ref_sha1(ref_name, NULL);\n+\t\tstrbuf_reset(&ref_name);\n+\t\tstrbuf_addf(&ref_name, \"tags/%s\", t->name);\n+\t\tlock = lock_ref_sha1(ref_name.buf, NULL);\n \t\tif (!lock || write_ref_sha1(lock, t->sha1, msg) < 0)\n-\t\t\tfailure |= error(\"Unable to update %s\", ref_name);\n+\t\t\tfailure |= error(\"Unable to update %s\", ref_name.buf);\n \t}\n+\tstrbuf_release(&ref_name);\n }\n \n static void dump_marks_helper(FILE *f,\n-- \n2.1.0.346.ga0367b9\n"},{"id":"248175","messageId":"20140823053322.GE18075@peff.net","threadId":"37405","inReplyTo":"20140823052334.GA17813@peff.net","subject":"[PATCH 5/5] fast-import: stop using lock_ref_sha1","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-08-23T05:33:22Z","receivedAt":"2014-08-23T05:33:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We can use lock_any_ref_for_update instead. Besides being\nmore flexible, the only difference between the two is that\nlock_ref_sha1 does not allow \"top-level\" refs like\n\"refs/foo\" to be updated. However, we know that we do not\nhave such a ref, because we explicitly add the \"tags/\"\nprefix ourselves.\n\nNote that we now must feed the whole name \"refs/tags/X\"\ninstead of just \"tags/X\" to the function. As a result, our\nfailure error message is uses the longer name. This is\nprobably a good thing, though.\n\nAs an interesting side note, if we forgot to switch this\ninput to the function, the tests do not currently catch it.\nWe import a tag \"X\" and then check that we can access it at\n\"tags/X\". If we accidentally created \"tags/X\" at the\ntop-level $GIT_DIR instead of under \"refs/\", we would still\nfind it due to our ref lookup procedure!\n\nWe do not make such a mistake in this patch, of course, but\nwhile we're thinking about it, let's make the fast-import\ntests more robust by checking for fully qualified\nrefnames.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nAs I mentioned, I'd be OK with dropping this one in favor of just\nwaiting for Ronnie's transaction patches to graduate.\n\n fast-import.c          | 4 ++--\n t/t9300-fast-import.sh | 8 ++++----\n 2 files changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex a1479e9..04a85a4 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1738,8 +1738,8 @@ static void dump_tags(void)\n \n \tfor (t = first_tag; t; t = t->next_tag) {\n \t\tstrbuf_reset(&ref_name);\n-\t\tstrbuf_addf(&ref_name, \"tags/%s\", t->name);\n-\t\tlock = lock_ref_sha1(ref_name.buf, NULL);\n+\t\tstrbuf_addf(&ref_name, \"refs/tags/%s\", t->name);\n+\t\tlock = lock_any_ref_for_update(ref_name.buf, NULL, 0, NULL);\n \t\tif (!lock || write_ref_sha1(lock, t->sha1, msg) < 0)\n \t\t\tfailure |= error(\"Unable to update %s\", ref_name.buf);\n \t}\ndiff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\nindex 5fc9ef2..f4c6673 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -153,7 +153,7 @@ tag series-A\n An annotated tag without a tagger\n EOF\n test_expect_success 'A: verify tag/series-A' '\n-\tgit cat-file tag tags/series-A >actual &&\n+\tgit cat-file tag refs/tags/series-A >actual &&\n \ttest_cmp expect actual\n '\n \n@@ -165,7 +165,7 @@ tag series-A-blob\n An annotated tag that annotates a blob.\n EOF\n test_expect_success 'A: verify tag/series-A-blob' '\n-\tgit cat-file tag tags/series-A-blob >actual &&\n+\tgit cat-file tag refs/tags/series-A-blob >actual &&\n \ttest_cmp expect actual\n '\n \n@@ -232,8 +232,8 @@ EOF\n test_expect_success \\\n \t'A: tag blob by sha1' \\\n \t'git fast-import <input &&\n-\tgit cat-file tag tags/series-A-blob-2 >actual &&\n-\tgit cat-file tag tags/series-A-blob-3 >>actual &&\n+\tgit cat-file tag refs/tags/series-A-blob-2 >actual &&\n+\tgit cat-file tag refs/tags/series-A-blob-3 >>actual &&\n \ttest_cmp expect actual'\n \n test_tick\n-- \n2.1.0.346.ga0367b9\n"},{"id":"248180","messageId":"53F842E0.8000603@alum.mit.edu","threadId":"37405","inReplyTo":"20140823052334.GA17813@peff.net","subject":"Re: [PATCH 0/5] fix pack-refs pruning of refs/foo","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-08-23T07:29:36Z","receivedAt":"2014-08-23T07:29:36Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 08/23/2014 07:23 AM, Jeff King wrote:\n> I noticed that \"git pack-refs --all\" will pack a top-level ref like\n> \"refs/foo\", but will not actually prune \"$GIT_DIR/refs/foo\". I do not\n> see the point in having a policy not to pack \"refs/foo\" if \"--all\" is\n> given. But even if we did have such a policy, this seems broken; we\n> should either pack and prune, or leave them untouched. I don't see any\n> indication that the existing behavior was intentional.\n> \n> The problem is that pack-refs's prune_ref calls lock_ref_sha1, which\n> enforces this \"no toplevel\" behavior. I am not sure there is any real\n> point to this, given that most callers use lock_any_ref_for_update,\n> which is exactly equivalent but without the toplevel check.\n> \n> The first two patches deal with this by switching pack-refs to use\n> lock_any_ref_for_update. This will conflict slightly with Ronnie's\n> ref-transaction work, as he gets rid of lock_ref_sha1 entirely, and\n> moves the code directly into prune_ref. This can be trivially resolved\n> in favor of my patch, I think.\n> \n> The third patch is a cleanup I noticed while looking around, and I think\n> should not conflict with anyone (and is a good thing to do).\n> \n> The last two are trickier. I wondered if we could get rid of\n> lock_ref_sha1 entirely. After pack-refs, there are two callers:\n> fast-import.c and walker.c. After converting the first, it occurred to\n> me that Ronnie might be touching the same areas, and I see that yes,\n> indeed, there's quite a bit of conflict (and he reaches the same end\n> goal of dropping it entirely).\n> \n> So in that sense I do not mind dropping the final two patches. Ronnie's\n> endpoint is much better, moving to a ref_transaction. However, there is\n> actually a buffer overflow in the existing code. Ronnie's series fixes\n> it in a similar way (moving to a strbuf), and I'm fine with that\n> endpoint. But given that the ref transaction code is not yet merged (and\n> would certainly never be maint-track), I think it is worth applying the\n> buffer overflow fix separately.\n> \n> I think the final patch can probably be dropped, then. It is a clean-up,\n> but one that we can just get when Ronnie's series is merged.\n> \n>   [1/5]: git-prompt: do not look for refs/stash in $GIT_DIR\n>   [2/5]: pack-refs: prune top-level refs like \"refs/foo\"\n>   [3/5]: fast-import: clean up pack_data pointer in end_packfile\n>   [4/5]: fast-import: fix buffer overflow in dump_tags\n>   [5/5]: fast-import: stop using lock_ref_sha1\n\n+1 on patches 1 and 2\nPatch 3 is outside of my area of competence\n+1 on patch 4, which looks trivially correct.\n+1 on patch 5, though I agree with peff that it can be omitted in\ndeference to Ronnie's work.\n\nBy the way, while cleaning up in patch 5 you might take the chance to\nrename the local variable ref_name to refname to be consistent with most\nof our code, but this is by no means required.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"248181","messageId":"20140823074611.GA23670@peff.net","threadId":"37405","inReplyTo":"53F842E0.8000603@alum.mit.edu","subject":"Re: [PATCH 0/5] fix pack-refs pruning of refs/foo","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-08-23T07:46:11Z","receivedAt":"2014-08-23T07:46:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Aug 23, 2014 at 09:29:36AM +0200, Michael Haggerty wrote:\n\n> >   [1/5]: git-prompt: do not look for refs/stash in $GIT_DIR\n> >   [2/5]: pack-refs: prune top-level refs like \"refs/foo\"\n> >   [3/5]: fast-import: clean up pack_data pointer in end_packfile\n> >   [4/5]: fast-import: fix buffer overflow in dump_tags\n> >   [5/5]: fast-import: stop using lock_ref_sha1\n> \n> +1 on patches 1 and 2\n> Patch 3 is outside of my area of competence\n> +1 on patch 4, which looks trivially correct.\n> +1 on patch 5, though I agree with peff that it can be omitted in\n> deference to Ronnie's work.\n\nThanks.\n\n> By the way, while cleaning up in patch 5 you might take the chance to\n> rename the local variable ref_name to refname to be consistent with most\n> of our code, but this is by no means required.\n\nI had the exact same inclination, but dismissed it as me being too\npicky. :)\n\nI'll change it if I re-roll, but I think we'll end up just dropping that\npatch entirely.\n\n-Peff\n"},{"id":"248237","messageId":"CAL=YDW=-9Kkv4+Vp9+=TWNia0wcgq5ubNDgELvcxUvvuqFiJRw@mail.gmail.com","threadId":"37405","inReplyTo":"20140823053237.GD18075@peff.net","subject":"Re: [PATCH 4/5] fast-import: fix buffer overflow in dump_tags","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-08-25T17:11:06Z","receivedAt":"2014-08-25T17:11:06Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Jeff,\nWe have a fix like this in the next set of transaction updates.\nhttps://code-review.googlesource.com/#/c/1012/13/fast-import.c\n\nHowever, if your concerns are the integrity of the servers and not\ntaking any chances\nyou might not want to wait for my patches to graduate.\n\n\nronnie sahlberg\n\nOn Fri, Aug 22, 2014 at 10:32 PM, Jeff King <peff@peff.net> wrote:\n> When creating a new annotated tag, we sprintf the refname\n> into a static-sized buffer. If we have an absurdly long\n> tagname, like:\n>\n>   git init repo &&\n>   cd repo &&\n>   git commit --allow-empty -m foo &&\n>   git tag -m message mytag &&\n>   git fast-export mytag |\n>   perl -lpe '/^tag/ and s/mytag/\"a\" x 8192/e' |\n>   git fast-import <input\n>\n> we'll overflow the buffer. We can fix it by using a strbuf.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> I'm not sure how easily exploitable this is. The buffer is on the stack,\n> and we definitely demolish the return address. But we never actually\n> return from the function, since lock_ref_sha1 will fail in such a case\n> and die (it cannot succeed because the name is longer than PATH_MAX,\n> which we check when concatenating it with $GIT_DIR).\n>\n> Still, there is no limit to the size of buffer you can feed it, so it's\n> entirely possible you can overwrite something else and cause some\n> mischief. So I wouldn't call it trivially exploitable, but I would not\n> bet my servers that it is not (and of course it is easy to trigger if\n> you can convince somebody to run fast-import a stream, so any remote\n> helpers produce a potentially vulnerable situation).\n>\n>  fast-import.c | 10 ++++++----\n>  1 file changed, 6 insertions(+), 4 deletions(-)\n>\n> diff --git a/fast-import.c b/fast-import.c\n> index f25a4ae..a1479e9 100644\n> --- a/fast-import.c\n> +++ b/fast-import.c\n> @@ -1734,14 +1734,16 @@ static void dump_tags(void)\n>         static const char *msg = \"fast-import\";\n>         struct tag *t;\n>         struct ref_lock *lock;\n> -       char ref_name[PATH_MAX];\n> +       struct strbuf ref_name = STRBUF_INIT;\n>\n>         for (t = first_tag; t; t = t->next_tag) {\n> -               sprintf(ref_name, \"tags/%s\", t->name);\n> -               lock = lock_ref_sha1(ref_name, NULL);\n> +               strbuf_reset(&ref_name);\n> +               strbuf_addf(&ref_name, \"tags/%s\", t->name);\n> +               lock = lock_ref_sha1(ref_name.buf, NULL);\n>                 if (!lock || write_ref_sha1(lock, t->sha1, msg) < 0)\n> -                       failure |= error(\"Unable to update %s\", ref_name);\n> +                       failure |= error(\"Unable to update %s\", ref_name.buf);\n>         }\n> +       strbuf_release(&ref_name);\n>  }\n>\n>  static void dump_marks_helper(FILE *f,\n> --\n> 2.1.0.346.ga0367b9\n>\n"},{"id":"248239","messageId":"CAL=YDWmC9ncuH=fLR=8PWuB=kJeRP-O3K9MCKGaDfoxF6WWb8A@mail.gmail.com","threadId":"37405","inReplyTo":"20140823052741.GC18075@peff.net","subject":"Re: [PATCH 3/5] fast-import: clean up pack_data pointer in end_packfile","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-08-25T17:16:52Z","receivedAt":"2014-08-25T17:16:52Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Print an error before returning when pack_data is NULL ?\n\nOtherwise, LGTM\n\nOn Fri, Aug 22, 2014 at 10:27 PM, Jeff King <peff@peff.net> wrote:\n> We have a global pointer pack_data pointing to the current\n> pack we have open. Inside end_packfile we have two new\n> pointers, old_p and new_p. The latter points to pack_data,\n> and the former points to the new \"installed\" version of the\n> packfile we get when we hand the file off to the regular\n> sha1_file machinery. When then free old_p.\n>\n> Presumably the extra old_p pointer was there so that we\n> could overwrite pack_data with new_p and still free old_p,\n> but we don't do that. We just leave pack_data pointing to\n> bogus memory, and don't overwrite it until we call\n> start_packfile again (if ever).\n>\n> This can cause problems for our die routine, which calls\n> end_packfile to clean things up. If we die at the wrong\n> moment, we can end up looking at invalid memory in\n> pack_data left after the last end_packfile().\n>\n> Instead, let's make sure we set pack_data to NULL after we\n> free it, and make calling endfile() again with a NULL\n> pack_data a noop (there is nothing to end).\n>\n> We can further make things less confusing by dropping old_p\n> entirely, and moving new_p closer to its point of use.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> Noticed while running fast-import under valgrind to debug the next\n> commit. :)\n>\n>  fast-import.c | 11 +++++++----\n>  1 file changed, 7 insertions(+), 4 deletions(-)\n>\n> diff --git a/fast-import.c b/fast-import.c\n> index d73f58c..f25a4ae 100644\n> --- a/fast-import.c\n> +++ b/fast-import.c\n> @@ -946,10 +946,12 @@ static void unkeep_all_packs(void)\n>\n>  static void end_packfile(void)\n>  {\n> -       struct packed_git *old_p = pack_data, *new_p;\n> +       if (!pack_data)\n> +               return;\n>\n>         clear_delta_base_cache();\n>         if (object_count) {\n> +               struct packed_git *new_p;\n>                 unsigned char cur_pack_sha1[20];\n>                 char *idx_name;\n>                 int i;\n> @@ -991,10 +993,11 @@ static void end_packfile(void)\n>                 pack_id++;\n>         }\n>         else {\n> -               close(old_p->pack_fd);\n> -               unlink_or_warn(old_p->pack_name);\n> +               close(pack_data->pack_fd);\n> +               unlink_or_warn(pack_data->pack_name);\n>         }\n> -       free(old_p);\n> +       free(pack_data);\n> +       pack_data = NULL;\n>\n>         /* We can't carry a delta across packfiles. */\n>         strbuf_release(&last_blob.data);\n> --\n> 2.1.0.346.ga0367b9\n>\n"},{"id":"248242","messageId":"CAL=YDW=k4QpejdqPgnVy+05hjsVHw35BBbKmsAEOP-9gBMjCJw@mail.gmail.com","threadId":"37405","inReplyTo":"20140823053322.GE18075@peff.net","subject":"Re: [PATCH 5/5] fast-import: stop using lock_ref_sha1","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-08-25T17:22:48Z","receivedAt":"2014-08-25T17:22:48Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"The next ref transaction series does a similar change and ends up\nremoving the function lock_ref_sha1() completely.\nhttps://code-review.googlesource.com/#/c/1017/19/refs.c\n\nSo I think we can drop this patch.\n\n\nronnie sahlberg\n\n\nOn Fri, Aug 22, 2014 at 10:33 PM, Jeff King <peff@peff.net> wrote:\n> We can use lock_any_ref_for_update instead. Besides being\n> more flexible, the only difference between the two is that\n> lock_ref_sha1 does not allow \"top-level\" refs like\n> \"refs/foo\" to be updated. However, we know that we do not\n> have such a ref, because we explicitly add the \"tags/\"\n> prefix ourselves.\n>\n> Note that we now must feed the whole name \"refs/tags/X\"\n> instead of just \"tags/X\" to the function. As a result, our\n> failure error message is uses the longer name. This is\n> probably a good thing, though.\n>\n> As an interesting side note, if we forgot to switch this\n> input to the function, the tests do not currently catch it.\n> We import a tag \"X\" and then check that we can access it at\n> \"tags/X\". If we accidentally created \"tags/X\" at the\n> top-level $GIT_DIR instead of under \"refs/\", we would still\n> find it due to our ref lookup procedure!\n>\n> We do not make such a mistake in this patch, of course, but\n> while we're thinking about it, let's make the fast-import\n> tests more robust by checking for fully qualified\n> refnames.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> As I mentioned, I'd be OK with dropping this one in favor of just\n> waiting for Ronnie's transaction patches to graduate.\n>\n>  fast-import.c          | 4 ++--\n>  t/t9300-fast-import.sh | 8 ++++----\n>  2 files changed, 6 insertions(+), 6 deletions(-)\n>\n> diff --git a/fast-import.c b/fast-import.c\n> index a1479e9..04a85a4 100644\n> --- a/fast-import.c\n> +++ b/fast-import.c\n> @@ -1738,8 +1738,8 @@ static void dump_tags(void)\n>\n>         for (t = first_tag; t; t = t->next_tag) {\n>                 strbuf_reset(&ref_name);\n> -               strbuf_addf(&ref_name, \"tags/%s\", t->name);\n> -               lock = lock_ref_sha1(ref_name.buf, NULL);\n> +               strbuf_addf(&ref_name, \"refs/tags/%s\", t->name);\n> +               lock = lock_any_ref_for_update(ref_name.buf, NULL, 0, NULL);\n>                 if (!lock || write_ref_sha1(lock, t->sha1, msg) < 0)\n>                         failure |= error(\"Unable to update %s\", ref_name.buf);\n>         }\n> diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\n> index 5fc9ef2..f4c6673 100755\n> --- a/t/t9300-fast-import.sh\n> +++ b/t/t9300-fast-import.sh\n> @@ -153,7 +153,7 @@ tag series-A\n>  An annotated tag without a tagger\n>  EOF\n>  test_expect_success 'A: verify tag/series-A' '\n> -       git cat-file tag tags/series-A >actual &&\n> +       git cat-file tag refs/tags/series-A >actual &&\n>         test_cmp expect actual\n>  '\n>\n> @@ -165,7 +165,7 @@ tag series-A-blob\n>  An annotated tag that annotates a blob.\n>  EOF\n>  test_expect_success 'A: verify tag/series-A-blob' '\n> -       git cat-file tag tags/series-A-blob >actual &&\n> +       git cat-file tag refs/tags/series-A-blob >actual &&\n>         test_cmp expect actual\n>  '\n>\n> @@ -232,8 +232,8 @@ EOF\n>  test_expect_success \\\n>         'A: tag blob by sha1' \\\n>         'git fast-import <input &&\n> -       git cat-file tag tags/series-A-blob-2 >actual &&\n> -       git cat-file tag tags/series-A-blob-3 >>actual &&\n> +       git cat-file tag refs/tags/series-A-blob-2 >actual &&\n> +       git cat-file tag refs/tags/series-A-blob-3 >>actual &&\n>         test_cmp expect actual'\n>\n>  test_tick\n> --\n> 2.1.0.346.ga0367b9\n"},{"id":"248243","messageId":"CAL=YDWkFpjnsr+eqcOdrYQAPaPMfCdg17+yKvN=CwELbsOi-VA@mail.gmail.com","threadId":"37405","inReplyTo":"20140823052334.GA17813@peff.net","subject":"Re: [PATCH 0/5] fix pack-refs pruning of refs/foo","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-08-25T17:38:56Z","receivedAt":"2014-08-25T17:38:56Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"On Fri, Aug 22, 2014 at 10:23 PM, Jeff King <peff@peff.net> wrote:\n> I noticed that \"git pack-refs --all\" will pack a top-level ref like\n> \"refs/foo\", but will not actually prune \"$GIT_DIR/refs/foo\". I do not\n> see the point in having a policy not to pack \"refs/foo\" if \"--all\" is\n> given. But even if we did have such a policy, this seems broken; we\n> should either pack and prune, or leave them untouched. I don't see any\n> indication that the existing behavior was intentional.\n>\n> The problem is that pack-refs's prune_ref calls lock_ref_sha1, which\n> enforces this \"no toplevel\" behavior. I am not sure there is any real\n> point to this, given that most callers use lock_any_ref_for_update,\n> which is exactly equivalent but without the toplevel check.\n>\n> The first two patches deal with this by switching pack-refs to use\n> lock_any_ref_for_update. This will conflict slightly with Ronnie's\n> ref-transaction work, as he gets rid of lock_ref_sha1 entirely, and\n> moves the code directly into prune_ref. This can be trivially resolved\n> in favor of my patch, I think.\n>\n> The third patch is a cleanup I noticed while looking around, and I think\n> should not conflict with anyone (and is a good thing to do).\n>\n> The last two are trickier. I wondered if we could get rid of\n> lock_ref_sha1 entirely. After pack-refs, there are two callers:\n> fast-import.c and walker.c. After converting the first, it occurred to\n> me that Ronnie might be touching the same areas, and I see that yes,\n> indeed, there's quite a bit of conflict (and he reaches the same end\n> goal of dropping it entirely).\n>\n> So in that sense I do not mind dropping the final two patches. Ronnie's\n> endpoint is much better, moving to a ref_transaction. However, there is\n> actually a buffer overflow in the existing code. Ronnie's series fixes\n> it in a similar way (moving to a strbuf), and I'm fine with that\n> endpoint. But given that the ref transaction code is not yet merged (and\n> would certainly never be maint-track), I think it is worth applying the\n> buffer overflow fix separately.\n>\n> I think the final patch can probably be dropped, then. It is a clean-up,\n> but one that we can just get when Ronnie's series is merged.\n>\n>   [1/5]: git-prompt: do not look for refs/stash in $GIT_DIR\n>   [2/5]: pack-refs: prune top-level refs like \"refs/foo\"\n>   [3/5]: fast-import: clean up pack_data pointer in end_packfile\n>   [4/5]: fast-import: fix buffer overflow in dump_tags\n>   [5/5]: fast-import: stop using lock_ref_sha1\n>\n\n+1 on 1-3\n+1 on 4. While I have a similar fix in the transaction series, you\nshould not need to wait for that series to address a security concern.\n5: I think this one is not as urgent as the others so would prefer if\nit is dropped, just so it doesn't cause more merge conflicts than is\nalready present in the transaction series.\n\n\n1-4:\nReviewed-by: Ronnie Sahlberg <sahlberg@google.com>\n\n\n> -Peff\n"},{"id":"248253","messageId":"20140825185534.GA30953@peff.net","threadId":"37405","inReplyTo":"CAL=YDWmC9ncuH=fLR=8PWuB=kJeRP-O3K9MCKGaDfoxF6WWb8A@mail.gmail.com","subject":"Re: [PATCH 3/5] fast-import: clean up pack_data pointer in end_packfile","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-08-25T18:55:34Z","receivedAt":"2014-08-25T18:55:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 25, 2014 at 10:16:52AM -0700, Ronnie Sahlberg wrote:\n\n> Print an error before returning when pack_data is NULL ?\n\nI don't think so. We call end_packfile in some code paths (like the die\nhandler) as \"close if it's open\". So I think it makes sense for it to be\na noop if nothing is open.\n\n-Peff\n"},{"id":"248254","messageId":"20140825185620.GB30953@peff.net","threadId":"37405","inReplyTo":"CAL=YDWkFpjnsr+eqcOdrYQAPaPMfCdg17+yKvN=CwELbsOi-VA@mail.gmail.com","subject":"Re: [PATCH 0/5] fix pack-refs pruning of refs/foo","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-08-25T18:56:20Z","receivedAt":"2014-08-25T18:56:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 25, 2014 at 10:38:56AM -0700, Ronnie Sahlberg wrote:\n\n> >   [1/5]: git-prompt: do not look for refs/stash in $GIT_DIR\n> >   [2/5]: pack-refs: prune top-level refs like \"refs/foo\"\n> >   [3/5]: fast-import: clean up pack_data pointer in end_packfile\n> >   [4/5]: fast-import: fix buffer overflow in dump_tags\n> >   [5/5]: fast-import: stop using lock_ref_sha1\n> >\n> \n> +1 on 1-3\n> +1 on 4. While I have a similar fix in the transaction series, you\n> should not need to wait for that series to address a security concern.\n> 5: I think this one is not as urgent as the others so would prefer if\n> it is dropped, just so it doesn't cause more merge conflicts than is\n> already present in the transaction series.\n\nOK, I think we're in agreement then. Thanks for looking them over.\n\n-Peff\n"}]}