From 45e7c709f43c6520e772eaf83e24718aff2588b2 Mon Sep 17 00:00:00 2001 From: Johannes Altmanninger Date: Sat, 26 Sep 2020 14:13:56 +0200 Subject: [PATCH] Consolidate complete cycle detection and always report error on cycle Detect recursive calls to builtin complete and the internal completion in the same place. In 0a0149cc2 (Prevent infinite recursion when completion wraps variable assignment) we don't print an error when completing certain aliases like: alias vim "A=B vim" But we also gave no completions. We could make this case work, but I think that trying to salvage situations like this one is way too complex. Instead, let the user know by printing an error. Not sure if the style of the error fits. We could add some heuristic to alias to not add --wraps in some cyclic cases. --- src/builtin_complete.cpp | 11 ++--------- src/complete.cpp | 23 ++++++++++++++++------- src/parser.h | 4 ++-- tests/checks/complete.fish | 4 ++-- tests/checks/wraps.fish | 3 +++ 5 files changed, 25 insertions(+), 20 deletions(-) diff --git a/src/builtin_complete.cpp b/src/builtin_complete.cpp index 934277477..de191d4d4 100644 --- a/src/builtin_complete.cpp +++ b/src/builtin_complete.cpp @@ -359,14 +359,8 @@ maybe_t builtin_complete(parser_t &parser, io_streams_t &streams, wchar_t * parser.libdata().transient_commandlines.push_back(do_complete_param); cleanup_t remove_transient([&] { parser.libdata().transient_commandlines.pop_back(); }); - if (parser.libdata().builtin_complete_current_commandline) { - // Prevent accidental recursion (see #6171). - } else if (parser.libdata().builtin_complete_recursion_level >= 24) { - // Allow a limited number of recursive calls to complete (#3474). - streams.err.append_format(L"%ls: maximum recursion depth reached\n", cmd); - } else { - parser.libdata().builtin_complete_recursion_level++; - assert(!parser.libdata().builtin_complete_current_commandline); + // Prevent accidental recursion (see #6171). + if (!parser.libdata().builtin_complete_current_commandline) { if (!have_do_complete_param) parser.libdata().builtin_complete_current_commandline = true; @@ -403,7 +397,6 @@ maybe_t builtin_complete(parser_t &parser, io_streams_t &streams, wchar_t * streams.out.push_back(L'\n'); } - parser.libdata().builtin_complete_recursion_level--; parser.libdata().builtin_complete_current_commandline = false; } } else if (path.empty() && gnu_opt.empty() diff --git a/src/complete.cpp b/src/complete.cpp index 8b10f1df7..a7327c97c 100644 --- a/src/complete.cpp +++ b/src/complete.cpp @@ -1379,13 +1379,7 @@ void completer_t::complete_custom(const wcstring &cmd, const wcstring &cmdline, ctx.parser->libdata().transient_commandlines.push_back(unaliased_cmd); cleanup_t remove_transient( [&] { ctx.parser->libdata().transient_commandlines.pop_back(); }); - // Prevent infinite recursion when the completion for x wraps "A=B x" (#7344). - // Don't report an error since this could be a legitimate alias. - static uint32_t complete_assignment_recursion_count; - if (complete_assignment_recursion_count++ < 24) { - run_on_command(std::move(unaliased_cmd)); - } - complete_assignment_recursion_count--; + perform_for_command(std::move(unaliased_cmd)); *do_file = false; } else if (!complete_param(cmd, previous_argument, current_argument, !had_ddash)) { // Invoke any custom completions for this command. @@ -1511,6 +1505,21 @@ void completer_t::mark_completions_duplicating_arguments(const wcstring &cmd, } void completer_t::perform_for_command(wcstring cmd) { + // Limit recursion, in case a user-defined completion has cycles, or the completion for "x" + // wraps "A=B x" (#3474, #7344). No need to do that when there is no parser: this happens only + // for autosuggestions where we don't evaluate command substitutions or variable assignments. + if (ctx.parser) { + if (ctx.parser->libdata().complete_recursion_level >= 24) { + debug(0, _(L"completion reached maximum recursion depth, possible cycle?"), + cmd.c_str()); + return; + } + ++ctx.parser->libdata().complete_recursion_level; + }; + cleanup_t decrement{[this]() { + if (ctx.parser) --ctx.parser->libdata().complete_recursion_level; + }}; + const size_t cursor_pos = cmd.size(); // Find the plain statement to operate on. The cursor may be past it (#1261), so backtrack diff --git a/src/parser.h b/src/parser.h index 7c8b37b01..aec1e934c 100644 --- a/src/parser.h +++ b/src/parser.h @@ -148,8 +148,8 @@ struct library_data_t { /// Last reader run count. uint64_t last_exec_run_counter{UINT64_MAX}; - /// Number of recursive calls to builtin_complete(). - uint32_t builtin_complete_recursion_level{0}; + /// Number of recursive calls to the internal completion function. + uint32_t complete_recursion_level{0}; /// If we're currently repainting the commandline. /// Useful to stop infinite loops. diff --git a/tests/checks/complete.fish b/tests/checks/complete.fish index e463490ed..7f5583a34 100644 --- a/tests/checks/complete.fish +++ b/tests/checks/complete.fish @@ -65,7 +65,7 @@ complete # Recursive calls to complete (see #3474) complete -c complete_test_recurse1 -xa '(echo recursing 1>&2; complete -C"complete_test_recurse1 ")' -complete -C'complete_test_recurse1 ' +LANG=C complete -C'complete_test_recurse1 ' # CHECKERR: recursing # CHECKERR: recursing # CHECKERR: recursing @@ -90,7 +90,7 @@ complete -C'complete_test_recurse1 ' # CHECKERR: recursing # CHECKERR: recursing # CHECKERR: recursing -# CHECKERR: complete: maximum recursion depth reached +# CHECKERR: fish: completion reached maximum recursion depth, possible cycle? # short options complete -c foo -f -a non-option-argument diff --git a/tests/checks/wraps.fish b/tests/checks/wraps.fish index cd85051b3..98c014ba5 100644 --- a/tests/checks/wraps.fish +++ b/tests/checks/wraps.fish @@ -1,6 +1,8 @@ #RUN: %fish %s # Validate some things about command wrapping. +set -g LANG C # For predictable error messages. + # This tests that we do not trigger a combinatorial explosion - see #5638. # Ensure it completes successfully. complete -c testcommand --wraps "testcommand x " @@ -30,3 +32,4 @@ complete -C'testcommand2 explicit ' complete -c recvar --wraps 'A=B recvar' complete -C 'recvar ' +# CHECKERR: fish: completion reached maximum recursion depth, possible cycle?