[ruby/fiddle] Improve documentation on how to correctly free memory and free memory in tests (#33)

https://github.com/ruby/fiddle/commit/e59cfd708a

6 files changed, 125 insertions, 53 deletions

diff --git a/ext/fiddle/lib/fiddle/struct.rb b/ext/fiddle/lib/fiddle/struct.rb
index 8a48a1cb8b..259903d25c 100644
--- a/ext/fiddle/lib/fiddle/struct.rb
+++ b/ext/fiddle/lib/fiddle/struct.rb
@@ -73,7 +73,12 @@ module Fiddle
     #
     #   MyStruct = Fiddle::CStructBuilder.create(Fiddle::CUnion, types, members)
     #
-    #   obj = MyStruct.allocate
+    #   obj = MyStruct.malloc
+    #   begin
+    #     ...
+    #   ensure
+    #     Fiddle.free obj.to_ptr
+    #   end
     #
     def create(klass, types, members)
       new_class = Class.new(klass){
@@ -112,7 +117,7 @@ module Fiddle
 
     # Allocates a C struct with the +types+ provided.
     #
-    # When the instance is garbage collected, the C function +func+ is called.
+    # See Fiddle::Pointer.malloc for memory management issues.
     def CStructEntity.malloc(types, func = nil)
       addr = Fiddle.malloc(CStructEntity.size(types))
       CStructEntity.new(addr, types, func)
@@ -267,7 +272,7 @@ module Fiddle
 
     # Allocates a C union the +types+ provided.
     #
-    # When the instance is garbage collected, the C function +func+ is called.
+    # See Fiddle::Pointer.malloc for memory management issues.
     def CUnionEntity.malloc(types, func=nil)
       addr = Fiddle.malloc(CUnionEntity.size(types))
       CUnionEntity.new(addr, types, func)
diff --git a/ext/fiddle/pointer.c b/ext/fiddle/pointer.c
index 117cc9b826..7c60da477a 100644
--- a/ext/fiddle/pointer.c
+++ b/ext/fiddle/pointer.c
@@ -193,14 +193,34 @@ rb_fiddle_ptr_initialize(int argc, VALUE argv[], VALUE self)
 
 /*
  * call-seq:
- *
  *    Fiddle::Pointer.malloc(size, freefunc = nil)  => fiddle pointer instance
  *
+ * == Examples
+ *
+ *    # Relying on the garbage collector - may lead to unlimited memory allocated before freeing any, but safe
+ *    pointer = Fiddle::Pointer.malloc(size, Fiddle::RUBY_FREE)
+ *    ...
+ *
+ *    # Manual freeing
+ *    pointer = Fiddle::Pointer.malloc(size)
+ *    begin
+ *      ...
+ *    ensure
+ *      Fiddle.free pointer
+ *    end
+ *
+ *    # No free function and no call to free - the native memory will leak if the pointer is garbage collected
+ *    pointer = Fiddle::Pointer.malloc(size)
+ *    ...
+ *
  * Allocate +size+ bytes of memory and associate it with an optional
  * +freefunc+ that will be called when the pointer is garbage collected.
- *
  * +freefunc+ must be an address pointing to a function or an instance of
- * Fiddle::Function
+ * +Fiddle::Function+. Using +freefunc+ may lead to unlimited memory being
+ * allocated before any is freed as the native memory the pointer references
+ * does not contribute to triggering the Ruby garbage collector. Consider
+ * manually freeing the memory as illustrated above. You cannot combine
+ * the techniques as this may lead to a double-free.
  */
 static VALUE
 rb_fiddle_ptr_s_malloc(int argc, VALUE argv[], VALUE klass)
diff --git a/test/fiddle/test_c_struct_entry.rb b/test/fiddle/test_c_struct_entry.rb
index 8ece438f54..33bfee6218 100644
--- a/test/fiddle/test_c_struct_entry.rb
+++ b/test/fiddle/test_c_struct_entry.rb
@@ -43,7 +43,7 @@ module Fiddle
     end
 
     def test_set_ctypes
-      union = CStructEntity.malloc [TYPE_INT, TYPE_LONG]
+      union = CStructEntity.malloc [TYPE_INT, TYPE_LONG], Fiddle::RUBY_FREE
       union.assign_names %w[int long]
 
       # this test is roundabout because the stored ctypes are not accessible
@@ -55,20 +55,20 @@ module Fiddle
     end
 
     def test_aref_pointer_array
-      team = CStructEntity.malloc([[TYPE_VOIDP, 2]])
+      team = CStructEntity.malloc([[TYPE_VOIDP, 2]], Fiddle::RUBY_FREE)
       team.assign_names(["names"])
-      alice = Fiddle::Pointer.malloc(6)
+      alice = Fiddle::Pointer.malloc(6, Fiddle::RUBY_FREE)
       alice[0, 6] = "Alice\0"
-      bob = Fiddle::Pointer.malloc(4)
+      bob = Fiddle::Pointer.malloc(4, Fiddle::RUBY_FREE)
       bob[0, 4] = "Bob\0"
       team["names"] = [alice, bob]
       assert_equal(["Alice", "Bob"], team["names"].map(&:to_s))
     end
 
     def test_aref_pointer
-      user = CStructEntity.malloc([TYPE_VOIDP])
+      user = CStructEntity.malloc([TYPE_VOIDP], Fiddle::RUBY_FREE)
       user.assign_names(["name"])
-      alice = Fiddle::Pointer.malloc(6)
+      alice = Fiddle::Pointer.malloc(6, Fiddle::RUBY_FREE)
       alice[0, 6] = "Alice\0"
       user["name"] = alice
       assert_equal("Alice", user["name"].to_s)
diff --git a/test/fiddle/test_c_union_entity.rb b/test/fiddle/test_c_union_entity.rb
index 5727a20e3b..9310084733 100644
--- a/test/fiddle/test_c_union_entity.rb
+++ b/test/fiddle/test_c_union_entity.rb
@@ -21,7 +21,7 @@ module Fiddle
     end
 
     def test_set_ctypes
-      union = CUnionEntity.malloc [TYPE_INT, TYPE_LONG]
+      union = CUnionEntity.malloc [TYPE_INT, TYPE_LONG], Fiddle::RUBY_FREE
       union.assign_names %w[int long]
 
       # this test is roundabout because the stored ctypes are not accessible
diff --git a/test/fiddle/test_import.rb b/test/fiddle/test_import.rb
index 867f6960ee..fce8b864d9 100644
--- a/test/fiddle/test_import.rb
+++ b/test/fiddle/test_import.rb
@@ -57,35 +57,56 @@ module Fiddle
     def test_struct_memory_access()
       # check memory operations performed directly on struct
       my_struct = Fiddle::Importer.struct(['int id']).malloc
-      my_struct[0, Fiddle::SIZEOF_INT] = "\x01".b * Fiddle::SIZEOF_INT
-      assert_equal 0x01010101, my_struct.id
-
-      my_struct.id = 0
-      assert_equal "\x00".b * Fiddle::SIZEOF_INT, my_struct[0, Fiddle::SIZEOF_INT]
+      begin
+        my_struct[0, Fiddle::SIZEOF_INT] = "\x01".b * Fiddle::SIZEOF_INT
+        assert_equal 0x01010101, my_struct.id
+
+        my_struct.id = 0
+        assert_equal "\x00".b * Fiddle::SIZEOF_INT, my_struct[0, Fiddle::SIZEOF_INT]
+      ensure
+        Fiddle.free my_struct.to_ptr
+      end
     end
 
     def test_struct_ptr_array_subscript_multiarg()
       # check memory operations performed on struct#to_ptr
       struct = Fiddle::Importer.struct([ 'int x' ]).malloc
-      ptr = struct.to_ptr
+      begin
+        ptr = struct.to_ptr
 
-      struct.x = 0x02020202
-      assert_equal("\x02".b * Fiddle::SIZEOF_INT, ptr[0, Fiddle::SIZEOF_INT])
+        struct.x = 0x02020202
+        assert_equal("\x02".b * Fiddle::SIZEOF_INT, ptr[0, Fiddle::SIZEOF_INT])
 
-      ptr[0, Fiddle::SIZEOF_INT] = "\x01".b * Fiddle::SIZEOF_INT
-      assert_equal 0x01010101, struct.x
+        ptr[0, Fiddle::SIZEOF_INT] = "\x01".b * Fiddle::SIZEOF_INT
+        assert_equal 0x01010101, struct.x
+      ensure
+        Fiddle.free struct.to_ptr
+      end
     end
 
     def test_malloc()
       s1 = LIBC::Timeval.malloc()
-      s2 = LIBC::Timeval.malloc()
-      refute_equal(s1.to_ptr.to_i, s2.to_ptr.to_i)
+      begin
+        s2 = LIBC::Timeval.malloc()
+        begin
+          refute_equal(s1.to_ptr.to_i, s2.to_ptr.to_i)
+        ensure
+          Fiddle.free s2.to_ptr
+        end
+      ensure
+        Fiddle.free s1.to_ptr
+      end
     end
 
     def test_sizeof()
       assert_equal(SIZEOF_VOIDP, LIBC.sizeof("FILE*"))
       assert_equal(LIBC::MyStruct.size(), LIBC.sizeof(LIBC::MyStruct))
-      assert_equal(LIBC::MyStruct.size(), LIBC.sizeof(LIBC::MyStruct.malloc()))
+      my_struct = LIBC::MyStruct.malloc()
+      begin
+        assert_equal(LIBC::MyStruct.size(), LIBC.sizeof(my_struct))
+      ensure
+        Fiddle.free my_struct.to_ptr
+      end
       assert_equal(SIZEOF_LONG_LONG, LIBC.sizeof("long long")) if defined?(SIZEOF_LONG_LONG)
     end
 
@@ -131,35 +152,51 @@ module Fiddle
 
     def test_struct_array_assignment()
       instance = Fiddle::Importer.struct(["unsigned int stages[3]"]).malloc
-      instance.stages[0] = 1024
-      instance.stages[1] = 10
-      instance.stages[2] = 100
-      assert_equal 1024, instance.stages[0]
-      assert_equal 10, instance.stages[1]
-      assert_equal 100, instance.stages[2]
-      assert_equal [1024, 10, 100].pack(Fiddle::PackInfo::PACK_MAP[-Fiddle::TYPE_INT] * 3),
-                   instance.to_ptr[0, 3 * Fiddle::SIZEOF_INT]
-      assert_raise(IndexError) { instance.stages[-1] = 5 }
-      assert_raise(IndexError) { instance.stages[3] = 5 }
+      begin
+        instance.stages[0] = 1024
+        instance.stages[1] = 10
+        instance.stages[2] = 100
+        assert_equal 1024, instance.stages[0]
+        assert_equal 10, instance.stages[1]
+        assert_equal 100, instance.stages[2]
+        assert_equal [1024, 10, 100].pack(Fiddle::PackInfo::PACK_MAP[-Fiddle::TYPE_INT] * 3),
+                    instance.to_ptr[0, 3 * Fiddle::SIZEOF_INT]
+        assert_raise(IndexError) { instance.stages[-1] = 5 }
+        assert_raise(IndexError) { instance.stages[3] = 5 }
+      ensure
+        Fiddle.free instance.to_ptr
+      end
     end
 
     def test_struct()
       s = LIBC::MyStruct.malloc()
-      s.num = [0,1,2,3,4]
-      s.c = ?a.ord
-      s.buff = "012345\377"
-      assert_equal([0,1,2,3,4], s.num)
-      assert_equal(?a.ord, s.c)
-      assert_equal([?0.ord,?1.ord,?2.ord,?3.ord,?4.ord,?5.ord,?\377.ord], s.buff)
+      begin
+        s.num = [0,1,2,3,4]
+        s.c = ?a.ord
+        s.buff = "012345\377"
+        assert_equal([0,1,2,3,4], s.num)
+        assert_equal(?a.ord, s.c)
+        assert_equal([?0.ord,?1.ord,?2.ord,?3.ord,?4.ord,?5.ord,?\377.ord], s.buff)
+      ensure
+        Fiddle.free s.to_ptr
+      end
     end
 
     def test_gettimeofday()
       if( defined?(LIBC.gettimeofday) )
         timeval = LIBC::Timeval.malloc()
-        timezone = LIBC::Timezone.malloc()
-        LIBC.gettimeofday(timeval, timezone)
-        cur = Time.now()
-        assert(cur.to_i - 2 <= timeval.tv_sec && timeval.tv_sec <= cur.to_i)
+        begin
+          timezone = LIBC::Timezone.malloc()
+          begin
+            LIBC.gettimeofday(timeval, timezone)
+          ensure
+            Fiddle.free timezone.to_ptr
+          end
+          cur = Time.now()
+          assert(cur.to_i - 2 <= timeval.tv_sec && timeval.tv_sec <= cur.to_i)
+        ensure
+          Fiddle.free timeval.to_ptr
+        end
       end
     end
 
diff --git a/test/fiddle/test_pointer.rb b/test/fiddle/test_pointer.rb
index 5581c1dea7..c69e4f7142 100644
--- a/test/fiddle/test_pointer.rb
+++ b/test/fiddle/test_pointer.rb
@@ -84,7 +84,7 @@ module Fiddle
     end
 
     def test_to_ptr_io
-      buf = Pointer.malloc(10)
+      buf = Pointer.malloc(10, Fiddle::RUBY_FREE)
       File.open(__FILE__, 'r') do |f|
         ptr = Pointer.to_ptr f
         fread = Function.new(@libc['fread'],
@@ -145,7 +145,11 @@ module Fiddle
 
     def test_free
       ptr = Pointer.malloc(4)
-      assert_nil ptr.free
+      begin
+        assert_nil ptr.free
+      ensure
+        Fiddle.free ptr
+      end
     end
 
     def test_free=
@@ -173,15 +177,21 @@ module Fiddle
 
     def test_size
       ptr = Pointer.malloc(4)
-      assert_equal 4, ptr.size
-      Fiddle.free ptr.to_i
+      begin
+        assert_equal 4, ptr.size
+      ensure
+        Fiddle.free ptr
+      end
     end
 
     def test_size=
       ptr = Pointer.malloc(4)
-      ptr.size = 10
-      assert_equal 10, ptr.size
-      Fiddle.free ptr.to_i
+      begin
+        ptr.size = 10
+        assert_equal 10, ptr.size
+      ensure
+        Fiddle.free ptr
+      end
     end
 
     def test_aref_aset