kislyuk / argcomplete

Python and tab completion, better together.
https://kislyuk.github.io/argcomplete/
Apache License 2.0
1.39k stars 129 forks source link

No stdin for python calls from bash completion functions #488

Closed bfis closed 2 weeks ago

bfis commented 3 weeks ago

Prevents usage of stdin by (python) executables that are called during completion generation. This prevents the completion locking up the entire shell when the python script is broken i.e. it enters an interactive mode (REPL) instead of generating the completions, as expected.

This lockup can be provoked by having a script marked as compatible, but read stdin (& wait) for some reason e.g. due to REPL - I've added a corresponding test.

kislyuk commented 3 weeks ago

Thanks for your interest in argcomplete. Unfortunately I don't think we will be able to accept this pull request. I understand the problem that you're trying to solve, however replacing fd 0 can carry a lot of other side effects which might be more severe and more difficult to solve than simply advising users to not issue blocking reads from stdin before calling argcomplete.autocomplete(). For example, many command line applications try to query the tty properties on startup and might not properly function at all if fd 0 is not a tty.

I'm going to close this PR for now, but feel free to comment if you want to provide more information on why this might be the right approach, and we can reopen if necessary.

bfis commented 2 weeks ago

I'd argue that a program's autocomplete functionality silently breaking is vastly preferable to a user becoming stuck in an invisible program, especially since since this would happen during attempts to tab-complete where a lasting program execution is entirely unexpected.

Also, any program executed on a command line can be reasonable expected to have it's stdin replaced by a non-tty file, especially /dev/null which is commonly done e.g. when using nohup. If a program still needs access to the interactive prompt, it can query the controlling terminal (see ctermid), even if the stdin is replaced by /dev/null. This can be readily demonstrated by:

sudo sh -c "exit 36" </dev/null >/dev/null 2>/dev/null; echo $?
It should also be noted that the stuck script in the provided testcase, is only a minimal implementation to trigger the problematic behavior. It has the potential to be much worse. ```python #!/usr/bin/env python # PYTHON_ARGCOMPLETE_OK from signal import pthread_sigmask, valid_signals, SIG_SETMASK pthread_sigmask(SIG_SETMASK, valid_signals()) while True: input() ```

In general, i think it is a poor idea to execute effectively arbitrary commands in an automatic fashion where the stdin is still passed to the program but all output (stdout and especially stderr) is hidden. As it is currently implemented, if the bug is triggered, a user will have no indication what it happening and only experience a potentially permanently blocked shell prompt, after not even executing something but merely attempting to autocomplete - which is generally understood to be non-invasive.

If some programs using argcomplete break with this PR's change, their error outputs would be hidden anyway and the completion would simply fail - a much less severe situation than invisibly running an interactive/blocking program. Such affected programs would likely experience unexpected errors, such as an EOFError for effectively running:

 python3 -c "input()" </dev/null

Still, I understand that this PR's change is not some minimal patch but has the potential to impact a noticeable number of users, which should be handled appropriately e.g. only be included in a minor (or even major) version release.

kislyuk commented 2 weeks ago

OK, those are some good points. Another way of putting it is, stdin being a tty can reasonably be expected to be an indicator for direct interaction with the user, but collecting completions is not direct interaction with the user.

It turns out that zsh already shuts off stdin when running completions, so this issue only affects bash. I'm going to think about this a bit more and will likely merge this PR as is.

this would happen during attempts to tab-complete where a lasting program execution is entirely unexpected

That's a good point too. I'm thinking maybe argcomplete should spawn background watchdog shellcode for every completion run, to kill the python process after a timeout.

bfis commented 2 weeks ago

this would happen during attempts to tab-complete where a lasting program execution is entirely unexpected

That's a good point too. I'm thinking maybe argcomplete should spawn background watchdog shellcode for every completion run, to kill the python process after a timeout.

This could easily be implemented via timeout, which should be fairly widely available.

kislyuk commented 2 weeks ago

Good suggestion, but I'm not ready to introduce a dependency that's not based on a shell builtin.