Conversation
|
|
||
| rwo->closed = 1; | ||
| xmlFreeTextWriter(rwo->writer); | ||
|
|
There was a problem hiding this comment.
Is there a reason to reverse the order?
There was a problem hiding this comment.
Yes — the order is intentionally reversed from the original #if 0 code. xmlFreeTextWriter internally calls xmlOutputBufferClose, which can flush pending output to the buffer during cleanup. Freeing the buffer first (as in the old disabled code) would be a use-after-free. By saving the buffer pointer beforehand and calling xmlBufferFree only after xmlFreeTextWriter completes, we ensure the writer has fully shut down
|
Thanks - left one comment. |
|
For some reason tests are totally broken now, so I would like to look at that before merging this PR. |
I looked at these and added two commits: one was that the the libdir directive in the Rakefile had a regex that would only match a single digit and since 3.2 and 3.3 have patch digits The other was deprecated minitest assertion - if you want to assert something is nil you have to explicitly use assert_nil I think both of these issues are pre-existing to my changes but hopefully this is helpful. The Windows builds were failing at least as far back as #220 |
|
I agree they were pre-existing, still need to be fixed. I was going to address separately. But thanks for looking. I will just merge this now, and then if you want to get the rest of the tests working that would be great. Otherwise I will take a look. |
sorry line 75 should have been |
Summary
XML::Writer.stringleaks thexmlBufferallocated byxmlBufferCreateon every instance. The buffer is never freed because thexmlBufferFreecall inrxml_writer_freeis disabled by#if 0.Root Cause
In
ext/libxml/ruby_xml_writer.c, the GC finalizerrxml_writer_free(line 44) has thexmlBufferFreecall commented out:The comment "seems to be done by xmlFreeTextWriter" is incorrect.
xmlFreeTextWritercallsxmlOutputBufferClosewhich frees thexmlOutputBufferwrapper, but does NOT free the underlyingxmlBufferthat was passed toxmlNewTextWriterMemory. The libxml2 documentation forxmlNewTextWriterMemorydoes not state that the caller's buffer is adopted — unlikexmlNewTextWriterwhich explicitly notes "the out parameter will be deallocated when the writer is closed."The buffer is allocated at line 168:
And passed to libxml2 at line 172:
When the Ruby GC collects the Writer,
rxml_writer_freeruns,xmlFreeTextWriterfrees the writer and its output buffer, butrwo->bufferis leaked.Fix
Re-enable
xmlBufferFreeinrxml_writer_free. The buffer pointer is saved before callingxmlFreeTextWriterin case the writer modifies the struct during cleanup:The order matters:
xmlFreeTextWritermust run first to flush pending output to the buffer, then the buffer is freed.Test
Add a regression in
test/test_writer.rbthat repeatedly createsXML::Writer.stringinstances, forces GC between rounds, and asserts RSS stays flat after warm-up.Without the fix, RSS kept growing across rounds. With the fix, it stabilizes.
Verification
bundle exec ruby -Itest test/test_writer.rbbundle exec ruby -Itest test/test_xml.rb