Merge pull request #1932 from puppetlabs/FACT-2678

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

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

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

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
 

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