From db4be155a7542f9025b410920c9f7290ed302246 Mon Sep 17 00:00:00 2001 From: BogdanIrimie Date: Wed, 24 Jun 2020 11:09:24 +0300 Subject: [PATCH] (FACT-2678) Only change environment variable in open3 run. --- lib/custom_facts/core/execution/base.rb | 36 +++++++++---------- .../core/execution/fact_manager_spec.rb | 24 +++++-------- .../resolvers/lsb_release_resolver_spec.rb | 10 +++--- spec/facter/resolvers/partitions_spec.rb | 18 ++++++---- 4 files changed, 44 insertions(+), 44 deletions(-) diff --git a/lib/custom_facts/core/execution/base.rb b/lib/custom_facts/core/execution/base.rb index e32fec00f3..1bd9de6851 100644 --- a/lib/custom_facts/core/execution/base.rb +++ b/lib/custom_facts/core/execution/base.rb @@ -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| @@ -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 @@ -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 diff --git a/spec/custom_facts/core/execution/fact_manager_spec.rb b/spec/custom_facts/core/execution/fact_manager_spec.rb index 0ab07d66ae..067561bb54 100644 --- a/spec/custom_facts/core/execution/fact_manager_spec.rb +++ b/spec/custom_facts/core/execution/fact_manager_spec.rb @@ -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 @@ -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 @@ -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') @@ -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) @@ -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) @@ -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 diff --git a/spec/facter/resolvers/lsb_release_resolver_spec.rb b/spec/facter/resolvers/lsb_release_resolver_spec.rb index 878da87301..80e13ba4da 100644 --- a/spec/facter/resolvers/lsb_release_resolver_spec.rb +++ b/spec/facter/resolvers/lsb_release_resolver_spec.rb @@ -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 @@ -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 @@ -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 diff --git a/spec/facter/resolvers/partitions_spec.rb b/spec/facter/resolvers/partitions_spec.rb index 2b0f996115..85959d092b 100644 --- a/spec/facter/resolvers/partitions_spec.rb +++ b/spec/facter/resolvers/partitions_spec.rb @@ -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 @@ -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 @@ -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