From c9f431a0b7c4624625a47fca43ecb26c25e6449f Mon Sep 17 00:00:00 2001 From: Andy Chu Date: Sun, 3 Feb 2019 02:14:27 -0800 Subject: [PATCH] [completion] Fix two sources of infinite loops. - The basename() test in GetSpecForFirst() was broken, which caused an infinite loop when running ~/.local/bin/mypy. Fixed with unit test. - The 124 protocol now tests if the completion spec was changed before retrying. This prevents a function that simply does 'return 124' from entering an infinite loop. The unit tests have coverage for this case via 'return-124.bash'. Addresses issue #208. --- bin/oil.py | 4 ++- core/completion.py | 39 +++++++++++++++++++++++++--- core/completion_test.py | 35 ++++++++++++++++++++++--- core/test_lib.py | 3 ++- osh/builtin_comp.py | 5 ++-- testdata/completion/return-124.bash | 40 +++++++++++++++++++++++++++++ 6 files changed, 115 insertions(+), 11 deletions(-) create mode 100644 testdata/completion/return-124.bash diff --git a/bin/oil.py b/bin/oil.py index 5d9f2febf4..9b9203d1b8 100755 --- a/bin/oil.py +++ b/bin/oil.py @@ -392,7 +392,9 @@ def ShellMain(lang, argv0, argv, login_shell): ex.bool_ev = bool_ev ex.tracer = tracer - spec_builder = builtin_comp.SpecBuilder(ex, parse_ctx, word_ev, splitter) + spec_builder = builtin_comp.SpecBuilder(ex, parse_ctx, word_ev, splitter, + comp_lookup) + # Add some builtins that depend on the executor! complete_builtin = builtin_comp.Complete(spec_builder, comp_lookup) builtins[builtin_e.COMPLETE] = complete_builtin diff --git a/core/completion.py b/core/completion.py index 7ba4cfd08e..92243a563e 100755 --- a/core/completion.py +++ b/core/completion.py @@ -146,6 +146,8 @@ def __init__(self): '__first': _DO_NOTHING, } + self.commands_with_spec_changes = [] # for the 124 protocol + # So you can register *.sh, unlike bash. List of (glob, [actions]), # searched linearly. self.patterns = [] @@ -162,12 +164,21 @@ def PrintSpecs(self): for pat, spec in self.patterns: print('%s = %s' % (pat, spec)) + def ClearCommandsChanged(self): + del self.commands_with_spec_changes[:] + + def GetCommandsChanged(self): + return self.commands_with_spec_changes + def RegisterName(self, name, base_opts, user_spec): """Register a completion action with a name. Used by the 'complete' builtin. """ self.lookup[name] = (base_opts, user_spec) + if name not in ('__fallback', '__first'): + self.commands_with_spec_changes.append(name) + def RegisterGlob(self, glob_pat, base_opts, user_spec): self.patterns.append((glob_pat, base_opts, user_spec)) @@ -181,7 +192,7 @@ def GetSpecForName(self, argv0): return pair key = os_path.basename(argv0) - actions = self.lookup.get(key) + pair = self.lookup.get(key) if pair: return pair @@ -353,9 +364,15 @@ def Matches(self, comp): class ShellFuncAction(CompletionAction): """Call a user-defined function using bash's completion protocol.""" - def __init__(self, ex, func): + def __init__(self, ex, func, comp_lookup): + """ + Args: + comp_lookup: For the 124 protocol: test if the user-defined function + registered a new UserSpec. + """ self.ex = ex self.func = func + self.comp_lookup = comp_lookup def __repr__(self): # TODO: Add file and line number here! @@ -392,10 +409,24 @@ def Matches(self, comp): self.log('Running completion function %r with arguments %s', self.func.name, argv) + self.comp_lookup.ClearCommandsChanged() status = self.ex.RunFuncForCompletion(self.func, argv) + commands_changed = self.comp_lookup.GetCommandsChanged() + + self.log('comp.first %s, commands_changed: %s', comp.first, + commands_changed) + if status == 124: - self.log('Got status 124 from %r', self.func.name) - raise _RetryCompletion() + cmd = os_path.basename(comp.first) + if cmd in commands_changed: + self.log('Got status 124 from %r and %s commands changed', + self.func.name, commands_changed) + raise _RetryCompletion() + else: + util.error( + "Function %r returned 124, but the completion spec for %r wasn't " + "changed", self.func.name, cmd) + return [] # Read the response. We set it above, so this error would only happen if # the user unset it. diff --git a/core/completion_test.py b/core/completion_test.py index 5d601e6950..2bf69eb9de 100755 --- a/core/completion_test.py +++ b/core/completion_test.py @@ -107,8 +107,12 @@ def _MakeComp(self, words, index, to_complete): def testLookup(self): c = completion.Lookup() c.RegisterName('grep', BASE_OPTS, U1) - print(c.GetSpecForName('grep')) - print(c.GetSpecForName('/usr/bin/grep')) + + _, user_spec = c.GetSpecForName('grep') + self.assertEqual(1, len(user_spec.actions)) + + _, user_spec = c.GetSpecForName('/usr/bin/grep') + self.assertEqual(1, len(user_spec.actions)) c.RegisterGlob('*.py', BASE_OPTS, U1) base_opts, comp = c.GetSpecForName('/usr/bin/foo.py') @@ -189,7 +193,8 @@ def testShellFuncExecution(self): ex = test_lib.InitExecutor(arena=arena) - a = completion.ShellFuncAction(ex, func_node) + comp_lookup = completion.Lookup() + a = completion.ShellFuncAction(ex, func_node, comp_lookup) comp = self._MakeComp(['f'], 0, 'f') matches = list(a.Matches(comp)) self.assertEqual(['f1', 'f2'], matches) @@ -489,6 +494,30 @@ def testCompletesAliases(self): m = list(r.Matches(MockApi('ll_own_completion '))) self.assertEqual(['own ', 'words '], sorted(m)) + def testNoInfiniteLoop(self): + # This was ONE place where we got an infinite loop. + + with open('testdata/completion/return-124.bash') as f: + code_str = f.read() + trail = parse_lib.Trail() + arena = alloc.SideArena('') + parse_ctx = parse_lib.ParseContext(arena, {}, trail=trail) + comp_lookup = completion.Lookup() + ex = test_lib.EvalCode(code_str, parse_ctx, comp_lookup=comp_lookup) + + r = _MakeRootCompleter(parse_ctx=parse_ctx, comp_lookup=comp_lookup) + + m = list(r.Matches(MockApi('bad '))) + self.assertEqual([], sorted(m)) + + # Error: spec not changed + m = list(r.Matches(MockApi('both '))) + self.assertEqual([], sorted(m)) + + # Redefines completions + m = list(r.Matches(MockApi('both2 '))) + self.assertEqual(['b1 ', 'b2 '], sorted(m)) + def testCompletesAssignment(self): # OSH doesn't do this. Here is noticed about bash --norc (which is # undoubtedly different from bash_completion): diff --git a/core/test_lib.py b/core/test_lib.py index 3f384ab09a..b07a7607d3 100755 --- a/core/test_lib.py +++ b/core/test_lib.py @@ -170,7 +170,8 @@ def InitExecutor(parse_ctx=None, comp_lookup=None, arena=None, mem=None): ex = cmd_exec.Executor(mem, fd_state, funcs, builtins, exec_opts, parse_ctx, exec_deps) - spec_builder = builtin_comp.SpecBuilder(ex, parse_ctx, word_ev, splitter) + spec_builder = builtin_comp.SpecBuilder(ex, parse_ctx, word_ev, splitter, + comp_lookup) # Add some builtins that depend on the executor! complete_builtin = builtin_comp.Complete(spec_builder, comp_lookup) # used later builtins[builtin_e.COMPLETE] = complete_builtin diff --git a/osh/builtin_comp.py b/osh/builtin_comp.py index 92c898c881..9b79935498 100644 --- a/osh/builtin_comp.py +++ b/osh/builtin_comp.py @@ -91,7 +91,7 @@ def Matches(self, comp): class SpecBuilder(object): - def __init__(self, ex, parse_ctx, word_ev, splitter): + def __init__(self, ex, parse_ctx, word_ev, splitter, comp_lookup): """ Args: ex: Executor for compgen -F @@ -101,6 +101,7 @@ def __init__(self, ex, parse_ctx, word_ev, splitter): self.parse_ctx = parse_ctx self.word_ev = word_ev self.splitter = splitter + self.comp_lookup = comp_lookup def Build(self, argv, arg, base_opts): """Given flags to complete/compgen, return a UserSpec.""" @@ -115,7 +116,7 @@ def Build(self, argv, arg, base_opts): func = ex.funcs.get(func_name) if func is None: raise args.UsageError('Function %r not found' % func_name) - actions.append(completion.ShellFuncAction(ex, func)) + actions.append(completion.ShellFuncAction(ex, func, self.comp_lookup)) # NOTE: We need completion for -A action itself!!! bash seems to have it. for name in arg.actions: diff --git a/testdata/completion/return-124.bash b/testdata/completion/return-124.bash new file mode 100644 index 0000000000..a877ba7949 --- /dev/null +++ b/testdata/completion/return-124.bash @@ -0,0 +1,40 @@ +_bad() { + #argv "$@" + + echo '_bad returning 124' + + # This caused an infinite loop in OSH, but not in bash. We have to test if + # the return value is 124 AND the compspec was updated. + # + # In bash, it seems like you EITHER set COMPREPLY or return 124, not BOTH! + # If it sees 124, it doesn't process the completions (unlike OSH at the + # moment). + + #COMPREPLY=(x y) + + return 124 +} +complete -F _bad bad + +_both() { + #echo '_both setting COMPREPLY and returning 124' + COMPREPLY=(x y) + return 124 +} +complete -F _both both + +_both2() { + #echo '_both setting COMPREPLY and returning 124' + COMPREPLY=(x y) + complete -W 'b1 b2' both2 + return 124 +} +complete -F _both2 both2 + +_default() { + echo '_default returning 124 without changing completion spec' + # We're supposed to source something here, but we didn't + return 124 +} + +complete -F _default -D