Merge pull request #296 from sled/bugfix/sled-295-ffi-pointer-size

jcupitt · web-flow · commit c9c7b566c4f6 · 2021-05-06T11:58:18.000+01:00
Fix FFI::Pointer handling in Image#new_from_memory and Image#new_from_memory_copy
diff --git a/lib/vips/image.rb b/lib/vips/image.rb
@@ -78,6 +78,11 @@ def self.p2str(pointer)
   class Image < Vips::Object
     alias_method :parent_get_typeof, :get_typeof
 
+    # FFI sets a pointer's size to this magic value if the size of the memory
+    # chunk the pointer points to is unknown to FFI.
+    UNKNOWN_POINTER_SIZE = FFI::Pointer.new(1).size
+    private_constant :UNKNOWN_POINTER_SIZE
+
     private
 
     # the layout of the VipsImage struct
@@ -326,6 +331,24 @@ def self.new_from_buffer data, option_string, **opts
     #   image.width, image.height, image.bands, image.format
     # ```
     #
+    # Creating a new image from a memory pointer:
+    #
+    # ```
+    # ptr = FFI::MemoryPointer.new(:uchar, 10*10)
+    # # => #<FFI::MemoryPointer address=0x00007fc236db31d0 size=100>
+    # x = Vips::Image.new_from_memory(ptr, 10, 10, 1, :uchar)
+    # ```
+    #
+    # Creating a new image from an address only pointer:
+    #
+    # ```
+    # ptr = call_to_external_c_library(w: 10, h: 10)
+    # # => #<FFI::Pointer address=0x00007f9780813a00>
+    # ptr_slice = ptr.slice(0, 10*10)
+    # # => #<FFI::Pointer address=0x00007f9780813a00 size=100>
+    # x = Vips::Image.new_from_memory(ptr_slice, 10, 10, 1, :uchar)
+    # ```
+    #
     # {new_from_memory} keeps a reference to the array of pixels you pass in
     # to try to prevent that memory from being freed by the Ruby GC while it
     # is being used.
@@ -340,13 +363,23 @@ def self.new_from_buffer data, option_string, **opts
     # @param format [Symbol] band format
     # @return [Image] the loaded image
     def self.new_from_memory data, width, height, bands, format
-      size = data.bytesize
-
       # prevent data from being freed with JRuby FFI
       if defined?(JRUBY_VERSION) && !data.is_a?(FFI::Pointer)
         data = ::FFI::MemoryPointer.new(:char, data.bytesize).write_bytes data
       end
 
+      if data.is_a?(FFI::Pointer)
+        # A pointer needs to know about the size of the memory it points to.
+        # If you have an address-only pointer, use the .slice method to wrap
+        # the pointer in a size aware pointer.
+        if data.size == UNKNOWN_POINTER_SIZE
+          raise Vips::Error, "size of memory is unknown"
+        end
+        size = data.size
+      else
+        size = data.bytesize
+      end
+
       format_number = GObject::GValue.from_nick BAND_FORMAT_TYPE, format
       vi = Vips.vips_image_new_from_memory data, size,
         width, height, bands, format_number
@@ -373,7 +406,17 @@ def self.new_from_memory data, width, height, bands, format
     # @return [Image] the loaded image
     def self.new_from_memory_copy data, width, height, bands, format
       format_number = GObject::GValue.from_nick BAND_FORMAT_TYPE, format
-      vi = Vips.vips_image_new_from_memory_copy data, data.bytesize,
+
+      if data.is_a?(FFI::Pointer)
+        if data.size == UNKNOWN_POINTER_SIZE
+          raise Vips::Error, "size of memory is unknown"
+        end
+        size = data.size
+      else
+        size = data.bytesize
+      end
+
+      vi = Vips.vips_image_new_from_memory_copy data, size,
         width, height, bands, format_number
       raise Vips::Error if vi.null?
       new(vi)
diff --git a/spec/image_spec.rb b/spec/image_spec.rb
@@ -44,6 +44,47 @@ def has_jpeg?
     expect(x.avg).to eq(128)
   end
 
+  it "can load an image from memory by memory pointer" do
+    data = FFI::MemoryPointer.new(:uchar, 16 * 16)
+    data.put_array_of_uchar(0, Array.new(16 * 16, 128))
+
+    x = Vips::Image.new_from_memory data, 16, 16, 1, :uchar
+
+    # GC to try to trigger a segv if data hasn't been reffed by
+    # new_from_memory
+    GC.start
+
+    expect(x.width).to eq(16)
+    expect(x.height).to eq(16)
+    expect(x.bands).to eq(1)
+    expect(x.avg).to eq(128)
+  end
+
+  it "can load an image from memory by size aware address pointer" do
+    memory = FFI::MemoryPointer.new(:uchar, 16 * 16)
+    memory.put_array_of_uchar(0, Array.new(16 * 16, 128))
+
+    data = FFI::Pointer.new(memory)
+    # JRuby's FFI implementation looses the size information
+    data = data.slice(0, 16 * 16) if defined?(JRUBY_VERSION)
+
+    x = Vips::Image.new_from_memory data, 16, 16, 1, :uchar
+
+    # GC to try to trigger a segv if data hasn't been reffed by
+    # new_from_memory
+    GC.start
+
+    expect(x.width).to eq(16)
+    expect(x.height).to eq(16)
+    expect(x.bands).to eq(1)
+    expect(x.avg).to eq(128)
+  end
+
+  it "throws an error when trying to load an image from memory with unknown size" do
+    data = FFI::Pointer.new(1)
+    expect { Vips::Image.new_from_memory(data, 16, 16, 1, :uchar) }.to raise_error(Vips::Error)
+  end
+
   it "can load an image from memory and copy" do
     image = Vips::Image.black(16, 16) + 128
     data = image.write_to_memory
@@ -57,6 +98,39 @@ def has_jpeg?
     expect(x.avg).to eq(128)
   end
 
+  it "can load and copy an image from memory by memory pointer" do
+    data = FFI::MemoryPointer.new(:uchar, 16 * 16)
+    data.put_array_of_uchar(0, Array.new(16 * 16, 128))
+
+    x = Vips::Image.new_from_memory_copy data, 16, 16, 1, :uchar
+
+    expect(x.width).to eq(16)
+    expect(x.height).to eq(16)
+    expect(x.bands).to eq(1)
+    expect(x.avg).to eq(128)
+  end
+
+  it "can load and copy an image from memory by size aware address pointer" do
+    memory = FFI::MemoryPointer.new(:uchar, 16 * 16)
+    memory.put_array_of_uchar(0, Array.new(16 * 16, 128))
+
+    data = FFI::Pointer.new(memory)
+    # JRuby's FFI implementation looses the size information
+    data = data.slice(0, 16 * 16) if defined?(JRUBY_VERSION)
+
+    x = Vips::Image.new_from_memory_copy data, 16, 16, 1, :uchar
+
+    expect(x.width).to eq(16)
+    expect(x.height).to eq(16)
+    expect(x.bands).to eq(1)
+    expect(x.avg).to eq(128)
+  end
+
+  it "throws an error when trying to load and copy from memory with unknown size" do
+    data = FFI::Pointer.new(1)
+    expect { Vips::Image.new_from_memory_copy(data, 16, 16, 1, :uchar) }.to raise_error(Vips::Error)
+  end
+
   if has_jpeg?
     it "can save an image to a buffer" do
       image = Vips::Image.black(16, 16) + 128
