Merge branch 'ticket/master/PUP-2738-FFI-Memory-Issues'

* ticket/master/PUP-2738-FFI-Memory-Issues:
  (PUP-2738) Windows::File use FFI::Pointer helper
  (PUP-2738) Move CloseHandle -> FFI::WIN32
  (PUP-2738) Windows utils appropriate return values
  (PUP-2738) Use block form of FFI::MemoryPointer
  (PUP-2738) Puppet::Util::Windows::Process FFI clean
  (PUP-2738) Puppet::Util::Windows::File FFI cleanup
  (PUP-2738) open_symlink should wide_string once
  (PUP-2738) Puppet::Util::Windows::User refactor
  (PUP-2738) FFI from_string_to_wide_string block
  (PUP-2738) FFI::Pointer#read_win32_local_pointer

lib/puppet/util/colors.rb

@@ -133,15 +133,29 @@ def WriteChar(str, col, row)
           writeCoord[:X] = row
           writeCoord[:Y] = col
 
-          numberOfCharsWritten_ptr = FFI::MemoryPointer.new(:dword, 1)
-          WriteConsoleOutputCharacterW(@handle, FFI::MemoryPointer.from_string_to_wide_string(str),
-            str.length, writeCoord, numberOfCharsWritten_ptr)
-          numberOfCharsWritten_ptr.read_dword
+          chars_written = 0
+          FFI::MemoryPointer.from_string_to_wide_string(str) do |msg_ptr|
+            FFI::MemoryPointer.new(:dword, 1) do |numberOfCharsWritten_ptr|
+              WriteConsoleOutputCharacterW(@handle, msg_ptr,
+                str.length, writeCoord, numberOfCharsWritten_ptr)
+              chars_written = numberOfCharsWritten_ptr.read_dword
+            end
+          end
+
+          chars_written
         end
 
         def Write(str)
-          WriteConsoleW(@handle, FFI::MemoryPointer.from_string_to_wide_string(str),
-            str.length, FFI::MemoryPointer.new(:dword, 1), FFI::MemoryPointer::NULL)
+          result = false
+          FFI::MemoryPointer.from_string_to_wide_string(str) do |msg_ptr|
+            FFI::MemoryPointer.new(:dword, 1) do |numberOfCharsWritten_ptr|
+              result = WriteConsoleW(@handle, msg_ptr,
+                str.length, FFI::MemoryPointer.new(:dword, 1),
+                FFI::MemoryPointer::NULL) != FFI::WIN32_FALSE
+            end
+          end
+
+          result
         end
       end
 

lib/puppet/util/execution.rb

@@ -180,8 +180,8 @@ def self.execute(command, options = NoOptionsSpecified)
       begin
         exit_status = Puppet::Util::Windows::Process.wait_process(process_info.process_handle)
       ensure
-        Puppet::Util::Windows::Process.CloseHandle(process_info.process_handle)
-        Puppet::Util::Windows::Process.CloseHandle(process_info.thread_handle)
+        FFI::WIN32.CloseHandle(process_info.process_handle)
+        FFI::WIN32.CloseHandle(process_info.thread_handle)
       end
     end
 

lib/puppet/util/windows/adsi.rb

@@ -34,14 +34,16 @@ def delete(name, resource_type)
     def computer_name
       unless @computer_name
         max_length = MAX_COMPUTERNAME_LENGTH + 1 # NULL terminated
-        buffer = FFI::MemoryPointer.new(max_length * 2) # wide string
-        buffer_size = FFI::MemoryPointer.new(:dword, 1)
-        buffer_size.write_dword(max_length) # length in TCHARs
-
-        if GetComputerNameW(buffer, buffer_size) == FFI::WIN32_FALSE
-          raise Puppet::Util::Windows::Error.new("Failed to get computer name")
+        FFI::MemoryPointer.new(max_length * 2) do |buffer| # wide string
+          FFI::MemoryPointer.new(:dword, 1) do |buffer_size|
+            buffer_size.write_dword(max_length) # length in TCHARs
+
+            if GetComputerNameW(buffer, buffer_size) == FFI::WIN32_FALSE
+              raise Puppet::Util::Windows::Error.new("Failed to get computer name")
+            end
+            @computer_name = buffer.read_wide_string(buffer_size.read_dword)
+          end
         end
-        @computer_name = buffer.read_wide_string(buffer_size.read_dword)
       end
       @computer_name
     end

lib/puppet/util/windows/api_types.rb

@@ -18,13 +18,17 @@ class ::FFI::Pointer
     NULL_HANDLE = 0
     NULL_TERMINATOR_WCHAR = 0
 
-    def self.from_string_to_wide_string(str)
+    def self.from_string_to_wide_string(str, &block)
       str = Puppet::Util::Windows::String.wide_string(str)
-      ptr = FFI::MemoryPointer.new(:byte, str.bytesize)
-      # uchar here is synonymous with byte
-      ptr.put_array_of_uchar(0, str.bytes.to_a)
+      FFI::MemoryPointer.new(:byte, str.bytesize) do |ptr|
+        # uchar here is synonymous with byte
+        ptr.put_array_of_uchar(0, str.bytes.to_a)
 
-      ptr
+        yield ptr
+      end
+
+      # ptr has already had free called, so nothing to return
+      nil
     end
 
     def read_win32_bool
@@ -62,6 +66,23 @@ def read_arbitrary_wide_string_up_to(max_char_length = 512)
       read_wide_string(max_char_length)
     end
 
+    def read_win32_local_pointer(&block)
+      ptr = nil
+      begin
+        ptr = read_pointer
+        yield ptr
+      ensure
+        if ptr && ! ptr.null?
+          if FFI::WIN32::LocalFree(ptr.address) != FFI::Pointer::NULL_HANDLE
+            Puppet.debug "LocalFree memory leak"
+          end
+        end
+      end
+
+      # ptr has already had LocalFree called, so nothing to return
+      nil
+    end
+
     alias_method :write_dword, :write_uint32
   end
 
@@ -118,4 +139,24 @@ def read_arbitrary_wide_string_up_to(max_char_length = 512)
   # 8 bits per byte
   FFI.typedef :uchar, :byte
   FFI.typedef :uint16, :wchar
+
+  module ::FFI::WIN32
+    extend ::FFI::Library
+
+    ffi_convention :stdcall
+
+    # http://msdn.microsoft.com/en-us/library/windows/desktop/aa366730(v=vs.85).aspx
+    # HLOCAL WINAPI LocalFree(
+    #   _In_  HLOCAL hMem
+    # );
+    ffi_lib :kernel32
+    attach_function :LocalFree, [:handle], :handle
+
+    # http://msdn.microsoft.com/en-us/library/windows/desktop/ms724211(v=vs.85).aspx
+    # BOOL WINAPI CloseHandle(
+    #   _In_  HANDLE hObject
+    # );
+    ffi_lib :kernel32
+    attach_function_private :CloseHandle, [:handle], :win32_bool
+  end
 end

lib/puppet/util/windows/error.rb

@@ -27,10 +27,10 @@ def self.format_error_code(code)
             FORMAT_MESSAGE_ARGUMENT_ARRAY |
             FORMAT_MESSAGE_IGNORE_INSERTS |
             FORMAT_MESSAGE_MAX_WIDTH_MASK
-    # this pointer actually points to a :lpwstr (pointer) since we're letting Windows allocate for us
-    buffer_ptr = FFI::MemoryPointer.new(:pointer, 1)
+    error_string = ''
 
-    begin
+    # this pointer actually points to a :lpwstr (pointer) since we're letting Windows allocate for us
+    FFI::MemoryPointer.new(:pointer, 1) do |buffer_ptr|
       length = FormatMessageW(flags, FFI::Pointer::NULL, code, dwLanguageId,
         buffer_ptr, 0, FFI::Pointer::NULL)
 
@@ -40,20 +40,16 @@ def self.format_error_code(code)
       end
 
       # returns an FFI::Pointer with autorelease set to false, which is what we want
-      wide_string_ptr = buffer_ptr.read_pointer
-
-      if wide_string_ptr.null?
-        raise Puppet::Error.new("FormatMessageW failed to allocate buffer for code #{code}")
-      end
-
-      return wide_string_ptr.read_wide_string(length)
-    ensure
-      if ! wide_string_ptr.nil? && ! wide_string_ptr.null?
-        if LocalFree(wide_string_ptr.address) != FFI::Pointer::NULL_HANDLE
-          Puppet.debug "LocalFree memory leak"
+      buffer_ptr.read_win32_local_pointer do |wide_string_ptr|
+        if wide_string_ptr.null?
+          raise Puppet::Error.new("FormatMessageW failed to allocate buffer for code #{code}")
         end
+
+        error_string = wide_string_ptr.read_wide_string(length)
       end
     end
+
+    error_string
   end
 
   FORMAT_MESSAGE_ALLOCATE_BUFFER   = 0x00000100
@@ -88,11 +84,4 @@ def self.format_error_code(code)
   ffi_lib :kernel32
   attach_function_private :FormatMessageW,
     [:dword, :lpcvoid, :dword, :dword, :pointer, :dword, :pointer], :dword
-
-  # http://msdn.microsoft.com/en-us/library/windows/desktop/aa366730(v=vs.85).aspx
-  # HLOCAL WINAPI LocalFree(
-  #   _In_  HLOCAL hMem
-  # );
-  ffi_lib :kernel32
-  attach_function_private :LocalFree, [:handle], :handle
 end

lib/puppet/util/windows/file.rb

@@ -68,25 +68,41 @@ def self.create_file(file_name, desired_access, share_mode, security_attributes,
         "#{flags_and_attributes.to_s(8)}, #{template_file_handle})")
   end
 
+  def self.get_reparse_point_data(handle, &block)
+    # must be multiple of 1024, min 10240
+    FFI::MemoryPointer.new(REPARSE_DATA_BUFFER.size) do |reparse_data_buffer_ptr|
+      device_io_control(handle, FSCTL_GET_REPARSE_POINT, nil, reparse_data_buffer_ptr)
+      yield REPARSE_DATA_BUFFER.new(reparse_data_buffer_ptr)
+    end
+
+    # underlying struct MemoryPointer has been cleaned up by this point, nothing to return
+    nil
+  end
+
   def self.device_io_control(handle, io_control_code, in_buffer = nil, out_buffer = nil)
     if out_buffer.nil?
       raise Puppet::Util::Windows::Error.new("out_buffer is required")
     end
 
-    result = DeviceIoControl(
-      handle,
-      io_control_code,
-      in_buffer, in_buffer.nil? ? 0 : in_buffer.size,
-      out_buffer, out_buffer.size,
-      FFI::MemoryPointer.new(:dword, 1),
-      nil
-    )
+    FFI::MemoryPointer.new(:dword, 1) do |bytes_returned_ptr|
+      result = DeviceIoControl(
+        handle,
+        io_control_code,
+        in_buffer, in_buffer.nil? ? 0 : in_buffer.size,
+        out_buffer, out_buffer.size,
+        bytes_returned_ptr,
+        nil
+      )
+
+      if result == FFI::WIN32_FALSE
+        raise Puppet::Util::Windows::Error.new(
+          "DeviceIoControl(#{handle}, #{io_control_code}, " +
+          "#{in_buffer}, #{in_buffer ? in_buffer.size : ''}, " +
+          "#{out_buffer}, #{out_buffer ? out_buffer.size : ''}")
+      end
+    end
 
-    return out_buffer if result != FFI::WIN32_FALSE
-    raise Puppet::Util::Windows::Error.new(
-      "DeviceIoControl(#{handle}, #{io_control_code}, " +
-      "#{in_buffer}, #{in_buffer ? in_buffer.size : ''}, " +
-      "#{out_buffer}, #{out_buffer ? out_buffer.size : ''}")
+    out_buffer
   end
 
   FILE_ATTRIBUTE_REPARSE_POINT = 0x400
@@ -110,22 +126,28 @@ def symlink?(file_name)
   def self.open_symlink(link_name)
     begin
       yield handle = create_file(
-      wide_string(link_name.to_s),
+      link_name,
       GENERIC_READ,
       FILE_SHARE_READ,
       nil, # security_attributes
       OPEN_EXISTING,
       FILE_FLAG_OPEN_REPARSE_POINT | FILE_FLAG_BACKUP_SEMANTICS,
       0) # template_file
     ensure
-      CloseHandle(handle) if handle
+      FFI::WIN32.CloseHandle(handle) if handle
     end
+
+    # handle has had CloseHandle called against it, so nothing to return
+    nil
   end
 
   def readlink(link_name)
+    link = nil
     open_symlink(link_name) do |handle|
-      resolve_symlink(handle)
+      link = resolve_symlink(handle)
     end
+
+    link
   end
   module_function :readlink
 
@@ -178,16 +200,16 @@ def stat.ftype
   FSCTL_GET_REPARSE_POINT = 0x900a8
 
   def self.resolve_symlink(handle)
-    # must be multiple of 1024, min 10240
-    out_buffer = FFI::MemoryPointer.new(REPARSE_DATA_BUFFER.size)
-    device_io_control(handle, FSCTL_GET_REPARSE_POINT, nil, out_buffer)
+    path = nil
+    get_reparse_point_data(handle) do |reparse_data|
+      offset = reparse_data[:PrintNameOffset]
+      length = reparse_data[:PrintNameLength]
 
-    reparse_data = REPARSE_DATA_BUFFER.new(out_buffer)
-    offset = reparse_data[:PrintNameOffset]
-    length = reparse_data[:PrintNameLength]
+      ptr = reparse_data.pointer + reparse_data.offset_of(:PathBuffer) + offset
+      path = ptr.read_wide_string(length / 2) # length is bytes, need UTF-16 wchars
+    end
 
-    result = reparse_data[:PathBuffer].to_a[offset, length].pack('C*')
-    result.force_encoding('UTF-16LE').encode(Encoding.default_external)
+    path
   end
 
   ffi_convention :stdcall
@@ -284,11 +306,4 @@ class REPARSE_DATA_BUFFER < FFI::Struct
            # technically a WCHAR buffer, but we care about size in bytes here
            :PathBuffer, [:byte, MAXIMUM_REPARSE_DATA_BUFFER_SIZE - 20]
   end
-
-  # http://msdn.microsoft.com/en-us/library/windows/desktop/ms724211(v=vs.85).aspx
-  # BOOL WINAPI CloseHandle(
-  #   _In_  HANDLE hObject
-  # );
-  ffi_lib :kernel32
-  attach_function_private :CloseHandle, [:handle], :win32_bool
 end