Merge branch 'an/popen-deadline' into 'master'

Popen with a timeout

See merge request gitlab-org/gitlab-ce!14872

app/models/repository.rb

@@ -1127,7 +1127,7 @@ class Repository
 
   def last_commit_id_for_path_by_shelling_out(sha, path)
     args = %W(rev-list --max-count=1 #{sha} -- #{path})
-    run_git(args).first.strip
+    raw_repository.run_git_with_timeout(args, Gitlab::Git::Popen::FAST_GIT_PROCESS_TIMEOUT).first.strip
   end
 
   def repository_storage_path

changelogs/unreleased/an-popen-deadline.yml

+---
+title: Use a timeout on certain git operations
+merge_request: 14872
+author:
+type: security

lib/gitlab/git/popen.rb

@@ -5,6 +5,8 @@ require 'open3'
 module Gitlab
   module Git
     module Popen
+      FAST_GIT_PROCESS_TIMEOUT = 15.seconds
+
       def popen(cmd, path, vars = {})
         unless cmd.is_a?(Array)
           raise "System commands must be given as an array of strings"
@@ -27,6 +29,67 @@ module Gitlab
 
         [@cmd_output, @cmd_status]
       end
+
+      def popen_with_timeout(cmd, timeout, path, vars = {})
+        unless cmd.is_a?(Array)
+          raise "System commands must be given as an array of strings"
+        end
+
+        path ||= Dir.pwd
+        vars['PWD'] = path
+
+        unless File.directory?(path)
+          FileUtils.mkdir_p(path)
+        end
+
+        rout, wout = IO.pipe
+        rerr, werr = IO.pipe
+
+        pid = Process.spawn(vars, *cmd, out: wout, err: werr, chdir: path, pgroup: true)
+
+        begin
+          status = process_wait_with_timeout(pid, timeout)
+
+          # close write ends so we could read them
+          wout.close
+          werr.close
+
+          cmd_output = rout.readlines.join
+          cmd_output << rerr.readlines.join # Copying the behaviour of `popen` which merges stderr into output
+
+          [cmd_output, status.exitstatus]
+        rescue Timeout::Error => e
+          kill_process_group_for_pid(pid)
+
+          raise e
+        ensure
+          wout.close unless wout.closed?
+          werr.close unless werr.closed?
+
+          rout.close
+          rerr.close
+        end
+      end
+
+      def process_wait_with_timeout(pid, timeout)
+        deadline = timeout.seconds.from_now
+        wait_time = 0.01
+
+        while deadline > Time.now
+          sleep(wait_time)
+          _, status = Process.wait2(pid, Process::WNOHANG)
+
+          return status unless status.nil?
+        end
+
+        raise Timeout::Error, "Timeout waiting for process ##{pid}"
+      end
+
+      def kill_process_group_for_pid(pid)
+        Process.kill("KILL", -pid)
+        Process.wait(pid)
+      rescue Errno::ESRCH
+      end
     end
   end
 end

lib/gitlab/git/repository.rb

@@ -1071,6 +1071,13 @@ module Gitlab
         end
       end
 
+      # Refactoring aid; allows us to copy code from app/models/repository.rb
+      def run_git_with_timeout(args, timeout, env: {})
+        circuit_breaker.perform do
+          popen_with_timeout([Gitlab.config.git.bin_path, *args], timeout, path, env)
+        end
+      end
+
       # Refactoring aid; allows us to copy code from app/models/repository.rb
       def commit(ref = 'HEAD')
         Gitlab::Git::Commit.find(self, ref)

spec/lib/gitlab/git/popen_spec.rb

+require 'spec_helper'
+
+describe 'Gitlab::Git::Popen' do
+  let(:path) { Rails.root.join('tmp').to_s }
+
+  let(:klass) do
+    Class.new(Object) do
+      include Gitlab::Git::Popen
+    end
+  end
+
+  context 'popen' do
+    context 'zero status' do
+      let(:result) { klass.new.popen(%w(ls), path) }
+      let(:output) { result.first }
+      let(:status) { result.last }
+
+      it { expect(status).to be_zero }
+      it { expect(output).to include('tests') }
+    end
+
+    context 'non-zero status' do
+      let(:result) { klass.new.popen(%w(cat NOTHING), path) }
+      let(:output) { result.first }
+      let(:status) { result.last }
+
+      it { expect(status).to eq(1) }
+      it { expect(output).to include('No such file or directory') }
+    end
+
+    context 'unsafe string command' do
+      it 'raises an error when it gets called with a string argument' do
+        expect { klass.new.popen('ls', path) }.to raise_error(RuntimeError)
+      end
+    end
+
+    context 'with custom options' do
+      let(:vars) { { 'foobar' => 123, 'PWD' => path } }
+      let(:options) { { chdir: path } }
+
+      it 'calls popen3 with the provided environment variables' do
+        expect(Open3).to receive(:popen3).with(vars, 'ls', options)
+
+        klass.new.popen(%w(ls), path, { 'foobar' => 123 })
+      end
+    end
+
+    context 'use stdin' do
+      let(:result) { klass.new.popen(%w[cat], path) { |stdin| stdin.write 'hello' } }
+      let(:output) { result.first }
+      let(:status) { result.last }
+
+      it { expect(status).to be_zero }
+      it { expect(output).to eq('hello') }
+    end
+  end
+
+  context 'popen_with_timeout' do
+    let(:timeout) { 1.second }
+
+    context 'no timeout' do
+      context 'zero status' do
+        let(:result) { klass.new.popen_with_timeout(%w(ls), timeout, path) }
+        let(:output) { result.first }
+        let(:status) { result.last }
+
+        it { expect(status).to be_zero }
+        it { expect(output).to include('tests') }
+      end
+
+      context 'non-zero status' do
+        let(:result) { klass.new.popen_with_timeout(%w(cat NOTHING), timeout, path) }
+        let(:output) { result.first }
+        let(:status) { result.last }
+
+        it { expect(status).to eq(1) }
+        it { expect(output).to include('No such file or directory') }
+      end
+
+      context 'unsafe string command' do
+        it 'raises an error when it gets called with a string argument' do
+          expect { klass.new.popen_with_timeout('ls', timeout, path) }.to raise_error(RuntimeError)
+        end
+      end
+    end
+
+    context 'timeout' do
+      context 'timeout' do
+        it "raises a Timeout::Error" do
+          expect { klass.new.popen_with_timeout(%w(sleep 1000), timeout, path) }.to raise_error(Timeout::Error)
+        end
+
+        it "handles processes that do not shutdown correctly" do
+          expect { klass.new.popen_with_timeout(['bash', '-c', "trap -- '' SIGTERM; sleep 1000"], timeout, path) }.to raise_error(Timeout::Error)
+        end
+      end
+
+      context 'timeout period' do
+        let(:time_taken) do
+          begin
+            start = Time.now
+            klass.new.popen_with_timeout(%w(sleep 1000), timeout, path)
+          rescue
+            Time.now - start
+          end
+        end
+
+        it { expect(time_taken).to be >= timeout }
+      end
+
+      context 'clean up' do
+        let(:instance) { klass.new }
+
+        it 'kills the child process' do
+          expect(instance).to receive(:kill_process_group_for_pid).and_wrap_original do |m, *args|
+            # is the PID, and it should not be running at this point
+            m.call(*args)
+
+            pid = args.first
+            begin
+              Process.getpgid(pid)
+              raise "The child process should have been killed"
+            rescue Errno::ESRCH
+            end
+          end
+
+          expect { instance.popen_with_timeout(['bash', '-c', "trap -- '' SIGTERM; sleep 1000"], timeout, path) }.to raise_error(Timeout::Error)
+        end
+      end
+    end
+  end
+end