Uploaded image for project: 'Puppet'
  1. Puppet
  2. PUP-5768

Fix problematic usage of ENV for Windows

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Normal
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: PUP 4.4.0
    • Component/s: None
    • Labels:
    • Release Notes:
      Bug Fix
    • Release Notes Summary:
      Hide
      Due to an underlying bug in the Ruby 2.1.x runtime, setting environment variables on Windows with Unicode characters that cannot be represented in the current local codepage could result in their corruption. Puppet now takes efforts to not use the problematic Ruby codepaths that can trigger this behavior, which were previously triggered any time an Exec resource was evaluated.
      Show
      Due to an underlying bug in the Ruby 2.1.x runtime, setting environment variables on Windows with Unicode characters that cannot be represented in the current local codepage could result in their corruption. Puppet now takes efforts to not use the problematic Ruby codepaths that can trigger this behavior, which were previously triggered any time an Exec resource was evaluated.

      Description

      As part of PUP-5726, Windows API calls for retrieving / setting environment variables were added. This was done to work around a Ruby ENV bug https://bugs.ruby-lang.org/issues/8822 that corrupts UTF8 characters set in the environment (as keys or values) that don't map cleanly to the local codepage. While this was allegedly fixed in Ruby 2.3.0, it has not yet been backported or scheduled for backport.

      Therefore, the withenv helper was updated to not use Rubys ENV, and to instead use the API calls.

      However, there are a number of places where ENV is accessed and modified that may also trigger corruption issues.

      This ticket should cover:

      • Finding other danger spots in the code - for instance access through ENV of PATH and HOME have risks of being problematic, but SYSTEMROOT, PATHEXT, windir and COMSPEC should be safe to ignore due to the low likelihood of these values containing Unicode characters. ENV usage inside tests can also generally be ignored, as problems running specs would likely only manifest on a non-English Windows install with a username that includes Unicode chars, that has a Unicode path as a home dir.
        • One way to synthetically trigger failures would be to insert a Unicode value into PATH like c:\áš  or similarly set HOME
      • Potentially adding a cross-platform wrapper for setting / reading env vars (that delegates to ENV for POSIX and calls Windows APIs otherwise)
      • Replace Windows specific ENV access where there is danger
      • Finding danger spots in other modules by auditing the forge / notifying authors where appropriate (probably best done in another ticket)

      I've grepped through all of the code, and have narrowed the problematic spots (covering the first point), down to:

      C:\source\puppet\lib\puppet\defaults.rb:
        236:           ENV["PATH"] = "" if ENV["PATH"].nil?
        237:           ENV["PATH"] = value unless value == "none"
        238:           paths = ENV["PATH"].split(File::PATH_SEPARATOR)
        239            Puppet::Util::Platform.default_paths.each do |path|
        240:             ENV["PATH"] += File::PATH_SEPARATOR + path unless paths.include?(path)
       
      C:\source\puppet\lib\puppet\util.rb:
        201:       ENV['PATH'].split(File::PATH_SEPARATOR).each do |dir|
        202          begin
        ...
        208:           if e.to_s =~ /HOME/ and (ENV['HOME'].nil? || ENV['HOME'] == "")
        209              # if we get here they have a tilde in their PATH.  We'll issue a single warning about this and then
       
      C:\source\puppet\lib\puppet\file_system\uniquefile.rb:
        159:       for dir in [ENV['TMPDIR'], ENV['TMP'], ENV['TEMP'], @@systmpdir, '/tmp']
        160          if dir and stat = File.stat(dir) and stat.directory? and stat.writable?
       
      C:\source\puppet\lib\puppet\node\environment.rb:
        451    def self.extralibs()
        452:     if ENV["PUPPETLIB"]
        453:       split_path(ENV["PUPPETLIB"])
       
      C:\source\puppet\lib\puppet\test\test_helper.rb:
        124:       $old_env = ENV.to_hash
        125  
        ...
        176        # faster to use the compare-then-set model to avoid excessive work that it
        177        # justifies the complexity.  --daniel 2012-03-15
        178:       unless ENV.to_hash == $old_env
        179:         ENV.clear
        180:         $old_env.each {|k, v| ENV[k] = v }
       
      C:\source\puppet\spec\unit\util\run_mode_spec.rb:
        276    def without_env(name, &block)
        277:     saved = ENV[name]
        278:     ENV.delete name
        279      yield
        280    ensure
        281:     ENV[name] = saved
        282    end
      

        Attachments

          Issue Links

            Activity

              jsd-sla-details-panel

                People

                • Assignee:
                  Unassigned
                  Reporter:
                  ethan Ethan Brown
                  QA Contact:
                  Ryan Gard
                • Votes:
                  0 Vote for this issue
                  Watchers:
                  6 Start watching this issue

                  Dates

                  • Created:
                    Updated:
                    Resolved: