Don't cleanup doc reference of unlinked node. #34
Conversation
|
This was introduced to solve #24. |
|
Cleanup doc reference is incorrect. We need to solve problem of lifetime document and node because node is a part of document and it must be freed before document. In #24 node created in one document is moved to another. I don't know correctly it or not. function libxml2.xmlRemoveNode(node)
xml2.xmlUnlinkNode(node)
xml2.xmlFreeNode(node)
end |
|
xmlua have big problem with lifetime of XmlNode. Every time while we use XPath or any other methods that wrap XmlNode to ffi.gc we create unique lua object with same C-level pointers. This is cause of all memory issues. For example: local xmlua = require "xmlua"
local xml = [[<?xml version='1.0' encoding = 'windows-1251'?>
<ROOT>
<A>
<B>
<C>1</C>
<C>1</C>
<C>1</C>
</B>
<B>
<C>1</C>
<C>1</C>
<C>1</C>
</B>
<X>
<C>1</C>
<C>1</C>
<C>1</C>
</X>
<X>
<C>1</C>
<C>1</C>
<C>1</C>
</X>
</A>
</ROOT>]]
local doc = xmlua.XML.parse(xml)
local a = doc:search("/ROOT/A")
local b = doc:search("/ROOT/A/B")
a:unlink()
a = nil
collectgarbage("collect")
collectgarbage("collect")
b:unlink()
b = nil
collectgarbage("collect")
collectgarbage("collect")In case: function libxml2.xmlUnlinkNode(node)
xml2.xmlUnlinkNode(node)
--xml2.xmlSetTreeDoc(node, ffi.NULL)
return ffi.gc(node, xml2.xmlFreeNode)
endwe have errors like: ==175939== Invalid read of size 4 with function libxml2.xmlUnlinkNode(node)
xml2.xmlUnlinkNode(node)
xml2.xmlSetTreeDoc(node, ffi.NULL)
return ffi.gc(node, xml2.xmlFreeNode)
endwe have errors like: ==176221== Invalid free() / delete / delete[] / realloc() And i don't know how to resolve this problem. |
|
IMHO, to solve partially the problem xml2.xmlSetTreeDoc(node, ffi.NULL) must be removed from This: function libxml2.xmlUnlinkNode(node)
xml2.xmlUnlinkNode(node)
xml2.xmlSetTreeDoc(node, ffi.NULL)
return ffi.gc(node, xml2.xmlFreeNode)
endabsolutely incorrect and this is a crash in most cases in xmlFreeNode. |
|
Small changes in #24: local xmlua = require("xmlua")
local function add_child(parent, child)
return parent:add_child(child)
end
local function create_element(name, attributes)
local elm = xmlua.XML.parse("<?xml version='1.0' encoding = 'windows-1251'?><" .. name .. "/>"):root()
if attributes then
for att_name, val in pairs(attributes) do
elm:set_attribute(att_name, val)
end
end
return elm:unlink()
end
do
local a = create_element("a", {
["a1"] = "a1-test",
["a2"] = "a2-test"
})
local b = create_element("b", {
["b1"] = "b1-test",
["b2"] = "b2-test"
})
add_child(a, b)
end
collectgarbage("collect")
collectgarbage("collect")function libxml2.xmlUnlinkNode(node)
xml2.xmlUnlinkNode(node)
xml2.xmlSetTreeDoc(node, ffi.NULL)
return ffi.gc(node, xml2.xmlFreeNode)
endproduces: xml2.xmlSetTreeDoc(node, ffi.NULL) - is not a guard from the problem. |
|
@kou I pushed various fixes for memory issues. Also fixed problem with moving unlinked node to another document. |
There was a problem hiding this comment.
In general, we don't merge a PR that has multiple logical changes.
You should split this to one PR per logical change.
I don't think your point (calling xml2.xmlSetTreeDoc(node, ffi.NULL) in libxml2.xmlUnlinkNode() is wrong) is the real problem.
I think that the real problem is xmlFreeDoc()/xmlFreeDoc() order. If both of xmlDoc and xmlNode are GC targets, LuaJIT may use freeing xmlDoc and then freeing xmlNode order. I think that it's the real problem.
If we think that "xmlua" is responsible for the case, "xmlua" needs to do something for the case.
If we think that "users" are responsible for the case, "xmlua" just needs to provide Node:free() that calls libxml2.xmlFreeNode() and "users" use it for the case.
|
@kou |
If we reset to NULL doc reference of cur->node (for example) this line https://github.com/GNOME/libxml2/blob/master/tree.c#L3775 will be work incorrectly. It will not see cur->doc->dict and try to use xmlFree(cur->name). This is a cause of Invalid Free because cur->name - is a dictionary entry. It may be a small chunk of big chunk allocated memory. |
|
Ok. I can split this patch but some later because i must fix issues in this library for production use. Now i want to make it stable for my cases. After that i will split it. |
Is this really change this situation? |
You are right. In small cases i can't make this situation but in big program: reverse order. But it happens only on destroy vm while service works this situation isn't happening. I will think how to solve it. Maybe add a list of unlinked nodes to document and free it on document destroy. This may be only in case of destroy last unlinked node. |
|
Maybe it works: function Document.new(raw_document, errors)
if not errors then
errors = {}
end
local unlinked_nodes = {}
local document = {
raw_document = raw_document,
errors = errors,
unlinked_nodes = unlinked_nodes
}
ffi.gc(document.raw_document, function(pdocument)
for _,node in ipairs(unlinked_nodes) do
print_dbg("Free unlinked: ", node)
libxml2.xmlFreeNode(node)
end
print_dbg("xmlFreeDoc: ", pdocument)
libxml2.xmlFreeDoc(pdocument)
end)
setmetatable(document, metatable)
return document
endfunction methods:unlink()
Node.unlink(self)
local unlinked_nodes = self.document.unlinked_nodes
table.insert(unlinked_nodes, self.node)
return self
endfunction libxml2.xmlUnlinkNode(node)
xml2.xmlUnlinkNode(node)
print_dbg("xmlUnlinkNode: ", node, ", doc=", node.doc)
endFirst checks is OK. |
|
Last version also works fine with: local xmlua = require "xmlua"
local xml = [[<?xml version='1.0' encoding = 'windows-1251'?>
<ROOT>
<A>
<B>
<C>1</C>
<C>1</C>
<C>1</C>
</B>
<B>
<C>1</C>
<C>1</C>
<C>1</C>
</B>
<X>
<C>1</C>
<C>1</C>
<C>1</C>
</X>
<X>
<C>1</C>
<C>1</C>
<C>1</C>
</X>
</A>
</ROOT>]]
local doc = xmlua.XML.parse(xml)
local a = doc:search("/ROOT/A")
local b = doc:search("/ROOT/A/B")
a:unlink()
a = nil
doc = nil
collectgarbage("collect")
collectgarbage("collect")
b:unlink()
b = nil
collectgarbage("collect")
collectgarbage("collect") |
ZigzagAK
force-pushed
the
master
branch
2 times, most recently
from
68ca520
to
c4fda2e
Compare
|
Fixed serialization memory leak if using custom encoding. libxml2 don't cleanup xmlSaveCtxtPtr->handler in xmlFreeSaveCtxt. I don't know why. |
This is cause if incorrect free in xmlFreeNode (DICT_FREE(cur->name)).
debug.log