From 175caab58300af9158eb19bd6f9d8427c483d46e Mon Sep 17 00:00:00 2001 From: Mahmoud Al-Qudsi Date: Tue, 25 Oct 2022 12:30:30 -0500 Subject: [PATCH] Prevent stack overflow from eval/substitution recursion It seems to have originally been thought that the only possible way a stack overflow could happen is via function calls, but there are other possibilities. Issue #9302 reports how `eval` can be abused to recursively execute a string substitution ad infinitum, triggering a stack overflow in fish. This patch extends the stack overflow check to also check the current `eval_level` against a new constant `FISH_MAX_EVAL_DEPTH`, currently set to a conservative but hopefully still fair limit of 500. For future reference, with the default stack size for the main/foreground thread of 8 MiB, we actually have room for a stack depth around 2800, but that's only with extremely minimal state stored in each stack frame. I'm not entirely sure why we don't check `eval_depth` regardless of block type; it can't be for performance reasons since it's just a simple integer comparison - and a ridiculously easily one for the branch predictor handle, at that - but maybe it's to try and support non-recursive nested execution blocks of greater than `FISH_MAX_STACK_DEPTH`? But even without recursion, the stack can still overflow so may be we should just bump the limit up some (to 500 like the new `FISH_MAX_EVAL_DEPTH`?) and check it all the time? Closes #9302. --- src/expand.cpp | 6 ++++++ src/parse_constants.h | 3 +++ src/parse_execution.cpp | 7 +++++-- src/parser.h | 3 +++ 4 files changed, 17 insertions(+), 2 deletions(-) diff --git a/src/expand.cpp b/src/expand.cpp index 17af38038..d6f091131 100644 --- a/src/expand.cpp +++ b/src/expand.cpp @@ -31,6 +31,7 @@ #include "operation_context.h" #include "parse_constants.h" #include "parse_util.h" +#include "parser.h" #include "path.h" #include "util.h" #include "wcstringutil.h" @@ -641,8 +642,13 @@ static expand_result_t expand_cmdsubst(wcstring input, const operation_context_t case STATUS_READ_TOO_MUCH: err = L"Too much data emitted by command substitution so it was discarded"; break; + // TODO: STATUS_CMD_ERROR is overused and too generic. We shouldn't have to test things + // to figure out what error to show after we've already been given an error code. case STATUS_CMD_ERROR: err = L"Too many active file descriptors"; + if (ctx.parser->is_eval_depth_exceeded()) { + err = L"Unable to evaluate string substitution"; + } break; case STATUS_CMD_UNKNOWN: err = L"Unknown command"; diff --git a/src/parse_constants.h b/src/parse_constants.h index bd89e6dc5..0276b3e2f 100644 --- a/src/parse_constants.h +++ b/src/parse_constants.h @@ -211,6 +211,9 @@ enum class pipeline_position_t : uint8_t { /// Maximum number of function calls. #define FISH_MAX_STACK_DEPTH 128 +/// Maximum number of nested string substitutions (in lieu of evals) +#define FISH_MAX_EVAL_DEPTH 500 + /// Error message on a function that calls itself immediately. #define INFINITE_FUNC_RECURSION_ERR_MSG \ _(L"The function '%ls' calls itself immediately, which would result in an infinite loop.") diff --git a/src/parse_execution.cpp b/src/parse_execution.cpp index dcf839dbf..5c197a4b4 100644 --- a/src/parse_execution.cpp +++ b/src/parse_execution.cpp @@ -1531,8 +1531,11 @@ end_execution_reason_t parse_execution_context_t::eval_node(const ast::job_list_ INFINITE_FUNC_RECURSION_ERR_MSG, func_name.c_str()); } - // Check for stack overflow. The TOP check ensures we only do this for function calls. - if (associated_block->type() == block_type_t::top && parser->function_stack_is_overflowing()) { + // Check for stack overflow in case of funtion calls (regular stack overflow) or string + // substitution blocks, which can be recursively called with eval (issue #9302). + if ((associated_block->type() == block_type_t::top && + parser->function_stack_is_overflowing()) || + (associated_block->type() == block_type_t::subst && parser->is_eval_depth_exceeded())) { return this->report_error(STATUS_CMD_ERROR, job_list, CALL_STACK_LIMIT_EXCEEDED_ERR_MSG); } return this->run_job_list(job_list, associated_block); diff --git a/src/parser.h b/src/parser.h index 54163e10e..0a3acacd2 100644 --- a/src/parser.h +++ b/src/parser.h @@ -459,6 +459,9 @@ class parser_t : public std::enable_shared_from_this { /// \return the operation context for this parser. operation_context_t context(); + /// Checks if the max eval depth has been exceeded + bool is_eval_depth_exceeded() const { return eval_level >= FISH_MAX_EVAL_DEPTH; } + ~parser_t(); };