(MODULES-1723) Use wide character registry APIs

 - Switch to using RegQueryValueExW and RegDeleteKeyExW instead of their
   ANSI counterparts
 - Since Ruby 2.1.5 is broken with respec to calling delete_key and
   delete_value, add our own code for RegDeleteValueW to be used.
   Ensure that all deletions of registry keys go through the API rather
   than Ruby.
 - Implement a helper method for converting Ruby strings to UTF-16LE
   strings and for creating an unmanaged LCPWSTR pointer for use in FFI
   calls given a Ruby string.  This code appears in Puppet itself, but
   for compatibility reasons, the Registry module cannot rely on the
   Puppet version that defines it.
 - Implement an each_value inside of the provider base code that either
   uses Rubys Win32::Registry#each_value in Ruby 2.0 and lower, or uses
   the Puppet 4 implementation if available.  This provides existing
   behavior for older versions of Puppet / Ruby which don't have LOCALE
   conversion issues, and the correct behavior for Puppet 4.  This was
   implemented this way to avoid dragging in a lot of duplicated FFI
   code from Puppet 4 introduced via:

   puppetlabs/puppet@b46ede7

Author: Iristyle

lib/puppet/provider/registry_key/registry.rb

@@ -38,9 +38,11 @@ def destroy
 
     raise ArgumentError, "Cannot delete root key: #{path}" unless subkey
 
-    # hive.hkey returns an integer value that's like a FD
-    if RegDeleteKeyExA(hive.hkey, subkey, access, 0) != 0
-      raise "Failed to delete registry key: #{self}"
+    from_string_to_wide_string(subkey) do |subkey_ptr|
+      # hive.hkey returns an integer value that's like a FD
+      if RegDeleteKeyExW(hive.hkey, subkey_ptr, access, 0) != 0
+        raise "Failed to delete registry key: #{self}"
+      end
     end
   end
 
@@ -49,7 +51,7 @@ def values
     # Only try and get the values for this key if the key itself exists.
     if exists? then
       hive.open(subkey, Win32::Registry::KEY_READ | access) do |reg|
-        reg.each_value do |name, type, data| names << name end
+        each_value(reg) do |name, type, data| names << name end
       end
     end
     names

lib/puppet/provider/registry_value/registry.rb

@@ -24,12 +24,14 @@ def exists?
     found = false
     begin
       hive.open(subkey, Win32::Registry::KEY_READ | access) do |reg|
-        status = RegQueryValueExA(reg.hkey, valuename,
-          FFI::MemoryPointer::NULL, FFI::MemoryPointer::NULL,
-          FFI::MemoryPointer::NULL, FFI::MemoryPointer::NULL)
+        from_string_to_wide_string(valuename) do |valuename_ptr|
+          status = RegQueryValueExW(reg.hkey, valuename_ptr,
+            FFI::MemoryPointer::NULL, FFI::MemoryPointer::NULL,
+            FFI::MemoryPointer::NULL, FFI::MemoryPointer::NULL)
 
-        found = status == 0
-        raise Win32::Registry::Error.new(status) if !found
+          found = status == 0
+          raise Win32::Registry::Error.new(status) if !found
+        end
       end
     rescue Win32::Registry::Error => detail
       case detail.code
@@ -61,8 +63,15 @@ def flush
 
   def destroy
     Puppet.debug("Destroying registry value: #{self}")
+    # On Ruby 2.1.x, due to https://bugs.ruby-lang.org/issues/10820, we see
+    # a FileNotFound error - hence an FFI re-implementation inside destroy
     hive.open(subkey, Win32::Registry::KEY_ALL_ACCESS | access) do |reg|
-      reg.delete_value(valuename)
+      from_string_to_wide_string(valuename) do |valuename_ptr|
+        if RegDeleteValueW(reg.hkey, valuename_ptr) != 0
+          msg = "Failed to delete registry value #{valuename} at #{reg.keyname}"
+          raise Puppet::Util::Windows::Error.new(msg)
+        end
+      end
     end
   end
 
@@ -86,10 +95,12 @@ def regvalue
     unless @regvalue
       @regvalue = {}
       hive.open(subkey, Win32::Registry::KEY_READ | access) do |reg|
-        if RegQueryValueExA(reg.hkey, valuename,
-          FFI::MemoryPointer::NULL, FFI::MemoryPointer::NULL,
-          FFI::MemoryPointer::NULL, FFI::MemoryPointer::NULL) == 0
-          @regvalue[:type], @regvalue[:data] = from_native(reg.read(valuename))
+        from_string_to_wide_string(valuename) do |valuename_ptr|
+          if RegQueryValueExW(reg.hkey, valuename_ptr,
+            FFI::MemoryPointer::NULL, FFI::MemoryPointer::NULL,
+            FFI::MemoryPointer::NULL, FFI::MemoryPointer::NULL) == 0
+            @regvalue[:type], @regvalue[:data] = from_native(reg.read(valuename))
+          end
         end
       end
     end

lib/puppet_x/puppetlabs/registry/provider_base.rb

@@ -3,7 +3,7 @@ module PuppetX
 module Puppetlabs
 module Registry
 module ProviderBase
-  def self.define_ffi
+  def self.define_ffi(base)
     extend FFI::Library
 
     ffi_convention :stdcall
@@ -28,7 +28,7 @@ def self.define_ffi
     #   _Inout_opt_  LPDWORD lpcbData
     # );
     ffi_lib :advapi32
-    attach_function :RegQueryValueExA,
+    attach_function :RegQueryValueExW,
       [:handle, :pointer, :pointer, :pointer, :pointer, :pointer], :win32_long
 
     # http://msdn.microsoft.com/en-us/library/windows/desktop/ms724847(v=vs.85).aspx
@@ -39,12 +39,49 @@ def self.define_ffi
     #   _Reserved_  DWORD Reserved
     # );
     ffi_lib :advapi32
-    attach_function :RegDeleteKeyExA,
+    attach_function :RegDeleteKeyExW,
       [:handle, :pointer, :win32_ulong, :dword], :win32_long
+
+    # https://msdn.microsoft.com/en-us/library/windows/desktop/ms724851(v=vs.85).aspx
+    # LONG WINAPI RegDeleteValue(
+    #   _In_      HKEY hKey,
+    #   _In_opt_  LPCTSTR lpValueName
+    # );
+    ffi_lib :advapi32
+    attach_function :RegDeleteValueW,
+      [:handle, :pointer], :win32_long
+
+    # this duplicates code found in puppet, but necessary for backwards compat
+    class << base
+      # note that :uchar is aliased in Puppet to :byte
+      def from_string_to_wide_string(str, &block)
+        str = wide_string(str)
+        FFI::MemoryPointer.new(:uchar, str.bytesize) do |ptr|
+          ptr.put_array_of_uchar(0, str.bytes.to_a)
+
+          yield ptr
+        end
+
+        # ptr has already had free called, so nothing to return
+        nil
+      end
+
+      def wide_string(str)
+        # if given a nil string, assume caller wants to pass a nil pointer to win32
+        return nil if str.nil?
+        # ruby (< 2.1) does not respect multibyte terminators, so it is possible
+        # for a string to contain a single trailing null byte, followed by garbage
+        # causing buffer overruns.
+        #
+        # See http://svn.ruby-lang.org/cgi-bin/viewvc.cgi?revision=41920&view=revision
+        newstr = str + "\0".encode(str.encoding)
+        newstr.encode!('UTF-16LE')
+      end
+    end
   end
 
   # This is a class method in order to be easily mocked in the spec tests.
-  def self.initialize_system_api
+  def self.initialize_system_api(base)
     if Puppet.features.microsoft_windows?
       begin
         require 'win32/registry'
@@ -58,22 +95,30 @@ def self.initialize_system_api
 
       begin
         require 'ffi'
-        define_ffi
+        define_ffi(base)
       rescue LoadError => exc
         msg = "Could not load the required ffi library [#{exc.message}]"
         Puppet.err msg
         error = Puppet::Error.new(msg)
         error.set_backtrace exc.backtrace
         raise error
       end
+
+      class << base
+        # create instance to access mix-in methods since it doesn't use module_function
+        require 'puppet/util/windows/registry'
+        def RegistryHelpers
+          @registry_helpers ||= Class.new.extend(Puppet::Util::Windows::Registry)
+        end
+      end
     end
   end
 
   def self.included(base)
     # Initialize the Win32 API.  This is a method call so the spec tests can
     # easily mock the initialization of the Win32 libraries on non-win32
     # systems.
-    initialize_system_api
+    initialize_system_api(base)
 
     # Define an hkeys class method in the eigenclass we're being mixed into.
     # This is the one true place to define the root hives we support.
@@ -91,10 +136,18 @@ def hkeys
   # The rest of these methods will be mixed in as instance methods into the
   # provider class.  The path method is expected to be mixed in by the provider
   # specific module, ProviderKeyBase or ProviderValueBase
+  def from_string_to_wide_string(str, &block)
+    self.class.from_string_to_wide_string(str, &block)
+  end
+
   def hkeys
     self.class.hkeys
   end
 
+  def registry_helpers
+    self.class.RegistryHelpers
+  end
+
   def hive
     hkeys[path.root]
   end
@@ -136,6 +189,22 @@ def name2type(name)
     type2name_map.each_pair {|k,v| name2type[v] = k}
     name2type[name]
   end
+
+  def each_value(key, &block)
+    # This problem affects Ruby 2.1 and higher by introducing locale conversion
+    # unnecessary. Puppet 4 introduces it's own each_value patches to the
+    # Registry abstraction to work around these problems
+    # https://github.com/puppetlabs/puppet/commit/b46ede74f640a809b68a473ac8720b93b13d2ac3
+    if registry_helpers.respond_to?(:each_value)
+      registry_helpers.each_value(key) do |name, type, data|
+        yield name, type, data
+      end
+    else
+      key.each_value do |name, type, data|
+        yield name, type, data
+      end
+    end
+  end
 end
 end
 end

spec/unit/puppet/provider/registry_key_spec.rb

@@ -11,16 +11,16 @@
   subkey_name ="PuppetRegProviderTest"
 
   before(:each) do
-    if RUBY_VERSION >= '2.1'
-      # problematic Ruby codepath triggers a conversion of UTF-16LE to
-      # a local codepage which can totally break when that codepage has no
-      # conversion from the given UTF-16LE characters to local codepage
-      # a prime example is that IBM437 has no conversion from a Unicode en-dash
-      Win32::Registry.any_instance.expects(:export_string).never
+    # problematic Ruby codepath triggers a conversion of UTF-16LE to
+    # a local codepage which can totally break when that codepage has no
+    # conversion from the given UTF-16LE characters to local codepage
+    # a prime example is that IBM437 has no conversion from a Unicode en-dash
+    Win32::Registry.any_instance.expects(:export_string).never
 
-      Win32::Registry.any_instance.expects(:delete_value).never
-      Win32::Registry.any_instance.expects(:delete_key).never
+    Win32::Registry.any_instance.expects(:delete_value).never
+    Win32::Registry.any_instance.expects(:delete_key).never
 
+    if RUBY_VERSION >= '2.1'
       # also, expect that we're not using Rubys each_key / each_value which exhibit bad behavior
       Win32::Registry.any_instance.expects(:each_key).never
       Win32::Registry.any_instance.expects(:each_value).never

spec/unit/puppet/provider/registry_value_spec.rb

@@ -17,16 +17,16 @@
   end
 
   before(:each) do
-    if RUBY_VERSION >= '2.1'
-      # problematic Ruby codepath triggers a conversion of UTF-16LE to
-      # a local codepage which can totally break when that codepage has no
-      # conversion from the given UTF-16LE characters to local codepage
-      # a prime example is that IBM437 has no conversion from a Unicode en-dash
-      Win32::Registry.any_instance.expects(:export_string).never
+    # problematic Ruby codepath triggers a conversion of UTF-16LE to
+    # a local codepage which can totally break when that codepage has no
+    # conversion from the given UTF-16LE characters to local codepage
+    # a prime example is that IBM437 has no conversion from a Unicode en-dash
+    Win32::Registry.any_instance.expects(:export_string).never
 
-      Win32::Registry.any_instance.expects(:delete_value).never
-      Win32::Registry.any_instance.expects(:delete_key).never
+    Win32::Registry.any_instance.expects(:delete_value).never
+    Win32::Registry.any_instance.expects(:delete_key).never
 
+    if RUBY_VERSION >= '2.1'
       # also, expect that we're not using Rubys each_key / each_value which exhibit bad behavior
       Win32::Registry.any_instance.expects(:each_key).never
       Win32::Registry.any_instance.expects(:each_value).never