From 2555ecf7576d748166c0b4b54e5728c90eea6e5e Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sun, 10 Nov 2019 12:36:46 -0800 Subject: [PATCH] Remove the forbidden function stack Detect forbidden functions directly from the associated block_t. Also unify where we do stack overflow detection. --- src/exec.cpp | 2 -- src/parse_execution.cpp | 33 +++++++++++++------------------- src/parser.cpp | 19 ++++++++++++++---- src/parser.h | 12 +++--------- tests/checks/stack-overflow.fish | 2 ++ 5 files changed, 33 insertions(+), 35 deletions(-) diff --git a/src/exec.cpp b/src/exec.cpp index 21a190556..27f2101c1 100644 --- a/src/exec.cpp +++ b/src/exec.cpp @@ -814,11 +814,9 @@ static bool exec_block_or_func_process(parser_t &parser, std::shared_ptr block_t *fb = parser.push_block(block_t::function_block(func_name, argv, props->shadow_scope)); function_prepare_environment(parser.vars(), func_name, std::move(argv), inherit_vars); - parser.forbid_function(func_name); internal_exec_helper(parser, props->parsed_source, props->body_node, io_chain, j); - parser.allow_function(); parser.pop_block(fb); // If we returned due to a return statement, then stop returning now. diff --git a/src/parse_execution.cpp b/src/parse_execution.cpp index a742f57f5..0bdd90133 100644 --- a/src/parse_execution.cpp +++ b/src/parse_execution.cpp @@ -108,11 +108,8 @@ tnode_t parse_execution_context_t::infinite_recursive_statem return {}; } - // Check to see which function call is forbidden. - if (parser->forbidden_function.empty()) { - return {}; - } - const wcstring &forbidden_function_name = parser->forbidden_function.back(); + // Get the function name of the immediate block. + const wcstring &forbidden_function_name = parent->function_name; // Get the first job in the job list. tnode_t first_job = job_list.try_get_child().child<0>(); @@ -823,13 +820,6 @@ parse_execution_result_t parse_execution_context_t::populate_plain_process( // Determine the process type. enum process_type_t process_type = process_type_for_command(statement, cmd); - // Check for stack overflow. - if (process_type == process_type_t::function && - parser->forbidden_function.size() > FISH_MAX_STACK_DEPTH) { - this->report_error(statement, CALL_STACK_LIMIT_EXCEEDED_ERR_MSG); - return parse_execution_errored; - } - // Protect against exec with background processes running if (process_type == process_type_t::exec && parser->is_interactive()) { bool have_bg = false; @@ -1451,21 +1441,24 @@ parse_execution_result_t parse_execution_context_t::eval_node(tnode_t block_io_push(&block_io, io); - enum parse_execution_result_t status = parse_execution_success; + + // Check for infinite recursion: a function which immediately calls itself.. wcstring func_name; auto infinite_recursive_node = this->infinite_recursive_statement_in_job_list(job_list, &func_name); if (infinite_recursive_node) { // We have an infinite recursion. - this->report_error(infinite_recursive_node, INFINITE_FUNC_RECURSION_ERR_MSG, - func_name.c_str()); - status = parse_execution_errored; - } else { - // No infinite recursion. - status = this->run_job_list(job_list, associated_block); + return this->report_error(infinite_recursive_node, INFINITE_FUNC_RECURSION_ERR_MSG, + func_name.c_str()); } - return status; + + // Check for stack overflow. The TOP check ensures we only do this for function calls. + if (associated_block->type() == TOP && parser->function_stack_is_overflowing()) { + return this->report_error(job_list, CALL_STACK_LIMIT_EXCEEDED_ERR_MSG); + } + return this->run_job_list(job_list, associated_block); } int parse_execution_context_t::line_offset_of_node(tnode_t node) { diff --git a/src/parser.cpp b/src/parser.cpp index 2c48686f1..2357f3794 100644 --- a/src/parser.cpp +++ b/src/parser.cpp @@ -240,10 +240,6 @@ block_t *parser_t::block_at_index(size_t idx) { block_t *parser_t::current_block() { return block_stack.empty() ? NULL : &block_stack.back(); } -void parser_t::forbid_function(const wcstring &function) { forbidden_function.push_back(function); } - -void parser_t::allow_function() { forbidden_function.pop_back(); } - /// Print profiling information to the specified stream. static void print_profile(const std::vector> &items, FILE *out) { for (size_t pos = 0; pos < items.size(); pos++) { @@ -510,6 +506,21 @@ const wchar_t *parser_t::current_filename() const { return libdata().current_filename; } +bool parser_t::function_stack_is_overflowing() const { + // We are interested in whether the count of functions on the stack exceeds + // FISH_MAX_STACK_DEPTH. We don't separately track the number of functions, but we can have a + // fast path through the eval_level. If the eval_level is in bounds, so must be the stack depth. + if (eval_level <= FISH_MAX_STACK_DEPTH) { + return false; + } + // Count the functions. + int depth = 0; + for (const auto &b : block_stack) { + depth += (b.type() == FUNCTION_CALL || b.type() == FUNCTION_CALL_NO_SHADOW); + } + return depth > FISH_MAX_STACK_DEPTH; +} + wcstring parser_t::current_line() { if (!execution_context) { return wcstring(); diff --git a/src/parser.h b/src/parser.h index 3ba18ab62..923851b81 100644 --- a/src/parser.h +++ b/src/parser.h @@ -193,8 +193,6 @@ class parser_t : public std::enable_shared_from_this { volatile sig_atomic_t cancellation_requested = false; /// The current execution context. std::unique_ptr execution_context; - /// List of called functions, used to help prevent infinite recursion. - wcstring_list_t forbidden_function; /// The jobs associated with this parser. job_list_t job_list; /// The list of blocks. This is a deque because we give out raw pointers to callers, who hold @@ -342,13 +340,6 @@ class parser_t : public std::enable_shared_from_this { void get_backtrace(const wcstring &src, const parse_error_list_t &errors, wcstring &output) const; - /// Tell the parser that the specified function may not be run if not inside of a conditional - /// block. This is to remove some possibilities of infinite recursion. - void forbid_function(const wcstring &function); - - /// Undo last call to parser_forbid_function(). - void allow_function(); - /// Output profiling data to the given filename. void emit_profiling(const char *path) const; @@ -364,6 +355,9 @@ class parser_t : public std::enable_shared_from_this { /// Return a string representing the current stack trace. wcstring stack_trace() const; + /// \return whether the number of functions in the stack exceeds our stack depth limit. + bool function_stack_is_overflowing() const; + /// \return a shared pointer reference to this parser. std::shared_ptr shared(); diff --git a/tests/checks/stack-overflow.fish b/tests/checks/stack-overflow.fish index 201a73666..e7d742135 100644 --- a/tests/checks/stack-overflow.fish +++ b/tests/checks/stack-overflow.fish @@ -275,3 +275,5 @@ left #CHECKERR: {{.*}} #CHECKERR: {{.*}} #CHECKERR: {{.*}} +#CHECKERR: {{.*}} +#CHECKERR: {{.*}}