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

@@ -814,11 +814,9 @@ static bool exec_block_or_func_process(parser_t &parser, std::shared_ptr<job_t>
         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.

src/parse_execution.cpp

@@ -108,11 +108,8 @@ tnode_t<g::plain_statement> 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<g::job> first_job = job_list.try_get_child<g::job_conjunction, 1>().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<g::job_lis
     // Apply this block IO for the duration of this function.
     assert(job_list && "Empty node in eval_node");
     assert(job_list.matches_node_tree(tree()) && "job_list has unexpected tree");
+    assert(associated_block && "Null block");
     scoped_push<io_chain_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<g::job> node) {

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<std::unique_ptr<profile_item_t>> &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();

src/parser.h

@@ -193,8 +193,6 @@ class parser_t : public std::enable_shared_from_this<parser_t> {
     volatile sig_atomic_t cancellation_requested = false;
     /// The current execution context.
     std::unique_ptr<parse_execution_context_t> 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<parser_t> {
     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<parser_t> {
     /// 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<parser_t> shared();
 

tests/checks/stack-overflow.fish

@@ -275,3 +275,5 @@ left
 #CHECKERR: {{.*}}
 #CHECKERR: {{.*}}
 #CHECKERR: {{.*}}
+#CHECKERR: {{.*}}
+#CHECKERR: {{.*}}