Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(FACT-2678) Facter sometimes pollutes the calling processes environment (race condition) #1932

Merged
merged 1 commit into from
Jun 24, 2020
Merged

(FACT-2678) Facter sometimes pollutes the calling processes environment (race condition) #1932

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 18 additions & 18 deletions lib/custom_facts/core/execution/base.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ module Execution
class Base
STDERR_MESSAGE = 'Command %s resulted with the following stderr message: %s'

# This is part of the public API. No race condition can happen
# here because custom facts are executed sequentially
def with_env(values)
old = {}
values.each do |var, value|
Expand Down Expand Up @@ -38,27 +40,22 @@ def execute(command, options = {})
expand = options.fetch(:expand, true)
logger = options[:logger]

# Set LC_ALL and LANG to force i18n to C for the duration of this exec;
# this ensures that any code that parses the
# output of the command can expect it to be in a consistent / predictable format / locale
with_env 'LC_ALL' => 'C', 'LANG' => 'C' do
expanded_command = if !expand && builtin_command?(command) || logger
command
else
expand_command(command)
end
expanded_command = if !expand && builtin_command?(command) || logger
command
else
expand_command(command)
end

if expanded_command.nil?
if on_fail == :raise
raise Facter::Core::Execution::ExecutionFailure.new,
"Could not execute '#{command}': command not found"
end

return on_fail
if expanded_command.nil?
if on_fail == :raise
raise Facter::Core::Execution::ExecutionFailure.new,
"Could not execute '#{command}': command not found"
end

execute_command(expanded_command, on_fail, logger)
return on_fail
end

execute_command(expanded_command, on_fail, logger)
end

private
Expand All @@ -82,7 +79,10 @@ def builtin_command?(command)

def execute_command(command, on_fail, logger = nil)
begin
out, stderr, _status_ = Open3.capture3(command.to_s)
# Set LC_ALL and LANG to force i18n to C for the duration of this exec;
# this ensures that any code that parses the
# output of the command can expect it to be in a consistent / predictable format / locale
out, stderr, _status_ = Open3.capture3({ 'LC_ALL' => 'C', 'LANG' => 'C' }, command.to_s)
log_stderr(stderr, command, logger)
rescue StandardError => e
return '' if logger
Expand Down
24 changes: 9 additions & 15 deletions spec/custom_facts/core/execution/fact_manager_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,19 +79,12 @@ def handy_method
end

describe '#execute' do
it 'switches LANG and LC_ALL to C when executing the command' do
allow(ENV).to receive(:[]=)
allow(Open3).to receive(:capture3).with('foo').and_return('')
expect(ENV).to receive(:[]=).with('LC_ALL', 'C')
executor.execute('foo', logger: Facter::Log.new('class'))
end

it 'expands the command before running it' do
allow(File).to receive(:executable?).and_return(false)
allow(FileTest).to receive(:file?).and_return(false)
allow(File).to receive(:executable?).with('/sbin/foo').and_return(true)
allow(FileTest).to receive(:file?).with('/sbin/foo').and_return(true)
expect(Open3).to receive(:capture3).with('/sbin/foo').and_return('')
expect(Open3).to receive(:capture3).with({ 'LC_ALL' => 'C', 'LANG' => 'C' }, '/sbin/foo').and_return('')
executor.execute('foo')
end

Expand All @@ -107,7 +100,7 @@ def handy_method
end

it 'does not expant builtin command' do
allow(Open3).to receive(:capture3).with('/bin/foo').and_return('')
allow(Open3).to receive(:capture3).with({ 'LC_ALL' => 'C', 'LANG' => 'C' }, '/bin/foo').and_return('')
allow(Open3).to receive(:capture2).with('type /bin/foo').and_return('builtin')
executor.execute('/bin/foo', expand: false)
end
Expand All @@ -125,8 +118,8 @@ def handy_method
end

it 'throws exception' do
allow(Open3).to receive(:capture3).with('foo').and_return('')
allow(Open3).to receive(:capture2).with('type foo').and_return('builtin')
allow(Open3).to receive(:capture3).with({ 'LC_ALL' => 'C', 'LANG' => 'C' }, 'foo').and_return('')
allow(Open3).to receive(:capture2).with({ 'LC_ALL' => 'C', 'LANG' => 'C' }, 'type foo').and_return('builtin')
expect { execution_base.execute('foo', expand: false) }
.to raise_error(ArgumentError,
'Unsupported argument on Windows expand with value false')
Expand All @@ -138,7 +131,8 @@ def handy_method
let(:command) { '/bin/foo' }

before do
allow(Open3).to receive(:capture3).with(command).and_return(['', 'some error'])
allow(Open3).to receive(:capture3).with({ 'LC_ALL' => 'C', 'LANG' => 'C' }, command)
.and_return(['', 'some error'])
allow(Facter::Log).to receive(:new).with('foo').and_return(logger)

allow(File).to receive(:executable?).with(command).and_return(true)
Expand Down Expand Up @@ -169,7 +163,7 @@ def handy_method

describe 'when command execution fails' do
before do
allow(Open3).to receive(:capture3).with('/bin/foo').and_raise('kaboom!')
allow(Open3).to receive(:capture3).with({ 'LC_ALL' => 'C', 'LANG' => 'C' }, '/bin/foo').and_raise('kaboom!')
allow(File).to receive(:executable?).and_return(false)
allow(FileTest).to receive(:file?).and_return(false)
allow(File).to receive(:executable?).with('/bin/foo').and_return(true)
Expand All @@ -194,13 +188,13 @@ def handy_method
end

it 'returns the output of the command' do
allow(Open3).to receive(:capture3).with('/sbin/foo').and_return('hi')
allow(Open3).to receive(:capture3).with({ 'LC_ALL' => 'C', 'LANG' => 'C' }, '/sbin/foo').and_return('hi')

expect(executor.execute('foo')).to eq 'hi'
end

it 'strips off trailing newlines' do
allow(Open3).to receive(:capture3).with('/sbin/foo').and_return "hi\n"
allow(Open3).to receive(:capture3).with({ 'LC_ALL' => 'C', 'LANG' => 'C' }, '/sbin/foo').and_return "hi\n"

expect(executor.execute('foo')).to eq 'hi'
end
Expand Down
10 changes: 5 additions & 5 deletions spec/facter/resolvers/lsb_release_resolver_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,10 @@
context 'when system is ubuntu' do
before do
allow(Open3).to receive(:capture3)
.with('which lsb_release')
.with({ 'LC_ALL' => 'C', 'LANG' => 'C' }, 'which lsb_release')
.and_return(['/usr/bin/lsb_release', '', 0])
allow(Open3).to receive(:capture3)
.with('lsb_release -a')
.with({ 'LC_ALL' => 'C', 'LANG' => 'C' }, 'lsb_release -a')
.and_return(["Distributor ID:\tUbuntu\nDescription:\tUbuntu 18.04.1 LTS\nRelease:\t18.04\nCodename:\tbionic\n",
'', 0])
end
Expand Down Expand Up @@ -44,10 +44,10 @@
context 'when system is centos' do
before do
allow(Open3).to receive(:capture3)
.with('which lsb_release')
.with({ 'LC_ALL' => 'C', 'LANG' => 'C' }, 'which lsb_release')
.and_return(['/usr/bin/lsb_release', '', 0])
allow(Open3).to receive(:capture3)
.with('lsb_release -a')
.with({ 'LC_ALL' => 'C', 'LANG' => 'C' }, 'lsb_release -a')
.and_return([load_fixture('centos_lsb_release').read, '', 0])
end

Expand Down Expand Up @@ -85,7 +85,7 @@
context 'when lsb_release is not installed on system' do
before do
allow(Open3).to receive(:capture3)
.with('which lsb_release')
.with({ 'LC_ALL' => 'C', 'LANG' => 'C' }, 'which lsb_release')
.and_return(['', 'no lsb_release in (PATH:usr/sbin)', 1])
end

Expand Down
18 changes: 12 additions & 6 deletions spec/facter/resolvers/partitions_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,10 @@
.with("#{sys_block_path}/sda/sda2/size", '0').and_return('201213')
allow(Facter::Util::FileHelper).to receive(:safe_read)
.with("#{sys_block_path}/sda/sda1/size", '0').and_return('234')
allow(Open3).to receive(:capture3).with('which blkid').and_return('/usr/bin/blkid')
allow(Open3).to receive(:capture3).with('blkid').and_return(load_fixture('blkid_output').read)
allow(Open3).to receive(:capture3).with({ 'LC_ALL' => 'C', 'LANG' => 'C' }, 'which blkid')
.and_return('/usr/bin/blkid')
allow(Open3).to receive(:capture3).with({ 'LC_ALL' => 'C', 'LANG' => 'C' }, 'blkid')
.and_return(load_fixture('blkid_output').read)
end

context 'when device size files are readable' do
Expand Down Expand Up @@ -87,8 +89,10 @@
.with("#{sys_block_path}/sda/dm/name").and_return('VolGroup00-LogVol00')
allow(Facter::Util::FileHelper).to receive(:safe_read)
.with("#{sys_block_path}/sda/size", '0').and_return('201213')
allow(Open3).to receive(:capture3).with('which blkid').and_return('/usr/bin/blkid')
allow(Open3).to receive(:capture3).with('blkid').and_return(load_fixture('blkid_output').read)
allow(Open3).to receive(:capture3).with({ 'LC_ALL' => 'C', 'LANG' => 'C' }, 'which blkid')
.and_return('/usr/bin/blkid')
allow(Open3).to receive(:capture3).with({ 'LC_ALL' => 'C', 'LANG' => 'C' }, 'blkid')
.and_return(load_fixture('blkid_output').read)
end

context 'when device name file is readable' do
Expand Down Expand Up @@ -125,8 +129,10 @@
.with("#{sys_block_path}/sda/loop/backing_file").and_return('some_path')
allow(Facter::Util::FileHelper).to receive(:safe_read)
.with("#{sys_block_path}/sda/size", '0').and_return('201213')
allow(Open3).to receive(:capture3).with('which blkid').and_return('/usr/bin/blkid')
allow(Open3).to receive(:capture3).with('blkid').and_return(load_fixture('blkid_output').read)
allow(Open3).to receive(:capture3).with({ 'LC_ALL' => 'C', 'LANG' => 'C' }, 'which blkid')
.and_return('/usr/bin/blkid')
allow(Open3).to receive(:capture3).with({ 'LC_ALL' => 'C', 'LANG' => 'C' }, 'blkid')
.and_return(load_fixture('blkid_output').read)
end

context 'when backing_file is readable' do
Expand Down