From bfe056a5d4294089dec2fe357456a0dec273e5df Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Mon, 11 Mar 2019 11:50:02 +1300 Subject: [PATCH 1/3] Avoid deadlocks in popen3 When there is a lot of stderr and/or stdout output from the file opened by popen3, then the code can deadlock and hang forever. See the note in the Ruby docs around the usage of popen3: https://docs.ruby-lang.org/en/2.0.0/Open3.html#method-i-popen3 Also changing a use of open3 to capture3, as the code doesn't need to write to standard in. --- ruby/lib/gitlab/git/hook.rb | 15 ++++++++------- ruby/lib/gitlab/git/repository_mirroring.rb | 9 ++------- 2 files changed, 10 insertions(+), 14 deletions(-) diff --git a/ruby/lib/gitlab/git/hook.rb b/ruby/lib/gitlab/git/hook.rb index 222c93d3bb3..437de575daa 100644 --- a/ruby/lib/gitlab/git/hook.rb +++ b/ruby/lib/gitlab/git/hook.rb @@ -72,9 +72,16 @@ module Gitlab stdin.close + # Reading from pipes needs to be non-blocking to avoid deadlocks + out_reader = Thread.new { stdout.read } + err_reader = Thread.new { stderr.read } + + stdout_messages = out_reader.value + stderr_messages = err_reader.value + unless wait_thr.value == 0 exit_status = false - exit_message = retrieve_error_message(stderr, stdout) + exit_message = stderr_messages.presence || stdout_messages end end @@ -94,12 +101,6 @@ module Gitlab [status.success?, stderr.presence || stdout] end - def retrieve_error_message(stderr, stdout) - err_message = stderr.read - err_message = err_message.blank? ? stdout.read : err_message - err_message - end - def env_base_vars(gl_id, gl_username) { 'GITLAB_SHELL_DIR' => Gitlab.config.gitlab_shell.path, diff --git a/ruby/lib/gitlab/git/repository_mirroring.rb b/ruby/lib/gitlab/git/repository_mirroring.rb index 26534051df6..6b0130a3d03 100644 --- a/ruby/lib/gitlab/git/repository_mirroring.rb +++ b/ruby/lib/gitlab/git/repository_mirroring.rb @@ -90,16 +90,11 @@ module Gitlab end def list_remote_tags(remote, env:) - tag_list, exit_code, error = nil cmd = %W[#{Gitlab.config.git.bin_path} --git-dir=#{path} ls-remote --tags #{remote}] - Open3.popen3(env, *cmd) do |_stdin, stdout, stderr, wait_thr| - tag_list = stdout.read - error = stderr.read - exit_code = wait_thr.value.exitstatus - end + tag_list, error, status = Open3.capture3(env, *cmd) - raise RemoteError, error unless exit_code.zero? + raise RemoteError, error unless status.success? tag_list.split("\n") end -- GitLab From 1b37cdd07e07b87ce2c194f92dbaea917dc418c4 Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Tue, 19 Mar 2019 11:45:15 +1300 Subject: [PATCH 2/3] Add spec for failure of retrieving tag list --- ruby/spec/lib/gitlab/git/repository_spec.rb | 31 +++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/ruby/spec/lib/gitlab/git/repository_spec.rb b/ruby/spec/lib/gitlab/git/repository_spec.rb index 4364b8818aa..6345fd39736 100644 --- a/ruby/spec/lib/gitlab/git/repository_spec.rb +++ b/ruby/spec/lib/gitlab/git/repository_spec.rb @@ -131,6 +131,37 @@ describe Gitlab::Git::Repository do # rubocop:disable Metrics/BlockLength it { expect(repository.tags.size).to eq(SeedRepo::Repo::TAGS.size) } end + describe '#remote_tags' do + context 'when listing of remote tags is successful' do + let(:target) { 'dc872e9fa6963f8f03da6c8f6f264d0845d6b092' } + let(:ref) { 'v1.10.0' } + let(:tag_list) { "#{target}\trefs/tags/#{ref}\n" } + let(:ref_name) { 'refs/heads/feature' } + + it 'returns the list of remote tags' do + expect(Open3).to receive(:capture3).and_return( + [tag_list, '', double('success?' => true)] + ) + + response = repository.remote_tags(ref_name) + + expect(response.length).to eq(1) + expect(response.first.target).to eq(target) + expect(response.first.name).to eq(ref) + end + end + + context 'when listing of remote tags fails' do + let(:ref_name) { 'invalid-ref-name' } + + it 'should raise an error' do + expect { repository.remote_tags(ref_name) }.to( + raise_error(Gitlab::Git::RepositoryMirroring::RemoteError) + ) + end + end + end + describe '#empty?' do it { expect(repository).not_to be_empty } end -- GitLab From 2ecf0ff51ced3d0c96011d7c89e5fad2afcc4fab Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Tue, 19 Mar 2019 12:02:18 +1300 Subject: [PATCH 3/3] Make behaviour of update Hook response consistent Previously the update Hook would return any standard error output, even if the script exited successfully. This is different from how the pre-receive and post-receive scripts behave, and from what is documented. --- ruby/lib/gitlab/git/hook.rb | 15 +++++++++-- ruby/spec/lib/gitlab/git/hook_spec.rb | 36 +++++++++++++++++++++++++-- 2 files changed, 47 insertions(+), 4 deletions(-) diff --git a/ruby/lib/gitlab/git/hook.rb b/ruby/lib/gitlab/git/hook.rb index 437de575daa..e1c4932d33f 100644 --- a/ruby/lib/gitlab/git/hook.rb +++ b/ruby/lib/gitlab/git/hook.rb @@ -81,7 +81,7 @@ module Gitlab unless wait_thr.value == 0 exit_status = false - exit_message = stderr_messages.presence || stdout_messages + exit_message = retrieve_error_messages(stderr_messages, stdout_messages) end end @@ -98,7 +98,18 @@ module Gitlab vars = env_base_vars(gl_id, gl_username) stdout, stderr, status = Open3.capture3(vars, path, *args, options) - [status.success?, stderr.presence || stdout] + + exit_message = if status.success? + nil + else + retrieve_error_messages(stderr, stdout) + end + + [status.success?, exit_message] + end + + def retrieve_error_messages(stderr, stdout) + stderr.presence || stdout end def env_base_vars(gl_id, gl_username) diff --git a/ruby/spec/lib/gitlab/git/hook_spec.rb b/ruby/spec/lib/gitlab/git/hook_spec.rb index 2e1551ba106..1678fe7ee1a 100644 --- a/ruby/spec/lib/gitlab/git/hook_spec.rb +++ b/ruby/spec/lib/gitlab/git/hook_spec.rb @@ -68,7 +68,14 @@ describe Gitlab::Git::Hook do end context 'when the hooks are successful' do - let(:script) { "#!/bin/sh\nexit 0\n" } + let(:script) do + <<-SCRIPT + #!/bin/sh + echo "message"; + 1>&2 echo "error"; + exit 0; + SCRIPT + end it 'returns true' do hook_names.each do |hook| @@ -78,10 +85,26 @@ describe Gitlab::Git::Hook do expect(trigger_result.first).to be(true) end end + + it 'returns no messages' do + hook_names.each do |hook| + trigger_result = described_class.new(hook, repo) + .trigger('user-456', 'admin', '0' * 40, 'a' * 40, 'master') + + expect(trigger_result.last).to be_blank + end + end end context 'when the hooks fail' do - let(:script) { "#!/bin/sh\nexit 1\n" } + let(:script) do + <<-SCRIPT + #!/bin/sh + echo "message"; + 1>&2 echo "error"; + exit 1; + SCRIPT + end it 'returns false' do hook_names.each do |hook| @@ -91,6 +114,15 @@ describe Gitlab::Git::Hook do expect(trigger_result.first).to be(false) end end + + it 'returns messages' do + hook_names.each do |hook| + trigger_result = described_class.new(hook, repo) + .trigger('user-1', 'admin', '0' * 40, 'a' * 40, 'master') + + expect(trigger_result.last).to eq("error\n") + end + end end end end -- GitLab