Commit f5302617 authored by Andrew Newdigate's avatar Andrew Newdigate Committed by Sean McGivern

Popen with a timeout

parent fde837d5
......@@ -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
......
---
title: Use a timeout on certain git operations
merge_request: 14872
author:
type: security
......@@ -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
......@@ -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)
......
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
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