Merge pull request #223 from jcalvert/fix/writer-string-buffer-leak

Fix XML::Writer.string buffer leak

Author: cfis

Rakefile

@@ -20,7 +20,7 @@ Rake::ExtensionTask.new do |ext|
   ext.gem_spec = spec
   ext.name = SO_NAME
   ext.ext_dir = "ext/libxml"
-  ext.lib_dir = "lib/#{RUBY_VERSION.sub(/\.\d$/, '')}"
+  ext.lib_dir = "lib/#{RUBY_VERSION.sub(/\.\d+$/, '')}"
   if RUBY_PLATFORM.match(/mswin|mingw/)
     ext.config_options <<
       if (dir = ENV['WINDOWS_XML2_INCLUDE'])

ext/libxml/ruby_xml_writer.c

@@ -43,15 +43,16 @@ typedef struct
 
 static void rxml_writer_free(rxml_writer_object* rwo)
 {
-#if 0 /* seems to be done by xmlFreeTextWriter */
-    if (NULL != rwo->buffer)
-    {
-        xmlBufferFree(rwo->buffer);
-    }
-#endif
+    xmlBufferPtr buffer = rwo->buffer;
 
     rwo->closed = 1;
     xmlFreeTextWriter(rwo->writer);
+
+    if (NULL != buffer)
+    {
+        xmlBufferFree(buffer);
+    }
+
     xfree(rwo);
 }
 
@@ -1121,4 +1122,3 @@ void rxml_init_writer(void)
     rb_undef_method(CLASS_OF(cXMLWriter), "new");
 #endif
 }
-

test/test_error.rb

@@ -71,8 +71,11 @@ def test_get_handler
     else
       LibXML::XML::Error.reset_handler
     end
-
-    assert_equal(LibXML::XML::Error.get_handler, saved_handler)
+    if saved_handler.nil?
+      assert_nil(LibXML::XML::get_handler)
+    else
+      assert_equal(LibXML::XML::Error.get_handler, saved_handler)
+    end
   end
 
   def test_verbose_handler

test/test_writer.rb

@@ -446,6 +446,31 @@ def test_nil_pe_issue
     assert_equal(writer.result, expected)
   end
 
+  def test_string_writer_does_not_leak_memory
+    skip('RSS check not supported on Windows') if windows?
+
+    rss_by_round = []
+
+    4.times do
+      5_000.times do
+        writer = LibXML::XML::Writer.string
+        document(writer, encoding: LibXML::XML::Encoding::UTF_8) do
+          writer.write_element('test', 'hello')
+        end
+        writer.result
+      end
+
+      GC.start(full_mark: true, immediate_sweep: true)
+
+      rss = rss_in_kb
+      skip('RSS check not available') unless rss
+
+      rss_by_round << rss
+    end
+
+    assert_operator(rss_by_round.last - rss_by_round[1], :<, 5_000)
+  end
+
   private
 
   def document(writer, options = {})
@@ -465,4 +490,11 @@ def element(writer, localname)
     yield if block_given?
     assert(writer.end_element)
   end
+
+  def rss_in_kb
+    rss = `ps -o rss= -p #{Process.pid}`.strip
+    Integer(rss)
+  rescue Errno::ENOENT, Errno::EPERM, ArgumentError
+    nil
+  end
 end