-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Crash compiling code that just require xml without using it. #10435
Comments
Requiring xml is enough to use it. It has some main initialization. Check references to LibXML.xmlGcMemSetup to find it. |
I guess I found the issue... |
yep, I reduced it to: require "xml"
{% if compare_versions(Crystal::VERSION, "0.35.0-0") >= 0 %}
@[Link("xml2", pkg_config: "libxml-2.0")]
{% else %}
@[Link("xml2")]
{% end %}
lib LibXML
fun xmlGcMemSetup(free_func : Void* ->,
malloc_func : LibC::SizeT -> Void*,
malloc_atomic_func : LibC::SizeT -> Void*,
realloc_func : Void*, LibC::SizeT -> Void*,
strdup_func : UInt8* -> UInt8*) : Int
end
LibXML.xmlGcMemSetup(
->GC.free,
->GC.malloc(LibC::SizeT),
->GC.malloc(LibC::SizeT),
->GC.realloc(Void*, LibC::SizeT),
->(str) {
len = LibC.strlen(str) + 1
copy = Pointer(UInt8).malloc(len)
copy.copy_from(str, len)
copy
}
)
@[Link("libvirt")]
lib LibVirt
alias ConnectPtr = Void*
fun connect_open = virConnectOpen(name : LibC::Char*) : ConnectPtr
fun connect_close = virConnectClose(conn : ConnectPtr) : Int32
end
ptr = LibVirt.connect_open("qemu:///system")
LibVirt.connect_close(ptr) So... it's not a Crystal bug, but IMO it deservers a mention on XML module documentation. |
I was able to use libvirt with Crystal libxml2 bindings by doing this workaround: require "xml"
@[Link("libvirt")]
lib LibVirt
alias ConnectPtr = Void*
fun connect_open = virConnectOpen(name : LibC::Char*) : ConnectPtr
fun connect_close = virConnectClose(conn : ConnectPtr) : Int32
end
def no_xmlgc
LibXML.xmlGcMemSetup(->LibC.free,
->LibC.malloc(LibC::SizeT),
->LibC.malloc(LibC::SizeT),
->LibC.realloc(Void*, LibC::SizeT),
->(str) {
len = LibC.strlen(str) + 1
copy = Pointer(UInt8).malloc(len)
copy.copy_from(str, len)
copy
})
yield
LibXML.xmlGcMemSetup(->GC.free,
->GC.malloc(LibC::SizeT),
->GC.malloc(LibC::SizeT),
->GC.realloc(Void*, LibC::SizeT),
->(str) {
len = LibC.strlen(str) + 1
copy = Pointer(UInt8).malloc(len)
copy.copy_from(str, len)
copy
})
end
no_xmlgc do
ptr = LibVirt.connect_open("qemu:///system")
pp! LibVirt.connect_close(ptr)
end
pp! XML.parse("<test />") Works... but is bad, because on multiple threads I believe that this need to be protected by a mutex or the caos will reign.... there's also the little overhead of 2 function calls. So I think the correct to do this is let Crystal XML bindings don't use the GC, but manage its memory on some |
The use case were I found this was rewriting a small ruby JSON-RPC server used by the company I'm working to Crystal, the application uses libvirt and also threads, in Ruby it need to use multiple process sometimes (and it's a hell) to keep the server responsive during some slow libvirt operations, the same might happen on Crystal if using fibers in the same thread, anyway.... as the XML use I do is really simple, I can workaround it by calling require "xml"
lib LibXML
fun xmlFreeNode(cur : Node*) : Void
end
module XML
struct Node
def free
LibXML.xmlFreeNode(@node)
end
end
end
@[Link("libvirt")]
lib LibVirt
alias ConnectPtr = Void*
fun connect_open = virConnectOpen(name : LibC::Char*) : ConnectPtr
fun connect_close = virConnectClose(conn : ConnectPtr) : Int32
end
LibXML.xmlGcMemSetup(->LibC.free, ->LibC.malloc(LibC::SizeT), ->LibC.malloc(LibC::SizeT), ->LibC.realloc(Void*, LibC::SizeT), ->(str) {
len = LibC.strlen(str) + 1
copy = Pointer(UInt8).malloc(len)
copy.copy_from(str, len)
copy
})
ptr = LibVirt.connect_open("qemu:///system")
pp! LibVirt.connect_close(ptr)
node = XML.parse("<test />")
node.free Doing so I noticed that the XML in stdlib classes are structs (probably for performance issues), not classes, so they don't have a finalize method and therefore my suggestion about let Crystal XML module manage the libXML2 memory can't be done without changing this. |
The finalize hook would need to be on the libxml2 data structur ( But the best solution would probably be to let libvirt use the libxml2 configuration from Crystal. Because the main program runs in Crystal's runtime, libraries should adopt to that, not the other way around. |
I think the best solution you mentioned isn't feasible, since most libraries that use libxml2 have no way to configure that and don't use a GC, calling the libXML2 free functions themselves... Even if we convince all projects using libXML2 to fill their projects with As I need the xml module in my application that uses libvirt, I'll end up with the worst solution... fork the xml module and apply these minor patches replacing |
I just looked how this works in Ruby with nokogiri and libvirt-ruby. It seems nokogiri does not set up libxml2 to use the GC. I suppose they do the same as your patch and free libxml2 memory when the Ruby objects are garbage-collected. So I suggest we should apply your patch. |
There's no patch ready yet 😛, but I can write one. The time to do this is now... since the changes from |
Why does |
|
BTW, the fix isn't just change struct by class and add some finalizers... since i.e. The fix isn't soooo simple as I expected. |
That's the beauty of being able to hook the libxml memory functions to a GC which takes care of all of this for us 😅 |
One patch that could be in 1.0.0 is to keep the GC taking care of libXML as it is now and just change the |
I think the change from struct to class can happen now and later we can figure ways to setup the GC. |
This will allow a future change to let the XML module manage the libXML2 memory, fixing crystal-lang#10435 without breaking the API.
This MR was easy to do :-) |
This will allow a future change to let the XML module manage the libXML2 memory, fixing #10435 without breaking the API.
@hugopl , were you able to use libvirt directly on Crystal, without Ruby? |
Yes, it's possible to use, you just can't use/include the XML module from stdlib, since it register hooks to free the memory using the GC.. then when libvirt tries to also free the memory it causes a double free. |
The following code crashes with the following stacktrace:
If I just comment the first line,
require "xml"
, the code works correctly and no crash happens. The weird thing is that no XML code is explicit used by the example.Tested with:
The text was updated successfully, but these errors were encountered: