From 6a03544166fb5e9f658876d996dfd1f1882fb45b Mon Sep 17 00:00:00 2001 From: Brad House Date: Thu, 6 Feb 2025 08:54:27 -0500 Subject: [PATCH 1/2] context/data: memleak due to API (mis)use Memory management is a foreign concept to python in general, however it appears DNode and Context require .free() and .destroy() to be called, respectively, to avoid a memory leak. This seems easily avoidable by simply attaching a destructor to the class so the python garbage collector can clean up automatically when all references go out of scope. We add a reference to any DNode object derived from another DNode object (but not duplicated) so we can determine if we are allowed to be the one to destroy the DNode cdata, as well as to keep proper reference counting in case the owner goes out of scope but a dependent reference is still in scope. This change should also be compatible with existing integrations which may be aware of this as it sets the internal `cdata` reference to None and checks it to ensure it doesn't run the cleanup again. That said, I think calling DNode.free() in this case is still risky since there are no checks to ensure this is truly valid like the destructor does. Signed-off-by: Brad House --- libyang/context.py | 16 ++++-- libyang/data.py | 127 +++++++++++++++++++++++++++++---------------- 2 files changed, 94 insertions(+), 49 deletions(-) diff --git a/libyang/context.py b/libyang/context.py index fb4a330..ae551f4 100644 --- a/libyang/context.py +++ b/libyang/context.py @@ -259,6 +259,10 @@ def __init__( raise self.error("cannot create context") self.external_module_loader = ContextExternalModuleLoader(self.cdata) + + def __del__(self): + self.destroy() + def compile_schema(self): ret = lib.ly_ctx_compile(self.cdata) if ret != lib.LY_SUCCESS: @@ -269,7 +273,11 @@ def get_yanglib_data(self, content_id_format=""): ret = lib.ly_ctx_get_yanglib_data(self.cdata, dnode, str2c(content_id_format)) if ret != lib.LY_SUCCESS: raise self.error("cannot get yanglib data") - return DNode.new(self, dnode[0]) + # The refcnt_parent parameter here is Context rather than all other cases + # where it is DNode. Its not entirely clear how else to do it, pretty + # sure this returns internal memory that can't be passed to + # lyd_free_all() or lyd_free_tree() though docs on this aren't great. + return DNode.new(self, dnode[0], self) def destroy(self): if self.cdata is not None: @@ -466,7 +474,7 @@ def create_data_path( if not dnode: raise self.error("cannot find created path") - return DNode.new(self, dnode) + return DNode.new(self, dnode, parent) def parse_op( self, @@ -495,7 +503,7 @@ def parse_op( if ret != lib.LY_SUCCESS: raise self.error("failed to parse input data") - return DNode.new(self, op[0]) + return DNode.new(self, op[0], parent) def parse_op_mem( self, @@ -578,7 +586,7 @@ def parse_data( dnode = dnode[0] if dnode == ffi.NULL: return None - return DNode.new(self, dnode) + return DNode.new(self, dnode, parent) def parse_data_mem( self, diff --git a/libyang/data.py b/libyang/data.py index 0d63d3c..313d415 100644 --- a/libyang/data.py +++ b/libyang/data.py @@ -279,17 +279,44 @@ class DNode: __slots__ = ("context", "cdata", "attributes", "free_func", "__dict__") - def __init__(self, context: "libyang.Context", cdata): + def __init__(self, context: "libyang.Context", cdata, refcnt_parent): """ :arg context: The libyang.Context python object. :arg cdata: The pointer to the C structure allocated by libyang.so. + :arg refcnt_parent: + New nodes may be created with internal references to the internal + cdata tree. By holding a reference to the parent node that created + us, we can force Python to keep proper reference counts so the + destructor doesn't get called. The only time this may be set to + None is if we can guarantee we are the initial creator of the + cdata object. """ self.context = context self.cdata = cdata # C type: "struct lyd_node *" self.attributes = None self.free_func = None # type: Callable[DNode] + self.refcnt_parent = refcnt_parent + + def __del__(self): + # Don't auto-destroy the node unless we know we're allowed. We can + # determine this if refcnt_parent is set or not. If we have a parent + # they own the registered cdata. + # Functions that may create a DNode without a parent: + # * Context.create_data_path() if parent is None + # * Context.parse_op{_mem}() if parent is None + # * Context.parse_data{_mem,_file}() if parent is None + # * DNode.diff() + # * DNode.duplicate() if parent is None + # * dict_to_dnode() if parent is None + # * DNode.merge_data_dict() + # * DNode.iter_tree() + # * DNode.leafref_nodes() + # Functions that might unset the refcnt_parent: + # * DNode.merge{_module}() + if not self.refcnt_parent: + self.free() def meta(self): ret = {} @@ -452,17 +479,17 @@ def schema(self) -> SNode: def parent(self) -> Optional["DNode"]: if not self.cdata.parent: return None - return self.new(self.context, self.cdata.parent) + return self.new(self.context, self.cdata.parent, self) def next(self) -> Optional["DNode"]: if not self.cdata.next: return None - return self.new(self.context, self.cdata.next) + return self.new(self.context, self.cdata.next, self) def prev(self) -> Optional["DNode"]: if not self.cdata.prev: return None - return self.new(self.context, self.cdata.prev) + return self.new(self.context, self.cdata.prev, self) def root(self) -> "DNode": node = self @@ -474,7 +501,7 @@ def first_sibling(self) -> "DNode": n = lib.lyd_first_sibling(self.cdata) if n == self.cdata: return self - return self.new(self.context, n) + return self.new(self.context, n, self) def siblings(self, include_self: bool = True) -> Iterator["DNode"]: n = lib.lyd_first_sibling(self.cdata) @@ -483,14 +510,14 @@ def siblings(self, include_self: bool = True) -> Iterator["DNode"]: if include_self: yield self else: - yield self.new(self.context, n) + yield self.new(self.context, n, self) n = n.next def find_path(self, path: str, output: bool = False): node = ffi.new("struct lyd_node **") ret = lib.lyd_find_path(self.cdata, str2c(path), output, node) if ret == lib.LY_SUCCESS: - return DNode.new(self.context, node[0]) + return DNode.new(self.context, node[0], self) return None def find_one(self, xpath: str) -> Optional["DNode"]: @@ -508,7 +535,7 @@ def find_all(self, xpath: str) -> Iterator["DNode"]: node_set = node_set[0] try: for i in range(node_set.count): - n = DNode.new(self.context, node_set.dnodes[i]) + n = DNode.new(self.context, node_set.dnodes[i], self) yield n finally: lib.ly_set_free(node_set, ffi.NULL) @@ -593,7 +620,8 @@ def diff( if node_p[0] == ffi.NULL: return None - return self.new(self.context, node_p[0]) + # New allocation, refcnt_parent is None + return self.new(self.context, node_p[0], None) def diff_apply(self, diff_node: "DNode") -> None: node_p = ffi.new("struct lyd_node **") @@ -630,7 +658,7 @@ def duplicate( else: lib.lyd_dup_single(self.cdata, parent, flags, node) - return DNode.new(self.context, node[0]) + return DNode.new(self.context, node[0], parent) def merge_module( self, @@ -648,6 +676,10 @@ def merge_module( if ret != lib.LY_SUCCESS: raise self.context.error("merge failed") + if destruct: + source.cdata = None + source.refcnt_parent = None + def merge( self, source: "DNode", @@ -669,10 +701,14 @@ def merge( self.cdata = node_p[0] + if destruct: + source.cdata = None + source.refcnt_parent = None + def iter_tree(self) -> Iterator["DNode"]: n = next_n = self.cdata while n != ffi.NULL: - yield self.new(self.context, n) + yield self.new(self.context, n, self) next_n = lib.lyd_child(n) if next_n == ffi.NULL: @@ -996,13 +1032,14 @@ def free_internal(self, with_siblings: bool = True) -> None: lib.lyd_free_tree(self.cdata) def free(self, with_siblings: bool = True) -> None: - try: - if self.free_func: - self.free_func(self) # pylint: disable=not-callable - else: - self.free_internal(with_siblings) - finally: - self.cdata = ffi.NULL + if self.cdata is not None: + try: + if self.free_func: + self.free_func(self) # pylint: disable=not-callable + else: + self.free_internal(with_siblings) + finally: + self.cdata = None def leafref_link_node_tree(self) -> None: """ @@ -1024,7 +1061,7 @@ def leafref_nodes(self) -> Iterator["DNode"]: if lib.lyd_leafref_get_links(term_node, out) != lib.LY_SUCCESS: return for n in ly_array_iter(out[0].leafref_nodes): - yield DNode.new(self.context, n) + yield DNode.new(self.context, n, self) def __repr__(self): cls = self.__class__ @@ -1045,7 +1082,7 @@ def _decorator(nodeclass): return _decorator @classmethod - def new(cls, context: "libyang.Context", cdata) -> "DNode": + def new(cls, context: "libyang.Context", cdata, refcnt_parent) -> "DNode": cdata = ffi.cast("struct lyd_node *", cdata) if not cdata.schema: schemas = list(context.find_path(cls._get_path(cdata))) @@ -1056,7 +1093,7 @@ def new(cls, context: "libyang.Context", cdata) -> "DNode": nodecls = cls.NODETYPE_CLASS.get(cdata.schema.nodetype, None) if nodecls is None: raise TypeError("node type %s not implemented" % cdata.schema.nodetype) - return nodecls(context, cdata) + return nodecls(context, cdata, refcnt_parent) @staticmethod def _get_path(cdata) -> str: @@ -1089,7 +1126,7 @@ def children(self, no_keys=False) -> Iterator[DNode]: while child: if child.schema != ffi.NULL: - yield DNode.new(self.context, child) + yield DNode.new(self.context, child, self) child = child.next def __iter__(self): @@ -1236,7 +1273,7 @@ def dict_to_dnode( created = [] - def _create_leaf(_parent, module, name, value, in_rpc_output=False): + def _create_leaf(_parent_cdata, module, name, value, in_rpc_output=False): if value is not None: if isinstance(value, bool): value = str(value).lower() @@ -1246,7 +1283,7 @@ def _create_leaf(_parent, module, name, value, in_rpc_output=False): n = ffi.new("struct lyd_node **") flags = newval_flags(rpc_output=in_rpc_output, store_only=store_only) ret = lib.lyd_new_term( - _parent, + _parent_cdata, module.cdata, str2c(name), str2c(value), @@ -1255,8 +1292,8 @@ def _create_leaf(_parent, module, name, value, in_rpc_output=False): ) if ret != lib.LY_SUCCESS: - if _parent: - parent_path = repr(DNode.new(module.context, _parent).path()) + if _parent_cdata: + parent_path = repr(DNode.new(module.context, _parent_cdata, parent).path()) else: parent_path = "module %r" % module.name() raise module.context.error( @@ -1264,12 +1301,12 @@ def _create_leaf(_parent, module, name, value, in_rpc_output=False): ) created.append(n[0]) - def _create_container(_parent, module, name, in_rpc_output=False): + def _create_container(_parent_cdata, module, name, in_rpc_output=False): n = ffi.new("struct lyd_node **") - ret = lib.lyd_new_inner(_parent, module.cdata, str2c(name), in_rpc_output, n) + ret = lib.lyd_new_inner(_parent_cdata, module.cdata, str2c(name), in_rpc_output, n) if ret != lib.LY_SUCCESS: - if _parent: - parent_path = repr(DNode.new(module.context, _parent).path()) + if _parent_cdata: + parent_path = repr(DNode.new(module.context, _parent_cdata, parent).path()) else: parent_path = "module %r" % module.name() raise module.context.error( @@ -1280,11 +1317,11 @@ def _create_container(_parent, module, name, in_rpc_output=False): created.append(n[0]) return n[0] - def _create_list(_parent, module, name, key_values, in_rpc_output=False): + def _create_list(_parent_cdata, module, name, key_values, in_rpc_output=False): n = ffi.new("struct lyd_node **") flags = newval_flags(rpc_output=in_rpc_output, store_only=store_only) ret = lib.lyd_new_list( - _parent, + _parent_cdata, module.cdata, str2c(name), flags, @@ -1292,8 +1329,8 @@ def _create_list(_parent, module, name, key_values, in_rpc_output=False): *[str2c(str(i)) for i in key_values], ) if ret != lib.LY_SUCCESS: - if _parent: - parent_path = repr(DNode.new(module.context, _parent).path()) + if _parent_cdata: + parent_path = repr(DNode.new(module.context, _parent_cdata, parent).path()) else: parent_path = "module %r" % module.name() raise module.context.error( @@ -1349,7 +1386,7 @@ def _dic_keys(_dic, _schema): return keys return _dic.keys() - def _to_dnode(_dic, _schema, _parent=ffi.NULL, in_rpc_output=False): + def _to_dnode(_dic, _schema, _parent_cdata=ffi.NULL, in_rpc_output=False): for key in _dic_keys(_dic, _schema): if ":" in key: prefix, name = key.split(":") @@ -1372,7 +1409,7 @@ def _to_dnode(_dic, _schema, _parent=ffi.NULL, in_rpc_output=False): value = _dic[key] if isinstance(s, SLeaf): - _create_leaf(_parent, module, name, value, in_rpc_output) + _create_leaf(_parent_cdata, module, name, value, in_rpc_output) elif isinstance(s, SLeafList): if not isinstance(value, (list, tuple)): @@ -1381,14 +1418,14 @@ def _to_dnode(_dic, _schema, _parent=ffi.NULL, in_rpc_output=False): % (s.schema_path(), value) ) for v in value: - _create_leaf(_parent, module, name, v, in_rpc_output) + _create_leaf(_parent_cdata, module, name, v, in_rpc_output) elif isinstance(s, SRpc): - n = _create_container(_parent, module, name, in_rpc_output) + n = _create_container(_parent_cdata, module, name, in_rpc_output) _to_dnode(value, s, n, rpcreply) elif isinstance(s, SContainer): - n = _create_container(_parent, module, name, in_rpc_output) + n = _create_container(_parent_cdata, module, name, in_rpc_output) _to_dnode(value, s, n, in_rpc_output) elif isinstance(s, SList): @@ -1413,31 +1450,31 @@ def _to_dnode(_dic, _schema, _parent=ffi.NULL, in_rpc_output=False): except KeyError as e: raise ValueError("Missing key %s in the list" % (k)) from e - n = _create_list(_parent, module, name, key_values, in_rpc_output) + n = _create_list(_parent_cdata, module, name, key_values, in_rpc_output) _to_dnode(val, s, n, in_rpc_output) elif isinstance(s, SNotif): - n = _create_container(_parent, module, name, in_rpc_output) + n = _create_container(_parent_cdata, module, name, in_rpc_output) _to_dnode(value, s, n, in_rpc_output) result = None try: if parent is not None: - _parent = parent.cdata + _parent_cdata = parent.cdata _schema_parent = parent.schema() else: - _parent = ffi.NULL + _parent_cdata = ffi.NULL _schema_parent = module _to_dnode( dic, _schema_parent, - _parent, + _parent_cdata, in_rpc_output=rpcreply and isinstance(parent, DRpc), ) if created: - result = DNode.new(module.context, created[0]) + result = DNode.new(module.context, created[0], parent) if validate: result.root().validate( no_state=no_state, From 09c4b42473e399816d00345fa25d1db42d69e842 Mon Sep 17 00:00:00 2001 From: Brad House Date: Thu, 6 Feb 2025 14:36:19 -0500 Subject: [PATCH 2/2] clarification --- libyang/data.py | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/libyang/data.py b/libyang/data.py index 313d415..2344bcb 100644 --- a/libyang/data.py +++ b/libyang/data.py @@ -1273,7 +1273,7 @@ def dict_to_dnode( created = [] - def _create_leaf(_parent_cdata, module, name, value, in_rpc_output=False): + def _create_leaf(refcnt_parent, _parent_cdata, module, name, value, in_rpc_output=False): if value is not None: if isinstance(value, bool): value = str(value).lower() @@ -1293,7 +1293,7 @@ def _create_leaf(_parent_cdata, module, name, value, in_rpc_output=False): if ret != lib.LY_SUCCESS: if _parent_cdata: - parent_path = repr(DNode.new(module.context, _parent_cdata, parent).path()) + parent_path = repr(DNode.new(module.context, _parent_cdata, refcnt_parent).path()) else: parent_path = "module %r" % module.name() raise module.context.error( @@ -1301,12 +1301,12 @@ def _create_leaf(_parent_cdata, module, name, value, in_rpc_output=False): ) created.append(n[0]) - def _create_container(_parent_cdata, module, name, in_rpc_output=False): + def _create_container(refcnt_parent, _parent_cdata, module, name, in_rpc_output=False): n = ffi.new("struct lyd_node **") ret = lib.lyd_new_inner(_parent_cdata, module.cdata, str2c(name), in_rpc_output, n) if ret != lib.LY_SUCCESS: if _parent_cdata: - parent_path = repr(DNode.new(module.context, _parent_cdata, parent).path()) + parent_path = repr(DNode.new(module.context, _parent_cdata, refcnt_parent).path()) else: parent_path = "module %r" % module.name() raise module.context.error( @@ -1317,7 +1317,7 @@ def _create_container(_parent_cdata, module, name, in_rpc_output=False): created.append(n[0]) return n[0] - def _create_list(_parent_cdata, module, name, key_values, in_rpc_output=False): + def _create_list(refcnt_parent, _parent_cdata, module, name, key_values, in_rpc_output=False): n = ffi.new("struct lyd_node **") flags = newval_flags(rpc_output=in_rpc_output, store_only=store_only) ret = lib.lyd_new_list( @@ -1330,7 +1330,7 @@ def _create_list(_parent_cdata, module, name, key_values, in_rpc_output=False): ) if ret != lib.LY_SUCCESS: if _parent_cdata: - parent_path = repr(DNode.new(module.context, _parent_cdata, parent).path()) + parent_path = repr(DNode.new(module.context, _parent_cdata, refcnt_parent).path()) else: parent_path = "module %r" % module.name() raise module.context.error( @@ -1386,7 +1386,7 @@ def _dic_keys(_dic, _schema): return keys return _dic.keys() - def _to_dnode(_dic, _schema, _parent_cdata=ffi.NULL, in_rpc_output=False): + def _to_dnode(refcnt_parent, _dic, _schema, _parent_cdata=ffi.NULL, in_rpc_output=False): for key in _dic_keys(_dic, _schema): if ":" in key: prefix, name = key.split(":") @@ -1409,7 +1409,7 @@ def _to_dnode(_dic, _schema, _parent_cdata=ffi.NULL, in_rpc_output=False): value = _dic[key] if isinstance(s, SLeaf): - _create_leaf(_parent_cdata, module, name, value, in_rpc_output) + _create_leaf(refcnt_parent, _parent_cdata, module, name, value, in_rpc_output) elif isinstance(s, SLeafList): if not isinstance(value, (list, tuple)): @@ -1418,14 +1418,14 @@ def _to_dnode(_dic, _schema, _parent_cdata=ffi.NULL, in_rpc_output=False): % (s.schema_path(), value) ) for v in value: - _create_leaf(_parent_cdata, module, name, v, in_rpc_output) + _create_leaf(refcnt_parent, _parent_cdata, module, name, v, in_rpc_output) elif isinstance(s, SRpc): - n = _create_container(_parent_cdata, module, name, in_rpc_output) + n = _create_container(refcnt_parent, _parent_cdata, module, name, in_rpc_output) _to_dnode(value, s, n, rpcreply) elif isinstance(s, SContainer): - n = _create_container(_parent_cdata, module, name, in_rpc_output) + n = _create_container(refcnt_parent, _parent_cdata, module, name, in_rpc_output) _to_dnode(value, s, n, in_rpc_output) elif isinstance(s, SList): @@ -1450,11 +1450,11 @@ def _to_dnode(_dic, _schema, _parent_cdata=ffi.NULL, in_rpc_output=False): except KeyError as e: raise ValueError("Missing key %s in the list" % (k)) from e - n = _create_list(_parent_cdata, module, name, key_values, in_rpc_output) + n = _create_list(refcnt_parent, _parent_cdata, module, name, key_values, in_rpc_output) _to_dnode(val, s, n, in_rpc_output) elif isinstance(s, SNotif): - n = _create_container(_parent_cdata, module, name, in_rpc_output) + n = _create_container(refcnt_parent, _parent_cdata, module, name, in_rpc_output) _to_dnode(value, s, n, in_rpc_output) result = None @@ -1468,6 +1468,7 @@ def _to_dnode(_dic, _schema, _parent_cdata=ffi.NULL, in_rpc_output=False): _schema_parent = module _to_dnode( + parent, dic, _schema_parent, _parent_cdata,