search

KWARC / rust-libxml

Rust wrapper for libxml2
https://crates.io/crates/libxml
MIT License
76 stars 38 forks source link

safely unlink xpath-obtained nodes #51

Closed dginev closed 5 years ago

dginev commented 5 years ago

Fixes #50 , thanks to @caldwell for the report!

Our memory management strategy is a bit too subtle, since the timing criteria can start getting hard to trace when we have all of nodes, document and xpath nodesets interplaying.

The goal remains simple: we bookkeep all nodes in our rust _Document object, and avoid freeing any node individually, as long as they are linked to an owner document - we free the entire document on drop, and the nodes get cleaned up along with it. For unlinked nodes, the idea so far has been that we can safely call xmlFreeNode when they drop out of scope in Rust, and have them cleaned up. But that relied on us figuring out the ownership and timing of all related structs, so that an unlinked node is really only explicitly freed once (i.e. it's really unlinked).

I admit I start getting lost in tracking the drop + free sequence, and would need some tooling to be really sure I know what is going on...

The solution I propose ought to be safe & a general guard we could have included earlier -- when a node is marked as unlinked, it can be explicitly forgotten from the nodes map stored by its Document, ensuring that as soon as it goes out of scope in Rust, the drop+free combo will fire. Previously these nodes remained in Document until the doc itself went out of scope, typically at the end of the program, which causes additional headache in figuring out the drop order. By forgetting the node as soon as it is unlinked we get closer to safe Rust behavior, as far as I understand things.

The one bit I want to double-check tomorrow, before I merge this PR, is whether we are not accidentally leaking any memory now -- I will rerun the tests under valgrind to double-check.

dginev commented 5 years ago

Ok, I can confirm this particular changeset does not influence leaking memory in either xpath_tests or tree_tests, so I haven't missed any major regressions.