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.
This commit is contained in:
Mahmoud Al-Qudsi 2022-10-25 12:30:30 -05:00
parent 14ecb63e40
commit 175caab583
4 changed files with 17 additions and 2 deletions

View File

@ -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";

View File

@ -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.")

View File

@ -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);

View File

@ -459,6 +459,9 @@ class parser_t : public std::enable_shared_from_this<parser_t> {
/// \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();
};