# HG changeset patch # User Petter T. # Date 1693465520 -7200 # Thu Aug 31 09:05:20 2023 +0200 # Node ID 8c58586da890090d82b70bbd69eea898301b1aaa # Parent 774c54278e97e1d3957598a5b6c5aa5c2531007f Nested functions handles make stack frames leak (bug #64620) If the handle is saved on parent frames. * libinterp/corefcn/call-stack.cc: Clear static and parent link on frame pop * libinterp/corefcn/stack-frame.h: Mark closure for all linked accesses. Fn to clear parent and static link. * libinterp/octave-value/ov-fcn-handle.cc: Search access links for context frame match * test/bug-64620/: Tests diff -r 774c54278e97 -r 8c58586da890 libinterp/corefcn/call-stack.cc --- a/libinterp/corefcn/call-stack.cc Wed Aug 30 19:12:53 2023 +0200 +++ b/libinterp/corefcn/call-stack.cc Thu Aug 31 09:05:20 2023 +0200 @@ -789,6 +789,8 @@ if (elt->is_closure_context ()) elt->break_closure_cycles (elt); + elt->clear_parent_static_link (); + m_cs.pop_back (); } } @@ -805,6 +807,8 @@ if (elt->is_closure_context ()) elt->break_closure_cycles (elt); + elt->clear_parent_static_link (); + return elt; } diff -r 774c54278e97 -r 8c58586da890 libinterp/corefcn/stack-frame.h --- a/libinterp/corefcn/stack-frame.h Wed Aug 30 19:12:53 2023 +0200 +++ b/libinterp/corefcn/stack-frame.h Thu Aug 31 09:05:20 2023 +0200 @@ -319,6 +319,12 @@ mark_global (sym); } + void clear_parent_static_link () + { + m_parent_link = nullptr; + m_static_link = nullptr; + } + std::size_t parent_frame_index () const { return m_parent_link->index (); } @@ -582,7 +588,21 @@ virtual void break_closure_cycles (const std::shared_ptr&) { } - void mark_closure_context () { m_is_closure_context = true; } + void mark_closure_context () + { + m_is_closure_context = true; + + // Mark any access linked frames as closure contexts too, + // so that they'll make any function handle on its stack frame + // weak when the frame itself is being popped. + auto nxt = access_link (); + while (nxt) + { + nxt->m_is_closure_context = true; + nxt = nxt->access_link (); + } + } + bool is_closure_context () const { return m_is_closure_context; } // The VM needs to tell the bytecode stackframe that it unwinds so diff -r 774c54278e97 -r 8c58586da890 libinterp/octave-value/ov-fcn-handle.cc --- a/libinterp/octave-value/ov-fcn-handle.cc Wed Aug 30 19:12:53 2023 +0200 +++ b/libinterp/octave-value/ov-fcn-handle.cc Thu Aug 31 09:05:20 2023 +0200 @@ -416,7 +416,19 @@ bool is_nested (const std::shared_ptr& frame) const { - return frame == m_stack_context; + if (frame == m_stack_context) + return true; + + // We need to check each of the access links of the context frame, for 'frame' too + auto nxt = m_stack_context->access_link (); + while (nxt) + { + if (nxt == frame) + return true; + nxt = nxt->access_link (); + } + + return false; } nested_fcn_handle * clone () const diff -r 774c54278e97 -r 8c58586da890 test/bug-64620/bug-64620.tst --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/test/bug-64620/bug-64620.tst Thu Aug 31 09:05:20 2023 +0200 @@ -0,0 +1,14 @@ +% Nested functions frame stored in handles would prevent parent frame +% from being released causing memory leak. + +%!test <*64620> +%! global cdef_alive_objects_cntr % Counts amount of alive cdef_counts_self handle objects +%! cdef_alive_objects_cntr = 0; +%! bug_64620_1; +%! assert (cdef_alive_objects_cntr == 0) +%! +%! clear -global cdef_alive_objects_cntr + +%!test <*64620> +%! bug_64620_2; % Asserts inside itself +%! clear -global cdef_alive_objects_cntr diff -r 774c54278e97 -r 8c58586da890 test/bug-64620/bug_64620_1.m --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/test/bug-64620/bug_64620_1.m Thu Aug 31 09:05:20 2023 +0200 @@ -0,0 +1,14 @@ +function bug_64620_1 + h = subby; +end + +function h = subby % The h return is needed for the bug to show + function nested_fn + % This framed stored shared pointer to subby's frame when put in handle + end + + h = @nested_fn; + + obj = cdef_counts_self; + % obj destructor was not triggered +end diff -r 774c54278e97 -r 8c58586da890 test/bug-64620/bug_64620_2.m --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/test/bug-64620/bug_64620_2.m Thu Aug 31 09:05:20 2023 +0200 @@ -0,0 +1,28 @@ +function bug_64620_2() + % Two levels of nested nesting + global cdef_alive_objects_cntr = 0; + cdef_alive_objects_cntr = 0; + + h = sub_returns_nested_fn2; + h (); + h = 3; % Clear handle, triggers dtors + + assert (cdef_alive_objects_cntr == 0); +end + +function h1 = sub_returns_nested_fn2 + c2 = cdef_counts_self; + + function h2 = nested_fn1 + c3 = cdef_counts_self; + + function nested_fn2 + c4 = cdef_counts_self; + end + + h2 = @nested_fn2; + end + + h1 = nested_fn1 (); +end + diff -r 774c54278e97 -r 8c58586da890 test/bug-64620/cdef_counts_self.m --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/test/bug-64620/cdef_counts_self.m Thu Aug 31 09:05:20 2023 +0200 @@ -0,0 +1,12 @@ +classdef cdef_counts_self < handle + methods + function obj = cdef_counts_self () + global cdef_alive_objects_cntr = 0; + cdef_alive_objects_cntr++; + end + function delete (self) + global cdef_alive_objects_cntr = 0; + cdef_alive_objects_cntr--; + end + end +end \ No newline at end of file diff -r 774c54278e97 -r 8c58586da890 test/bug-64620/module.mk --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/test/bug-64620/module.mk Thu Aug 31 09:05:20 2023 +0200 @@ -0,0 +1,7 @@ +bug_64620_TEST_FILES = \ + %reldir%/bug-64620.tst \ + %reldir%/cdef_counts_self.m \ + %reldir%/bug_64620_1.m + %reldir%/bug_64620_2.m + +TEST_FILES += $(bug_64620_TEST_FILES)