Mercurial > octave
changeset 32691:2d3074d82ab3 bytecode-interpreter
Fix bytecode interpreter stack overflow with varargout (bug #65029)
Don't push varargout elements to the stack when returning from
bytecode functions to non-bytecode functions. Don't push more elements
to the stack then needed when returning from bytecode to bytecode.
* pt-bytecode-vm.cc: Avoid overflow
* bytecode_varargout.m: Update tests
author | Petter T. |
---|---|
date | Thu, 14 Dec 2023 17:16:13 +0100 |
parents | ac71ac0d6522 |
children | b1f779dc05d2 |
files | libinterp/parse-tree/pt-bytecode-vm.cc test/compile/bytecode_varargout.m |
diffstat | 2 files changed, 134 insertions(+), 35 deletions(-) [+] |
line wrap: on
line diff
--- a/libinterp/parse-tree/pt-bytecode-vm.cc Fri Jan 05 12:34:39 2024 -0500 +++ b/libinterp/parse-tree/pt-bytecode-vm.cc Thu Dec 14 17:16:13 2023 +0100 @@ -1330,13 +1330,10 @@ } sp -= n_dtor; - if (is_varargout) - { - n_returns_callee--; // Assume empty varargout - - // Expand the cell array and push the elements to the stack - octave_value ov_vararg = std::move (sp[-1].ov); - STACK_DESTROY (1); + if (OCTAVE_UNLIKELY (is_varargout)) + { + // Check that varargout is a cell or undefined + octave_value &ov_vararg = sp[-1].ov; bool vararg_defined = ov_vararg.is_defined (); if (vararg_defined && !ov_vararg.iscell ()) @@ -1345,23 +1342,6 @@ (*sp++).i = static_cast<int> (error_type::EXECUTION_EXC); goto unwind; } - if (vararg_defined) - { - // Push the cell array elements to the stack - Cell cell_vararg = ov_vararg.cell_value (); - for (int i = 0; i < cell_vararg.numel (); i++) - { - octave_value &arg = cell_vararg(i); - PUSH_OV (std::move (arg)); - n_returns_callee++; - } - } - // Only push an empty varargout if we are returning to another bytecode function - else if (bsp != rsp) - { - n_returns_callee++; - PUSH_OV (); - } } if (OCTAVE_UNLIKELY (m_profiler_enabled)) @@ -1388,17 +1368,53 @@ int j; // nargout 0 should still give one return value, if there is one int n_root_wanted = std::max (root_nargout, 1); - for (j = 1; j < n_returns_callee && j < (n_root_wanted + 1); j++) + + if (is_varargout) { - if (OCTAVE_UNLIKELY (bsp[j].ov.is_ref ())) - ret.append (bsp[j].ov.ref_rep ()->deref ()); - else - ret.append (std::move (bsp[j].ov)); - bsp[j].ov.~octave_value (); + CHECK_PANIC(n_returns_callee >= 2); + + octave_value ov_vararg = sp[-1].ov; // varargout on the top of the stack + + bool vararg_defined = ov_vararg.is_defined (); + + for (j = 1; j < n_returns_callee - 1 && j < n_root_wanted + 1; j++) + { + if (OCTAVE_UNLIKELY (bsp[j].ov.is_ref ())) + ret.append (bsp[j].ov.ref_rep ()->deref ()); + else + ret.append (std::move (bsp[j].ov)); + bsp[j].ov.~octave_value (); + } + // Append varargout to ret + if (vararg_defined && j < n_root_wanted + 1) + { + // Push the cell array elements to the stack + Cell cell_vararg = ov_vararg.cell_value (); + for (int i = 0; i < cell_vararg.numel () && j + i < n_root_wanted + 1; i++) + { + octave_value &arg = cell_vararg(i); + ret.append (std::move (arg)); + } + } + + // Destroy varargout and rest of return values, if any + for (; j < n_returns_callee; j++) + bsp[j].ov.~octave_value (); } - // Destroy rest of return values, if any - for (; j < n_returns_callee; j++) - bsp[j].ov.~octave_value (); + else + { + for (j = 1; j < n_returns_callee && j < (n_root_wanted + 1); j++) + { + if (OCTAVE_UNLIKELY (bsp[j].ov.is_ref ())) + ret.append (bsp[j].ov.ref_rep ()->deref ()); + else + ret.append (std::move (bsp[j].ov)); + bsp[j].ov.~octave_value (); + } + // Destroy rest of return values, if any + for (; j < n_returns_callee; j++) + bsp[j].ov.~octave_value (); + } //Note: Stack frame object popped by caller CHECK_STACK (0); @@ -1454,8 +1470,53 @@ // Renaming variables to keep my sanity. int n_args_caller_expects = caller_nval_back; int n_args_callee_has = n_returns_callee - 1; // Exclude %nargout + int n_args_actually_moved = 0; + + if (OCTAVE_UNLIKELY (is_varargout)) + { + // Expand the cell array and push the elements to the end of the callee stack + octave_value ov_vararg = std::move (caller_stack_end[n_args_callee_has].ov); + + n_args_callee_has--; // Assume empty varargout + + bool vararg_defined = ov_vararg.is_defined (); + + if (vararg_defined) + { + // Push the cell array elements to the stack + Cell cell_vararg = ov_vararg.cell_value (); + octave_idx_type n = cell_vararg.numel (); + + octave_idx_type n_to_push; + // Atleast one (for 'ans') and deduct the amount of args already on the stack + n_to_push = std::max (1, n_args_caller_expects) - n_args_callee_has; + // Can't be negative + n_to_push = n_to_push < 0 ? 0 : n_to_push; + // Can't push more than amount of elements in cell + n_to_push = std::min (n , n_to_push); + + CHECK_STACK_N (n_to_push); + + int i = 0; + for (; i < n_to_push; i++) + { + octave_value &arg = cell_vararg(i); + // Construct octave_value with placement new, in the end of the callee stack + new (&caller_stack_end[n_args_callee_has + 1 + i].ov) octave_value (std::move (arg)); // +1 for %nargout + } + + n_args_callee_has += i; + } + else if (n_args_caller_expects) + { + // Push an empty varargout + new (&caller_stack_end[n_args_callee_has + 1].ov) octave_value {}; + + n_args_callee_has++; + } + } + int n_args_to_move = std::min (n_args_caller_expects, n_args_callee_has); - int n_args_actually_moved = 0; // If no return values is requested but there exists return values, // we need to push one to be able to write it to ans.
--- a/test/compile/bytecode_varargout.m Fri Jan 05 12:34:39 2024 -0500 +++ b/test/compile/bytecode_varargout.m Thu Dec 14 17:16:13 2023 +0100 @@ -51,9 +51,39 @@ __printf_assert__ ("%d ", a); [~, ~, ~] = sub_return_isargout (3); + + % Check that 40000 return values wont cause a stack overflow + ans = 0; + suby1 (40000); % returns [varargout] => [1 2 3 ... n] + assert (ans == 1) + + ans = 0; + suby2 (40000); % returns [a b varargout] => [1 2 3 ... n] + assert (ans == 1) + + % Check dropping return values + [a b c] = suby1 (10); + assert (all ([a b c] == [1 2 3])) + [a b c] = suby2 (10); + assert (all ([a b c] == [1 2 3])) + + % Check too few return values + threw = false; + try + [a b c] = suby1 (2); + catch + threw = true; + end + assert (threw); + + % Bug #65029, stack overflowed when deal returned + nlay = 20000; + a = struct ("aa", {1:nlay}); + tmp = {1:nlay}; + [a(1:nlay).aa] = deal (tmp{:}); end -function [a b c ] = sub_return_isargout (n) +function [a b c] = sub_return_isargout (n) b = 0; c = 0; a = isargout (n); end @@ -63,3 +93,11 @@ varargout{i} = i; end end + +function [a b varargout] = suby2(n) + a = 1; b = 2; + for i = 1:n-2 + varargout{i} = i + 2; + end +end +