scop / bash-completion

Programmable completion functions for bash
GNU General Public License v2.0
2.92k stars 380 forks source link

Aborting a completion attempt leaves `set -o noglob` in effect #786

Open abbeyj opened 2 years ago

abbeyj commented 2 years ago

Describe the bug

Certain completions turn off globbing with set -o noglob, perform some operation, and then attempt to reset globbing back to its original state. See for example https://github.com/scop/bash-completion/blob/36ceb272ddf7ef70b7fa79c5c3686080b1510054/bash_completion#L758-L763 but this pattern is used in more than one place.

Unfortunately if the user presses Ctrl+C in the middle of this then the reset code is never run and the shell is left with set -o noglob in effect. This will likely be very disconcerting for the user. For instance, running ls * reports ls: cannot access *: No such file or directory. That's the message you'd normally expect to see if the directory is empty but now you'll see it reported for all directories, giving the impression that all of your files have been deleted.

A user could recover from this by running set +o noglob but they'd have to realize what was wrong first. I'm assuming in most cases people will just think their terminal is "broken" and be forced to close it and open a new one.

See also #691 .

To reproduce

You'll need a directory somewhere that's very slow to enumerate. One option would be to mount an NFS directory from some slow, distant machine. Another would be to create a local directory with an absurdly large number of files in it and have a cold cache (possibly something like echo 3 > /proc/sys/vm/drop_caches might help on Linux). Or maybe https://serverfault.com/a/954175. I can personally reproduce this with a large directory on NFS.

# Verify that globbing is turned on to start
$ shopt -po noglob
set +o noglob

$ cd /path/to/your/directory/<TAB><Ctrl+C>

# Globbing is now unexpectedly turned off
$ shopt -po noglob
set -o noglob

Expected behavior

The noglob setting should retain the previous value.

Versions (please complete the following information)

Additional context

Maybe this could be handled by running the compgen in a subshell so that the setting would not have to be remembered and then explicitly restored?

Debug trace

I'm going to avoid including entire thing for now since it is large and I don't think it will help explain the problem. If you still need it, please tell me and I'll send the full one. It ends with this, the compgen having been interrupted by Ctrl+C:

+ local -a toks
+ local reset arg=-d
+ [[ -d == -d ]]
++ shopt -po noglob
+ reset='set +o noglob'
+ set -o noglob
+ toks=($(compgen -d -- "${cur-}"))
++ compgen -d -- /path/to/your/directory/
^C
$

I can't reproduce the problem on a different system that uses bash 5.1. I believe that's because of a recent change to bash, introduced in v5.1, that masks SIGINT during the execution of compgen: https://github.com/bminor/bash/blob/9439ce094c9aa7557a9d53ac7b412a23aa66e36b/subst.c#L6542. This makes the code in bash-completion work as expected but has the downside that you can't abort the completion and have to wait for it to finish no matter how long it takes.

scop commented 2 years ago

Hmph, yeah, I can see how that would happen. I wonder if a simple trap "$reset" RETURN or a derivative would be a pattern we could use to avoid this.

Semi-related: https://github.com/scop/bash-completion/pull/739

akinomyoga commented 2 years ago

I wonder if a simple trap "$reset" RETURN or a derivative would be a pattern we could use to avoid this.

I have thought the same thing after I first read this issue and tried it, but RETURN doesn't seem to be invoked when the completion is canceled by SIGINT. Currently, I don't come up with a good solution for this issue.

scop commented 2 years ago

RETURN doesn't seem to be invoked when the completion is canceled by SIGINT

Would trap '$reset' INT RETURN "fix" the general signal part of this problem? Managing original traps to those would still be needed I guess.

give up immediately restoring the state, but to restore the state on the next call of the completion

I'm not sure I follow, but wouldn't this leave the altered state in effect for the user's shell until the next call of the completion?

akinomyoga commented 2 years ago

RETURN doesn't seem to be invoked when the completion is canceled by SIGINT

Would trap '$reset' INT RETURN "fix" the general signal part of this problem? Managing original traps to those would still be needed I guess.

I think the problem with recovering the options could be solved, but then there arises another problem of the completion that can take a long time not being able to be canceled. Specifically, OP describes the case where the filename completion is attempted in a directory with many files on remote filesystems.

sidenote: implementation in ble.sh I now remembered that I have implemented in ble.sh more complicated processing for this issue of slow filename completions. I attempt the pathname expansions (aka glob expansions) in a background subshell, and wait for its termination or user inputs in the parent shell. The subshell is killed when there are any user inputs before its termination. In this case, the result needs to be passed to the parent shell after the completion of the pathname expansions. Here, the next problem is how to effectively pass the expansion results to the parent shell. It turned out that `result=($very_long_list)` can be as slow as the expansion itself and this cannot be canceled when we suppress the signals. I instead parse the result of `declare -p result` by `awk`. write a NUL-separated list of result elements to file, and read it using `mapfile` in the latest version of Bash. Adjustments are needed for older versions of Bash where `mapfile` doesn't support an option `-d ''` for the NUL delimiter. Other completions that might take a long time are handled in a similar way separately for each case.

give up immediately restoring the state, but to restore the state on the next call of the completion

I'm not sure I follow, but wouldn't this leave the altered state in effect for the user's shell until the next call of the completion?

Right. So this is merely a partial solution in case all the other solutions are unavailable.

akinomyoga commented 2 years ago

It turned out that the ERR trap seems to be reliably called in Bash 4.4+, though it doesn't work in Bash 4.2 and 4.3.

$ bash-dev --norc
$ function f1 { trap 'echo f1/ERR:$FUNCNAME' ERR; f2; echo never; false; }; function f2 { sleep 1; }; f1
^Cf1/ERR:f1

$ exit
$ bash-4.4 --norc
$ function f1 { trap 'echo f1/ERR:$FUNCNAME' ERR; f2; echo never; false; }; function f2 { sleep 1; }; f1
^Cf1/ERR:f1

$ exit
$ bash-4.3 --norc
$ function f1 { trap 'echo f1/ERR:$FUNCNAME' ERR; f2; echo never; false; }; function f2 { sleep 1; }; f1
^C
$

edit: Anyway, the user's ERR trap needs to be saved and restored.

akinomyoga commented 1 year ago

I think another way to make sure to recover the original settings is to recheck them before running the user commands.

Note: All of these approaches might be broken by the user's other settings overwriting DEBUG, PS0, and keybindings.

We might combine one of the above with #739 _comp_finalize. We might also handle $_ in #901 with the same approach.