Commit ae1da08f authored by Douwe Maan's avatar Douwe Maan

Merge branch 'safe-ref-updates' into 'master'

Safer ref updates

Use `git update-ref` to prevent clobbering concurrent ref updates. If
rugged/libgit2 is temporarily confused, due to concurrent `git gc` for
example, then it used to be possible to accidentally reset a ref to an
earlier state and lose commits. Because `git update-ref` does not get
confused by `git gc`, what will happen now is that the commit based on
confused information fails, preventing data loss.

Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/20353

See merge request !6130
parents b46c703b ffef94f1
...@@ -31,6 +31,7 @@ v 8.12.0 (unreleased) ...@@ -31,6 +31,7 @@ v 8.12.0 (unreleased)
- Remove prefixes from transition CSS property (ClemMakesApps) - Remove prefixes from transition CSS property (ClemMakesApps)
- Add Sentry logging to API calls - Add Sentry logging to API calls
- Add BroadcastMessage API - Add BroadcastMessage API
- Use 'git update-ref' for safer web commits !6130
- Automatically expand hidden discussions when accessed by a permalink !5585 (Mike Greiling) - Automatically expand hidden discussions when accessed by a permalink !5585 (Mike Greiling)
- Remove unused mixins (ClemMakesApps) - Remove unused mixins (ClemMakesApps)
- Add search to all issue board lists - Add search to all issue board lists
......
...@@ -149,7 +149,7 @@ class Repository ...@@ -149,7 +149,7 @@ class Repository
return false unless target return false unless target
GitHooksService.new.execute(user, path_to_repo, oldrev, target, ref) do GitHooksService.new.execute(user, path_to_repo, oldrev, target, ref) do
rugged.branches.create(branch_name, target) update_ref!(ref, target, oldrev)
end end
after_create_branch after_create_branch
...@@ -181,7 +181,7 @@ class Repository ...@@ -181,7 +181,7 @@ class Repository
ref = Gitlab::Git::BRANCH_REF_PREFIX + branch_name ref = Gitlab::Git::BRANCH_REF_PREFIX + branch_name
GitHooksService.new.execute(user, path_to_repo, oldrev, newrev, ref) do GitHooksService.new.execute(user, path_to_repo, oldrev, newrev, ref) do
rugged.branches.delete(branch_name) update_ref!(ref, newrev, oldrev)
end end
after_remove_branch after_remove_branch
...@@ -215,6 +215,21 @@ class Repository ...@@ -215,6 +215,21 @@ class Repository
rugged.references.exist?(ref) rugged.references.exist?(ref)
end end
def update_ref!(name, newrev, oldrev)
# We use 'git update-ref' because libgit2/rugged currently does not
# offer 'compare and swap' ref updates. Without compare-and-swap we can
# (and have!) accidentally reset the ref to an earlier state, clobbering
# commits. See also https://github.com/libgit2/libgit2/issues/1534.
command = %w[git update-ref --stdin -z]
_, status = Gitlab::Popen.popen(command, path_to_repo) do |stdin|
stdin.write("update #{name}\x00#{newrev}\x00#{oldrev}\x00")
end
return if status.zero?
raise CommitError.new("Could not update branch #{name.sub('refs/heads/', '')}. Please refresh and try again.")
end
# Makes sure a commit is kept around when Git garbage collection runs. # Makes sure a commit is kept around when Git garbage collection runs.
# Git GC will delete commits from the repository that are no longer in any # Git GC will delete commits from the repository that are no longer in any
# branches or tags, but we want to keep some of these commits around, for # branches or tags, but we want to keep some of these commits around, for
...@@ -1014,15 +1029,10 @@ class Repository ...@@ -1014,15 +1029,10 @@ class Repository
def commit_with_hooks(current_user, branch) def commit_with_hooks(current_user, branch)
update_autocrlf_option update_autocrlf_option
oldrev = Gitlab::Git::BLANK_SHA
ref = Gitlab::Git::BRANCH_REF_PREFIX + branch ref = Gitlab::Git::BRANCH_REF_PREFIX + branch
target_branch = find_branch(branch) target_branch = find_branch(branch)
was_empty = empty? was_empty = empty?
if !was_empty && target_branch
oldrev = target_branch.target.id
end
# Make commit # Make commit
newrev = yield(ref) newrev = yield(ref)
...@@ -1030,24 +1040,15 @@ class Repository ...@@ -1030,24 +1040,15 @@ class Repository
raise CommitError.new('Failed to create commit') raise CommitError.new('Failed to create commit')
end end
oldrev = rugged.lookup(newrev).parent_ids.first || Gitlab::Git::BLANK_SHA
GitHooksService.new.execute(current_user, path_to_repo, oldrev, newrev, ref) do GitHooksService.new.execute(current_user, path_to_repo, oldrev, newrev, ref) do
if was_empty || !target_branch update_ref!(ref, newrev, oldrev)
# Create branch
rugged.references.create(ref, newrev)
if was_empty || !target_branch
# If repo was empty expire cache # If repo was empty expire cache
after_create if was_empty after_create if was_empty
after_create_branch after_create_branch
else
# Update head
current_head = find_branch(branch).target.id
# Make sure target branch was not changed during pre-receive hook
if current_head == oldrev
rugged.references.update(ref, newrev)
else
raise CommitError.new('Commit was rejected because branch received new push')
end
end end
end end
......
...@@ -21,9 +21,9 @@ module Gitlab ...@@ -21,9 +21,9 @@ module Gitlab
@cmd_output = "" @cmd_output = ""
@cmd_status = 0 @cmd_status = 0
Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr| Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr|
# We are not using stdin so we should close it, in case the command we yield(stdin) if block_given?
# are running waits for input.
stdin.close stdin.close
@cmd_output << stdout.read @cmd_output << stdout.read
@cmd_output << stderr.read @cmd_output << stderr.read
@cmd_status = wait_thr.value.exitstatus @cmd_status = wait_thr.value.exitstatus
......
...@@ -40,4 +40,13 @@ describe 'Gitlab::Popen', lib: true, no_db: true do ...@@ -40,4 +40,13 @@ describe 'Gitlab::Popen', lib: true, no_db: true do
it { expect(@status).to be_zero } it { expect(@status).to be_zero }
it { expect(@output).to include('spec') } it { expect(@output).to include('spec') }
end end
context 'use stdin' do
before do
@output, @status = @klass.new.popen(%w[cat]) { |stdin| stdin.write 'hello' }
end
it { expect(@status).to be_zero }
it { expect(@output).to eq('hello') }
end
end end
...@@ -443,31 +443,32 @@ describe Repository, models: true do ...@@ -443,31 +443,32 @@ describe Repository, models: true do
describe '#commit_with_hooks' do describe '#commit_with_hooks' do
let(:old_rev) { '0b4bc9a49b562e85de7cc9e834518ea6828729b9' } # git rev-parse feature let(:old_rev) { '0b4bc9a49b562e85de7cc9e834518ea6828729b9' } # git rev-parse feature
let(:new_rev) { 'a74ae73c1ccde9b974a70e82b901588071dc142a' } # commit whose parent is old_rev
context 'when pre hooks were successful' do context 'when pre hooks were successful' do
before do before do
expect_any_instance_of(GitHooksService).to receive(:execute). expect_any_instance_of(GitHooksService).to receive(:execute).
with(user, repository.path_to_repo, old_rev, sample_commit.id, 'refs/heads/feature'). with(user, repository.path_to_repo, old_rev, new_rev, 'refs/heads/feature').
and_yield.and_return(true) and_yield.and_return(true)
end end
it 'runs without errors' do it 'runs without errors' do
expect do expect do
repository.commit_with_hooks(user, 'feature') { sample_commit.id } repository.commit_with_hooks(user, 'feature') { new_rev }
end.not_to raise_error end.not_to raise_error
end end
it 'ensures the autocrlf Git option is set to :input' do it 'ensures the autocrlf Git option is set to :input' do
expect(repository).to receive(:update_autocrlf_option) expect(repository).to receive(:update_autocrlf_option)
repository.commit_with_hooks(user, 'feature') { sample_commit.id } repository.commit_with_hooks(user, 'feature') { new_rev }
end end
context "when the branch wasn't empty" do context "when the branch wasn't empty" do
it 'updates the head' do it 'updates the head' do
expect(repository.find_branch('feature').target.id).to eq(old_rev) expect(repository.find_branch('feature').target.id).to eq(old_rev)
repository.commit_with_hooks(user, 'feature') { sample_commit.id } repository.commit_with_hooks(user, 'feature') { new_rev }
expect(repository.find_branch('feature').target.id).to eq(sample_commit.id) expect(repository.find_branch('feature').target.id).to eq(new_rev)
end end
end end
end end
...@@ -477,7 +478,7 @@ describe Repository, models: true do ...@@ -477,7 +478,7 @@ describe Repository, models: true do
allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return([false, '']) allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return([false, ''])
expect do expect do
repository.commit_with_hooks(user, 'feature') { sample_commit.id } repository.commit_with_hooks(user, 'feature') { new_rev }
end.to raise_error(GitHooksService::PreReceiveError) end.to raise_error(GitHooksService::PreReceiveError)
end end
end end
...@@ -485,6 +486,7 @@ describe Repository, models: true do ...@@ -485,6 +486,7 @@ describe Repository, models: true do
context 'when target branch is different from source branch' do context 'when target branch is different from source branch' do
before do before do
allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return([true, '']) allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return([true, ''])
allow(repository).to receive(:update_ref!)
end end
it 'expires branch cache' do it 'expires branch cache' do
...@@ -495,7 +497,7 @@ describe Repository, models: true do ...@@ -495,7 +497,7 @@ describe Repository, models: true do
expect(repository).to receive(:expire_has_visible_content_cache) expect(repository).to receive(:expire_has_visible_content_cache)
expect(repository).to receive(:expire_branch_count_cache) expect(repository).to receive(:expire_branch_count_cache)
repository.commit_with_hooks(user, 'new-feature') { sample_commit.id } repository.commit_with_hooks(user, 'new-feature') { new_rev }
end end
end end
...@@ -1268,4 +1270,18 @@ describe Repository, models: true do ...@@ -1268,4 +1270,18 @@ describe Repository, models: true do
File.delete(path) File.delete(path)
end end
end end
describe '#update_ref!' do
it 'can create a ref' do
repository.update_ref!('refs/heads/foobar', 'refs/heads/master', Gitlab::Git::BLANK_SHA)
expect(repository.find_branch('foobar')).not_to be_nil
end
it 'raises CommitError when the ref update fails' do
expect do
repository.update_ref!('refs/heads/master', 'refs/heads/master', Gitlab::Git::BLANK_SHA)
end.to raise_error(Repository::CommitError)
end
end
end end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment