Skip to content

Commit 10d00b0

Browse files
committed
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 <[email protected]>
1 parent 8534053 commit 10d00b0

File tree

2 files changed

+94
-49
lines changed

2 files changed

+94
-49
lines changed

libyang/context.py

+12-4
Original file line numberDiff line numberDiff line change
@@ -259,6 +259,10 @@ def __init__(
259259
raise self.error("cannot create context")
260260
self.external_module_loader = ContextExternalModuleLoader(self.cdata)
261261

262+
263+
def __del__(self):
264+
self.destroy()
265+
262266
def compile_schema(self):
263267
ret = lib.ly_ctx_compile(self.cdata)
264268
if ret != lib.LY_SUCCESS:
@@ -269,7 +273,11 @@ def get_yanglib_data(self, content_id_format=""):
269273
ret = lib.ly_ctx_get_yanglib_data(self.cdata, dnode, str2c(content_id_format))
270274
if ret != lib.LY_SUCCESS:
271275
raise self.error("cannot get yanglib data")
272-
return DNode.new(self, dnode[0])
276+
# The refcnt_parent parameter here is Context rather than all other cases
277+
# where it is DNode. Its not entirely clear how else to do it, pretty
278+
# sure this returns internal memory that can't be passed to
279+
# lyd_free_all() or lyd_free_tree() though docs on this aren't great.
280+
return DNode.new(self, dnode[0], self)
273281

274282
def destroy(self):
275283
if self.cdata is not None:
@@ -466,7 +474,7 @@ def create_data_path(
466474
if not dnode:
467475
raise self.error("cannot find created path")
468476

469-
return DNode.new(self, dnode)
477+
return DNode.new(self, dnode, parent)
470478

471479
def parse_op(
472480
self,
@@ -495,7 +503,7 @@ def parse_op(
495503
if ret != lib.LY_SUCCESS:
496504
raise self.error("failed to parse input data")
497505

498-
return DNode.new(self, op[0])
506+
return DNode.new(self, op[0], parent)
499507

500508
def parse_op_mem(
501509
self,
@@ -578,7 +586,7 @@ def parse_data(
578586
dnode = dnode[0]
579587
if dnode == ffi.NULL:
580588
return None
581-
return DNode.new(self, dnode)
589+
return DNode.new(self, dnode, parent)
582590

583591
def parse_data_mem(
584592
self,

0 commit comments

Comments
 (0)