From 360244a51acbc0e734d13353785fff989015f0da Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 10 Apr 2017 18:13:22 -0400 Subject: receive-pack: drop tmp_objdir_env from run_update_hook Since 722ff7f87 (receive-pack: quarantine objects until pre-receive accepts, 2016-10-03), we have to feed the pre-receive hook the tmp_objdir environment, so that git programs run from the hook know where to find the objects. That commit modified run_update_hook() to do the same, but there it is a noop. By the time we get to the update hooks, we have already migrated the objects from quarantine, and so tmp_objdir_env() will always return NULL. We can drop this useless call. Note that the ordering here and the lack of support for the update hook is intentional. The update hook calls are interspersed with actual ref updates, and we must migrate the objects before any refs are updated (since otherwise those refs would appear broken to outside processes). So the only other options are: - remain in quarantine for the _first_ ref, but not the others. This is sufficiently confusing that it can be rejected outright. - run all the individual update hooks first, then migrate, then update all the refs. But this changes the repository state that the update hooks see (i.e., whether or not refs from the same push are updated yet or not). So the functionality is fine and remains unchanged with this patch; we're just cleaning up a useless and confusing line of code. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index 267d320..a740859 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -768,7 +768,6 @@ static int run_update_hook(struct command *cmd) proc.stdout_to_stderr = 1; proc.err = use_sideband ? -1 : 0; proc.argv = argv; - proc.env = tmp_objdir_env(tmp_objdir); code = start_command(&proc); if (code) -- cgit v0.10.2-6-g49f6 From eaeed077a69ad1e26b0c329ac0f6cbd397f5be9e Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 10 Apr 2017 18:13:39 -0400 Subject: receive-pack: document user-visible quarantine effects Commit 722ff7f87 (receive-pack: quarantine objects until pre-receive accepts, 2016-10-03) changed the underlying details of how we take in objects. This is mostly transparent to the user, but there are a few things they might notice. Let's document them. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/Documentation/git-receive-pack.txt b/Documentation/git-receive-pack.txt index 0ccd5fb..7267ecf 100644 --- a/Documentation/git-receive-pack.txt +++ b/Documentation/git-receive-pack.txt @@ -114,6 +114,8 @@ will be performed, and the update, post-receive and post-update hooks will not be invoked either. This can be useful to quickly bail out if the update is not to be supported. +See the notes on the quarantine environment below. + update Hook ----------- Before each ref is updated, if $GIT_DIR/hooks/update file exists @@ -214,6 +216,32 @@ if the repository is packed and is served via a dumb transport. exec git update-server-info +Quarantine Environment +---------------------- + +When `receive-pack` takes in objects, they are placed into a temporary +"quarantine" directory within the `$GIT_DIR/objects` directory and +migrated into the main object store only after the `pre-receive` hook +has completed. If the push fails before then, the temporary directory is +removed entirely. + +This has a few user-visible effects and caveats: + + 1. Pushes which fail due to problems with the incoming pack, missing + objects, or due to the `pre-receive` hook will not leave any + on-disk data. This is usually helpful to prevent repeated failed + pushes from filling up your disk, but can make debugging more + challenging. + + 2. Any objects created by the `pre-receive` hook will be created in + the quarantine directory (and migrated only if it succeeds). + + 3. The `pre-receive` hook MUST NOT update any refs to point to + quarantined objects. Other programs accessing the repository will + not be able to see the objects (and if the pre-receive hook fails, + those refs would become corrupted). + + SEE ALSO -------- linkgit:git-send-pack[1], linkgit:gitnamespaces[7] diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt index 9565dc3..32343ae 100644 --- a/Documentation/githooks.txt +++ b/Documentation/githooks.txt @@ -256,6 +256,9 @@ environment variables will not be set. If the client selects to use push options, but doesn't transmit any, the count variable will be set to zero, `GIT_PUSH_OPTION_COUNT=0`. +See the section on "Quarantine Environment" in +linkgit:git-receive-pack[1] for some caveats. + [[update]] update ~~~~~~ -- cgit v0.10.2-6-g49f6 From d8f4481c4f03132174b514f428cd67d2cc0dc997 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 10 Apr 2017 18:14:12 -0400 Subject: refs: reject ref updates while GIT_QUARANTINE_PATH is set As documented in git-receive-pack(1), updating a ref from within the pre-receive hook is dangerous and can corrupt your repo. This patch forbids ref updates entirely during the hook to make it harder for adventurous hook writers to shoot themselves in the foot. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/Documentation/git-receive-pack.txt b/Documentation/git-receive-pack.txt index 7267ecf..86a4b32 100644 --- a/Documentation/git-receive-pack.txt +++ b/Documentation/git-receive-pack.txt @@ -239,7 +239,8 @@ This has a few user-visible effects and caveats: 3. The `pre-receive` hook MUST NOT update any refs to point to quarantined objects. Other programs accessing the repository will not be able to see the objects (and if the pre-receive hook fails, - those refs would become corrupted). + those refs would become corrupted). For safety, any ref updates + from within `pre-receive` are automatically rejected. SEE ALSO diff --git a/refs.c b/refs.c index 5ffdd77..916b0d5 100644 --- a/refs.c +++ b/refs.c @@ -1465,6 +1465,12 @@ int ref_transaction_commit(struct ref_transaction *transaction, { struct ref_store *refs = get_ref_store(NULL); + if (getenv(GIT_QUARANTINE_ENVIRONMENT)) { + strbuf_addstr(err, + _("ref updates forbidden inside quarantine environment")); + return -1; + } + return refs->be->transaction_commit(refs, transaction, err); } diff --git a/t/t5547-push-quarantine.sh b/t/t5547-push-quarantine.sh index 1e5d32d..462bfc9 100755 --- a/t/t5547-push-quarantine.sh +++ b/t/t5547-push-quarantine.sh @@ -33,4 +33,15 @@ test_expect_success 'rejected objects are removed' ' test_cmp expect actual ' +test_expect_success 'updating a ref from quarantine is forbidden' ' + git init --bare update.git && + write_script update.git/hooks/pre-receive <<-\EOF && + read old new refname + git update-ref refs/heads/unrelated $new + exit 1 + EOF + test_must_fail git push update.git HEAD && + git -C update.git fsck +' + test_done -- cgit v0.10.2-6-g49f6